Commit 7681bfee authored by Yasuaki Ishimatsu's avatar Yasuaki Ishimatsu Committed by Jens Axboe
Browse files

block: fix accounting bug on cross partition merges 



/proc/diskstats would display a strange output as follows.

$ cat /proc/diskstats |grep sda
   8       0 sda 90524 7579 102154 20464 0 0 0 0 0 14096 20089
   8       1 sda1 19085 1352 21841 4209 0 0 0 0 4294967064 15689 4293424691
                                                ~~~~~~~~~~
   8       2 sda2 71252 3624 74891 15950 0 0 0 0 232 23995 1562390
   8       3 sda3 54 487 2188 92 0 0 0 0 0 88 92
   8       4 sda4 4 0 8 0 0 0 0 0 0 0 0
   8       5 sda5 81 2027 2130 138 0 0 0 0 0 87 137

Its reason is the wrong way of accounting hd_struct->in_flight. When a bio is
merged into a request belongs to different partition by ELEVATOR_FRONT_MERGE.

The detailed root cause is as follows.

Assuming that there are two partition, sda1 and sda2.

1. A request for sda2 is in request_queue. Hence sda1's hd_struct->in_flight
   is 0 and sda2's one is 1.

        | hd_struct->in_flight
   ---------------------------
   sda1 |          0
   sda2 |          1
   ---------------------------

2. A bio belongs to sda1 is issued and is merged into the request mentioned on
   step1 by ELEVATOR_BACK_MERGE. The first sector of the request is changed
   from sda2 region to sda1 region. However the two partition's
   hd_struct->in_flight are not changed.

        | hd_struct->in_flight
   ---------------------------
   sda1 |          0
   sda2 |          1
   ---------------------------

3. The request is finished and blk_account_io_done() is called. In this case,
   sda2's hd_struct->in_flight, not a sda1's one, is decremented.

        | hd_struct->in_flight
   ---------------------------
   sda1 |         -1
   sda2 |          1
   ---------------------------

The patch fixes the problem by caching the partition lookup
inside the request structure, hence making sure that the increment
and decrement will always happen on the same partition struct. This
also speeds up IO with accounting enabled, since it cuts down on
the number of lookups we have to do.

When reloading partition tables, quiesce IO to ensure that no
request references to the partition struct exists. When it is safe
to free the partition table, the IO for that device is restarted
again.

Signed-off-by: default avatarYasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Cc: stable@kernel.org
Signed-off-by: default avatarJens Axboe <jaxboe@fusionio.com>
parent 495d2b38

block/blk-core.c

+16 −8
Original line number Diff line number Diff line
@@ -64,13 +64,15 @@ 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);

		rq->part = part;

	}



	part_stat_unlock();
@@ -128,6 +130,7 @@ void blk_rq_init(struct request_queue *q, struct request *rq) 
	rq->ref_count = 1;

	rq->start_time = jiffies;

	set_start_time_ns(rq);

	rq->part = NULL;

}

EXPORT_SYMBOL(blk_rq_init);
@@ -804,11 +807,16 @@ static struct request *get_request(struct request_queue *q, int rw_flags, 
	rl->starved[is_sync] = 0;



	priv = !test_bit(QUEUE_FLAG_ELVSWITCH, &q->queue_flags);

	if (priv)

	if (priv) {

		rl->elvpriv++;



		/*

		 * Don't do stats for non-priv requests

		 */

		if (blk_queue_io_stat(q))

			rw_flags |= REQ_IO_STAT;

	}



	spin_unlock_irq(q->queue_lock);



	rq = blk_alloc_request(q, rw_flags, priv, gfp_mask);
@@ -1777,7 +1785,7 @@ static void blk_account_io_completion(struct request *req, unsigned int bytes) 
		int cpu;



		cpu = part_stat_lock();

		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));

		part = req->part;

		part_stat_add(cpu, part, sectors[rw], bytes >> 9);

		part_stat_unlock();

	}
@@ -1797,7 +1805,7 @@ static void blk_account_io_done(struct request *req) 
		int cpu;



		cpu = part_stat_lock();

		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));

		part = req->part;



		part_stat_inc(cpu, part, ios[rw]);

		part_stat_add(cpu, part, ticks[rw], duration);

block/blk-merge.c

+1 −1
Original line number Diff line number Diff line
@@ -351,7 +351,7 @@ static void blk_account_io_merge(struct request *req) 
		int cpu;



		cpu = part_stat_lock();

		part = disk_map_sector_rcu(req->rq_disk, blk_rq_pos(req));

		part = req->part;



		part_round_stats(cpu, part);

		part_dec_in_flight(part, rq_data_dir(req));

block/blk.h

+0 −4
Original line number Diff line number Diff line
@@ -110,10 +110,6 @@ void blk_queue_congestion_threshold(struct request_queue *q); 


int blk_dev_init(void);



void elv_quiesce_start(struct request_queue *q);

void elv_quiesce_end(struct request_queue *q);





/*

 * Return the threshold (number of used requests) at which the queue is

 * considered to be congested.  It include a little hysteresis to keep the

block/genhd.c

+14 −0
Original line number Diff line number Diff line
@@ -932,8 +932,15 @@ static void disk_free_ptbl_rcu_cb(struct rcu_head *head) 
{

	struct disk_part_tbl *ptbl =

		container_of(head, struct disk_part_tbl, rcu_head);

	struct gendisk *disk = ptbl->disk;

	struct request_queue *q = disk->queue;

	unsigned long flags;



	kfree(ptbl);



	spin_lock_irqsave(q->queue_lock, flags);

	elv_quiesce_end(q);

	spin_unlock_irqrestore(q->queue_lock, flags);

}



/**
@@ -951,11 +958,17 @@ static void disk_replace_part_tbl(struct gendisk *disk, 
				  struct disk_part_tbl *new_ptbl)

{

	struct disk_part_tbl *old_ptbl = disk->part_tbl;

	struct request_queue *q = disk->queue;



	rcu_assign_pointer(disk->part_tbl, new_ptbl);



	if (old_ptbl) {

		rcu_assign_pointer(old_ptbl->last_lookup, NULL);



		spin_lock_irq(q->queue_lock);

		elv_quiesce_start(q);

		spin_unlock_irq(q->queue_lock);



		call_rcu(&old_ptbl->rcu_head, disk_free_ptbl_rcu_cb);

	}

}
@@ -996,6 +1009,7 @@ int disk_expand_part_tbl(struct gendisk *disk, int partno) 
		return -ENOMEM;



	new_ptbl->len = target;

	new_ptbl->disk = disk;



	for (i = 0; i < len; i++)

		rcu_assign_pointer(new_ptbl->part[i], old_ptbl->part[i]);

fs/partitions/check.c

+12 −0
Original line number Diff line number Diff line
@@ -365,17 +365,25 @@ struct device_type part_type = { 
static void delete_partition_rcu_cb(struct rcu_head *head)

{

	struct hd_struct *part = container_of(head, struct hd_struct, rcu_head);

	struct gendisk *disk = part_to_disk(part);

	struct request_queue *q = disk->queue;

	unsigned long flags;



	part->start_sect = 0;

	part->nr_sects = 0;

	part_stat_set_all(part, 0);

	put_device(part_to_dev(part));



	spin_lock_irqsave(q->queue_lock, flags);

	elv_quiesce_end(q);

	spin_unlock_irqrestore(q->queue_lock, flags);

}



void delete_partition(struct gendisk *disk, int partno)

{

	struct disk_part_tbl *ptbl = disk->part_tbl;

	struct hd_struct *part;

	struct request_queue *q = disk->queue;



	if (partno >= ptbl->len)

		return;
@@ -390,6 +398,10 @@ void delete_partition(struct gendisk *disk, int partno) 
	kobject_put(part->holder_dir);

	device_del(part_to_dev(part));



	spin_lock_irq(q->queue_lock);

	elv_quiesce_start(q);

	spin_unlock_irq(q->queue_lock);



	call_rcu(&part->rcu_head, delete_partition_rcu_cb);

}

CRA Git | Maintained and supported by SUSTech CRA and CCSE