Re: [PATCH] block: fix accounting bug on cross partition merges

[Jens Axboe]

On 2010-12-17 14:42, Jerome Marchand wrote:
> 
> /proc/diskstats would display a strange output as follows.

[snip]

This looks a lot better! One comment:

> diff --git a/block/blk-core.c b/block/blk-core.c
> index 4ce953f..064921d 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -64,13 +64,16 @@ static void drive_stat_acct(struct request *rq, int new_io)
>  		return;
>  
>  	cpu = part_stat_lock();
> -	part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
>  
> -	if (!new_io)
> +	if (!new_io) {
> +		part = rq->part;
>  		part_stat_inc(cpu, part, merges[rw]);
> -	else {
> +	} else {
> +		part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq));
>  		part_round_stats(cpu, part);
>  		part_inc_in_flight(part, rw);
> +		kref_get(&part->ref);
> +		rq->part = part;
>  	}
>  
>  	part_stat_unlock();

I don't think this is completely safe. The rcu lock is held due to the
part_stat_lock(), but that only prevents the __delete_partition()
callback from happening. Lets say you have this:

CPU0                                         CPU1
part = disk_map_sector_rcu()
                                             kref_put(part); <- now 0
part_stat_unlock()
                                             __delete_partition();
                                             ...
                                             delete_partition_rcu_cb();
merge, or endio, boom

Now rq has ->part pointing to freed memory, later merges or end
accounting will touch freed memory.

I think we can fix this by just having delete_partition_rcu_rb() check
the reference count and return if non-zero. Since someone holds a
reference to the table, they will drop it and we'll re-schedule the rcu
callback.


-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/