From 09e099d4bafea3b15be003d548bdf94b4b6e0e17 Mon Sep 17 00:00:00 2001 From: Jerome Marchand Date: Wed, 5 Jan 2011 16:57:38 +0100 Subject: 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. Also add a refcount to struct hd_struct to keep the partition in memory as long as users exist. We use kref_test_and_get() to ensure we don't add a reference to a partition which is going away. Signed-off-by: Jerome Marchand Signed-off-by: Yasuaki Ishimatsu Cc: stable@kernel.org Signed-off-by: Jens Axboe --- block/blk-merge.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'block/blk-merge.c') diff --git a/block/blk-merge.c b/block/blk-merge.c index 77b7c26df6b50..b06b83b89d896 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -351,11 +351,12 @@ 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)); + kref_put(&part->ref, __delete_partition); part_stat_unlock(); } } -- cgit v1.2.3 From 6c23a9681c0fe7fb7dd331b39dda11926f43746e Mon Sep 17 00:00:00 2001 From: Jens Axboe Date: Fri, 7 Jan 2011 08:43:37 +0100 Subject: block: add internal hd part table references We can't use krefs since it's apparently restricted to very basic reference counting. This reverts commit e4a683c8. Signed-off-by: Jens Axboe --- block/blk-core.c | 6 +++--- block/blk-merge.c | 2 +- block/genhd.c | 3 ++- fs/partitions/check.c | 8 +++----- include/linux/genhd.h | 27 +++++++++++++++++++++++++-- include/linux/kref.h | 1 - lib/kref.c | 12 ------------ 7 files changed, 34 insertions(+), 25 deletions(-) (limited to 'block/blk-merge.c') diff --git a/block/blk-core.c b/block/blk-core.c index 500c080a6a6b3..2f4002f79a24b 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -70,7 +70,7 @@ static void drive_stat_acct(struct request *rq, int new_io) part_stat_inc(cpu, part, merges[rw]); } else { part = disk_map_sector_rcu(rq->rq_disk, blk_rq_pos(rq)); - if (!kref_test_and_get(&part->ref)) { + if (!hd_struct_try_get(part)) { /* * The partition is already being removed, * the request will be accounted on the disk only @@ -80,7 +80,7 @@ static void drive_stat_acct(struct request *rq, int new_io) * it as any other partition. */ part = &rq->rq_disk->part0; - kref_get(&part->ref); + hd_struct_get(part); } part_round_stats(cpu, part); part_inc_in_flight(part, rw); @@ -1818,7 +1818,7 @@ static void blk_account_io_done(struct request *req) part_round_stats(cpu, part); part_dec_in_flight(part, rw); - kref_put(&part->ref, __delete_partition); + hd_struct_put(part); part_stat_unlock(); } } diff --git a/block/blk-merge.c b/block/blk-merge.c index b06b83b89d896..00b7d31b38a2f 100644 --- a/block/blk-merge.c +++ b/block/blk-merge.c @@ -356,7 +356,7 @@ static void blk_account_io_merge(struct request *req) part_round_stats(cpu, part); part_dec_in_flight(part, rq_data_dir(req)); - kref_put(&part->ref, __delete_partition); + hd_struct_put(part); part_stat_unlock(); } } diff --git a/block/genhd.c b/block/genhd.c index 85c150598830b..399d37ec7412d 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1192,7 +1192,8 @@ struct gendisk *alloc_disk_node(int minors, int node_id) return NULL; } disk->part_tbl->part[0] = &disk->part0; - kref_init(&disk->part0.ref); + + hd_ref_init(&disk->part0); disk->minors = minors; rand_initialize_disk(disk); diff --git a/fs/partitions/check.c b/fs/partitions/check.c index 48209f58522bc..011520df71ae6 100644 --- a/fs/partitions/check.c +++ b/fs/partitions/check.c @@ -381,10 +381,8 @@ static void delete_partition_rcu_cb(struct rcu_head *head) put_device(part_to_dev(part)); } -void __delete_partition(struct kref *ref) +void __delete_partition(struct hd_struct *part) { - struct hd_struct *part = container_of(ref, struct hd_struct, ref); - call_rcu(&part->rcu_head, delete_partition_rcu_cb); } @@ -406,7 +404,7 @@ void delete_partition(struct gendisk *disk, int partno) kobject_put(part->holder_dir); device_del(part_to_dev(part)); - kref_put(&part->ref, __delete_partition); + hd_struct_put(part); } static ssize_t whole_disk_show(struct device *dev, @@ -505,7 +503,7 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno, if (!dev_get_uevent_suppress(ddev)) kobject_uevent(&pdev->kobj, KOBJ_ADD); - kref_init(&p->ref); + hd_ref_init(p); return p; out_free_info: diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 2ba2792a3dd4e..2d04681459672 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -115,8 +115,8 @@ struct hd_struct { #else struct disk_stats dkstats; #endif + atomic_t ref; struct rcu_head rcu_head; - struct kref ref; }; #define GENHD_FL_REMOVABLE 1 @@ -584,7 +584,7 @@ extern struct hd_struct * __must_check add_partition(struct gendisk *disk, sector_t len, int flags, struct partition_meta_info *info); -extern void __delete_partition(struct kref *ref); +extern void __delete_partition(struct hd_struct *); extern void delete_partition(struct gendisk *, int); extern void printk_all_partitions(void); @@ -613,6 +613,29 @@ extern ssize_t part_fail_store(struct device *dev, const char *buf, size_t count); #endif /* CONFIG_FAIL_MAKE_REQUEST */ +static inline void hd_ref_init(struct hd_struct *part) +{ + atomic_set(&part->ref, 1); + smp_mb(); +} + +static inline void hd_struct_get(struct hd_struct *part) +{ + atomic_inc(&part->ref); + smp_mb__after_atomic_inc(); +} + +static inline int hd_struct_try_get(struct hd_struct *part) +{ + return atomic_inc_not_zero(&part->ref); +} + +static inline void hd_struct_put(struct hd_struct *part) +{ + if (atomic_dec_and_test(&part->ref)) + __delete_partition(part); +} + #else /* CONFIG_BLOCK */ static inline void printk_all_partitions(void) { } diff --git a/include/linux/kref.h b/include/linux/kref.h index 90b9e44abf54a..6cc38fc07ab76 100644 --- a/include/linux/kref.h +++ b/include/linux/kref.h @@ -23,7 +23,6 @@ struct kref { void kref_init(struct kref *kref); void kref_get(struct kref *kref); -int kref_test_and_get(struct kref *kref); int kref_put(struct kref *kref, void (*release) (struct kref *kref)); #endif /* _KREF_H_ */ diff --git a/lib/kref.c b/lib/kref.c index e7a6e10671223..d3d227a08a4bd 100644 --- a/lib/kref.c +++ b/lib/kref.c @@ -36,18 +36,6 @@ void kref_get(struct kref *kref) smp_mb__after_atomic_inc(); } -/** - * kref_test_and_get - increment refcount for object only if refcount is not - * zero. - * @kref: object. - * - * Return non-zero if the refcount was incremented, 0 otherwise - */ -int kref_test_and_get(struct kref *kref) -{ - return atomic_inc_not_zero(&kref->refcount); -} - /** * kref_put - decrement refcount for object. * @kref: object. -- cgit v1.2.3