fix request_queue life-cycle management

blkio_policy_parse_and_set() calls blkio_check_dev_num() to check whether the given dev_t is valid. blkio_check_dev_num() uses get_gendisk() for verification but never puts the returned genhd leaking the reference.

This patch collapses blkio_check_dev_num() into its caller and updates it such that the genhd is put before returning.

What's the advantage of collapsing blkio_check_dev_num(). Why not put the reference to gendisk in this function before returning either success or failure.

Hello,

Quoted 3 lines · Vivek Goyal

What's the advantage of collapsing blkio_check_dev_num(). Why not put the reference to gendisk in this function before returning either success or failure.

Heh, at first, I just thought there would be something which depends on disk still being around in the code path as unsynchronized one time check upfront doesn't really guarantee anything; then, I realized there was nothing, but I still left it like that because I personally think blkio_check_dev_num() w/o surrounding exclusion is a bad interface. It's at best opportunistic and likely to mislead people into believing that there's some magical implied synchronization.

Also, I'm planning on cleaning up synchronization around iocg and for it to work properly, it'll be necessary to do proper ref counting and removal on release anyway.

Are you trying to tie the rules with actual presence of device. Current model is that we just check for a valid device while somebody is specifying the rule. After that device can go away and rule can still be there. We are just trying to make sure that when you are putting the rule, atleast at that time device is present.

Hmmm.... I don't know. If we're gonna bind rule existence to that of

device, wouldn't it be better to simply not check whether the device exists? The current behavior seems pretty confusing to me.

Quoted 1 line · Tejun Heo

Hmmm.... I don't know. If we're gonna bind rule existence to that of

Heh, I meant to say, "if those two are unbound,"

Quoted 2 lines

device, wouldn't it be better to simply not check whether the device exists? The current behavior seems pretty confusing to me.

Hello, again.

(cc'ing containers list and Kay)

The original thread is at

http://thread.gmane.org/gmane.linux.kernel/1205150/focus=1205160

and it's about retaining blkiocg rules across device destruction.

Quoted 7 lines · Tejun Heo

Quoted 1 line · Tejun Heo

Hmmm.... I don't know. If we're gonna bind rule existence to that of

Heh, I meant to say, "if those two are unbound,"

Quoted 2 lines

device, wouldn't it be better to simply not check whether the device exists? The current behavior seems pretty confusing to me.

I've been thinking about it and the more I think about it, the current behavior seems just wrong. Device node # doesn't mean anything before a device actually appears there. There is no way userland can know deterministically how the device node allocation would end up.

For example, sd allocates minors according to internal ida allocation which is freed on scsi_disk_release() - ie. when all *kernel* references go away which the userland has no way to find out until after new device comes up with a different devt.

For EXT_DEVT devices, this becomes even less meaingful. There is absolutely no guarantee what devno would mean what. devno which is currently assigned to the whole disk now can be reassigned to a partition. There absolutely is no rule regarding who gets what numbers. ie. This can end up with rules pointing to partitions.

Moreover, it doesn't even make the implementation simpler. blkiocg currently keeps a separate list of policies so that they don't disappear along with blkg's.

The only way applying rules to dynamic devices can work is doing the proper dynamic configuration off udev and friends.

Quoted 3 lines

Moreover, it doesn't even make the implementation simpler. blkiocg currently keeps a separate list of policies so that they don't disappear along with blkg's.

One reason for keeping rules in blkiocg is that blkg don't get created until and unless IO happens in that cgroup. Rules can be created much before that.

Quoted 2 lines

The only way applying rules to dynamic devices can work is doing the proper dynamic configuration off udev and friends.

Actually it is not exactly a feature at this point of time. It was just for the sake of simplicity that I let the rules be there even if device has gone away and yes it is indeep a shortcoming that if a different device shows up with old device's major and minor, then old rule will get applied to new device.

Having said that, removal of rule upon device removal also might not make much sense.

- Rules are tied to cgroups and not to devices as such. So until cgroup goes away a user might be surprised that a configured rule for a device suddenly disappeared.

- Though my examples are not exactly similar, but when a device goes away we don't try to unmount the filesystem automatically. We don't try to get rid of /etc/fstab entries and if somebody as put a /etc/fstab entry based on device name, then they might end up mounting wrong device.

So I don't feel strongly to tie rules and device life time together. Making use of udev and friends to automatically add/remove rules as devices show up or go will make sense though.

I think this is essentially a bug.  If you have something like "dev = find_and_get(); put(dev); return dev != NULL;", it's a pretty good indication something is pretty screwed there, so unless someone screams really loud, I think I'm gonna push for removal of the feature.

Quoted 5 lines · Tejun Heo

I think this is essentially a bug.  If you have something like "dev = find_and_get(); put(dev); return dev != NULL;", it's a pretty good indication something is pretty screwed there, so unless someone screams really loud, I think I'm gonna push for removal of the feature.

Matching on unpredictable dev_t inside the kernel or in userspace is a serious bug. We can't do anything like that ever on a modern system, these times are long over.

Yes, please get rid of this misguided logic, nobody should get the impression that this can ever work.

Hello,

Quoted 4 lines · Vivek Goyal

Quoted 3 lines

Moreover, it doesn't even make the implementation simpler. blkiocg currently keeps a separate list of policies so that they don't disappear along with blkg's.

Quoted 3 lines

One reason for keeping rules in blkiocg is that blkg don't get created until and unless IO happens in that cgroup. Rules can be created much before that.

Yeah, I'd like to change that. I suppose it's result of evolution but it's not like rule addition is hot path or currently doesn't lookup bdev anyway.

Quoted 10 lines

Quoted 3 lines

The only way applying rules to dynamic devices can work is doing the proper dynamic configuration off udev and friends.

Actually it is not exactly a feature at this point of time. It was just for the sake of simplicity that I let the rules be there even if device has gone away and yes it is indeep a shortcoming that if a different device shows up with old device's major and minor, then old rule will get applied to new device.

Quoted 6 lines

Having said that, removal of rule upon device removal also might not make much sense.

- Rules are tied to cgroups and not to devices as such. So until cgroup goes away a user might be surprised that a configured rule for a device suddenly disappeared.

Rules are tied to their group-device pair and removal of either part should remove the rule. I mean, you're looking up and rejecting creation of new rules if the device isn't there.

Quoted 4 lines

- Though my examples are not exactly similar, but when a device goes away we don't try to unmount the filesystem automatically. We don't try to get rid of /etc/fstab entries and if somebody as put a /etc/fstab entry based on device name, then they might end up mounting wrong device.

That is different because the device node the filesystem holds onto is decommissioned and essentially put into zombie state for residual reference draining. It is NEVER re-used for any other purpose. If we need that type of ref draining, sure, we can do it, but there is no reason to do that for rules at all.

Quoted 3 lines

So I don't feel strongly to tie rules and device life time together. Making use of udev and friends to automatically add/remove rules as devices show up or go will make sense though.

I think this is essentially a bug. If you have something like "dev = find_and_get(); put(dev); return dev != NULL;", it's a pretty good indication something is pretty screwed there, so unless someone screams really loud, I think I'm gonna push for removal of the feature.

blk_throtl interface is block internal and there's no reason to have them in linux/blkdev.h. Move them to block/blk.h.

This patch doesn't introduce any functional change.

Signed-off-by Tejun Heo <[email protected]>

Cc Vivek Goyal <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-throttle.c   |    1 +
 block/blk.h            |   15 ++++++++++++++-
 include/linux/blkdev.h |   14 --------------
 3 files changed, 15 insertions(+), 15 deletions(-)

block/blk-throttle.c +1 -0

index a19f58c..f3f495e 100644
@@ -10,6 +10,7 @@
 #include <linux/bio.h>
 #include <linux/blktrace_api.h>
 #include "blk-cgroup.h"
+#include "blk.h"
 
 /* Max dispatch from a group in 1 round */
 static int throtl_grp_quantum = 8;

block/blk.h +14 -1

index 20b900a..da247ba 100644
@@ -188,4 +188,17 @@ static inline int blk_do_io_stat(struct request *rq)
 	        (rq->cmd_flags & REQ_DISCARD));
 }
 
-#endif
+#ifdef CONFIG_BLK_DEV_THROTTLING
+extern int blk_throtl_bio(struct request_queue *q, struct bio **bio);
+extern int blk_throtl_init(struct request_queue *q);
+extern void blk_throtl_exit(struct request_queue *q);
+#else /* CONFIG_BLK_DEV_THROTTLING */
+static inline int blk_throtl_bio(struct request_queue *q, struct bio **bio)
+{
+	return 0;
+}
+static inline int blk_throtl_init(struct request_queue *q) { return 0; }
+static inline void blk_throtl_exit(struct request_queue *q) { }
+#endif /* CONFIG_BLK_DEV_THROTTLING */
+
+#endif /* BLK_INTERNAL_H */

include/linux/blkdev.h +0 -14

index bf65f06..d98d7d6 100644
@@ -1198,20 +1198,6 @@ static inline uint64_t rq_io_start_time_ns(struct request *req)
 }
 #endif
 
-#ifdef CONFIG_BLK_DEV_THROTTLING
-extern int blk_throtl_init(struct request_queue *q);
-extern void blk_throtl_exit(struct request_queue *q);
-extern int blk_throtl_bio(struct request_queue *q, struct bio **bio);
-#else /* CONFIG_BLK_DEV_THROTTLING */
-static inline int blk_throtl_bio(struct request_queue *q, struct bio **bio)
-{
-	return 0;
-}
-
-static inline int blk_throtl_init(struct request_queue *q) { return 0; }
-static inline int blk_throtl_exit(struct request_queue *q) { return 0; }
-#endif /* CONFIG_BLK_DEV_THROTTLING */
-
 #define MODULE_ALIAS_BLOCKDEV(major,minor) \
 	MODULE_ALIAS("block-major-" __stringify(major) "-" __stringify(minor))
 #define MODULE_ALIAS_BLOCKDEV_MAJOR(major) \

Signature · 1 line

1.7.3.1

blk_alloc_request() and freed_request() take different combinations of REQ_* @flags, @priv and @is_sync when @flags is superset of the latter two. Make them take @flags only. This cleans up the code a bit and will ease updating allocation related REQ_* flags.

This patch doesn't introduce any functional difference.

Signed-off-by Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)

block/blk-core.c +1 -1

index a995141..fafa669 100644
@@ -575,7 +575,7 @@ static inline void blk_free_request(struct request_queue *q, struct request *rq)
 }
 
 static struct request *
-blk_alloc_request(struct request_queue *q, int flags, int priv, gfp_t gfp_mask)
+blk_alloc_request(struct request_queue *q, unsigned int flags, gfp_t gfp_mask)
 {
 	struct request *rq = mempool_alloc(q->rq.rq_pool, gfp_mask);
 

Quoted 10 lines

static struct request *

block/blk-core.c +1 -1

-blk_alloc_request(struct request_queue *q, int flags, int priv, gfp_t gfp_mask)
+blk_alloc_request(struct request_queue *q, unsigned int flags, gfp_t gfp_mask)
 {
 	struct request *rq = mempool_alloc(q->rq.rq_pool, gfp_mask);

@@ -586,12 +586,10 @@ blk_alloc_request(struct request_queue *q, int flags, int priv, gfp_t gfp_mask)
 
 	rq->cmd_flags = flags | REQ_ALLOCED;
 

block/blk-core.c blk_alloc_request(struct request_queue *q, int flags, int priv, gfp_t gfp_mask) · line 594

Quoted 6 lines

block/blk-core.c +0 -6

-	if (priv) {
-		if (unlikely(elv_set_request(q, rq, gfp_mask))) {
-			mempool_free(rq, q->rq.rq_pool);
-			return NULL;
-		}
-		rq->cmd_flags |= REQ_ELVPRIV;

Don't we need to set REQ_ELVPRIV in rq->cmd_flags in this new code?

blk_get/put_queue() in scsi_cmd_ioctl() and throtl_get_tg() are completely bogus. The caller must have a reference to the queue on entry and taking an extra reference doesn't change anything.

For scsi_cmd_ioctl(), the only effect is that it ends up checking QUEUE_FLAG_DEAD on entry; however, this is bogus as queue can die right after blk_get_queue(). Dead queue should be and is handled in request issue path (it's somewhat broken now but that's a separate problem and doesn't affect this one much).

throtl_get_tg() incorrectly assumes that q is rcu freed. Also, it doesn't check return value of blk_get_queue(). If the queue is already dead, it ends up doing an extra put.

Drop them.

Signed-off-by Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Vivek Goyal <[email protected]>

---
 block/blk-throttle.c |    8 +-------
 block/scsi_ioctl.c   |    3 +--
 2 files changed, 2 insertions(+), 9 deletions(-)

block/blk-throttle.c +1 -7

index f3f495e..ecba5fc 100644
@@ -324,12 +324,8 @@ static struct throtl_grp * throtl_get_tg(struct throtl_data *td)
 	/*
 	 * Need to allocate a group. Allocation of group also needs allocation
 	 * of per cpu stats which in-turn takes a mutex() and can block. Hence
-	 * we need to drop rcu lock and queue_lock before we call alloc
-	 *
-	 * Take the request queue reference to make sure queue does not
-	 * go away once we return from allocation.
+	 * we need to drop rcu lock and queue_lock before we call alloc.
 	 */
-	blk_get_queue(q);
 	rcu_read_unlock();
 	spin_unlock_irq(q->queue_lock);
 

@@ -339,13 +335,11 @@ static struct throtl_grp * throtl_get_tg(struct throtl_data *td)
 	 * dead
 	 */
 	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags))) {
-		blk_put_queue(q);
 		if (tg)
 			kfree(tg);
 
 		return ERR_PTR(-ENODEV);
 	}
-	blk_put_queue(q);
 
 	/* Group allocated and queue is still alive. take the lock */
 	spin_lock_irq(q->queue_lock);

block/scsi_ioctl.c +1 -2

index 4f4230b..fbdf0d8 100644
@@ -565,7 +565,7 @@ int scsi_cmd_ioctl(struct request_queue *q, struct gendisk *bd_disk, fmode_t mod
 {
 	int err;
 
-	if (!q || blk_get_queue(q))
+	if (!q)
 		return -ENXIO;
 
 	switch (cmd) {

@@ -686,7 +686,6 @@ int scsi_cmd_ioctl(struct request_queue *q, struct gendisk *bd_disk, fmode_t mod
 			err = -ENOTTY;
 	}
 
-	blk_put_queue(q);
 	return err;
 }
 EXPORT_SYMBOL(scsi_cmd_ioctl);

Signature · 1 line

1.7.3.1

What makes sure that caller has the reference to the queue upon entry. If that's the case, that's good. Just that I had not figured it out so resorted to defensive programming as taking extra reference does not harm.

blk_throtl_bio() and throtl_get_tg() have rather unusual interface.

* throtl_get_tg() returns pointer to a valid tg or ERR_PTR(-ENODEV), and drops queue_lock in the latter case. Different locking context depending on return value is error-prone and DEAD state is scheduled to be protected by queue_lock anyway. Move DEAD check inside queue_lock and return valid tg or NULL.

* blk_throtl_bio() indicates return status both with its return value and in/out param **@bio. The former is used to indicate whether queue is found to be dead during throtl processing. The latter whether the bio is throttled.

There's no point in returning DEAD check result from blk_throtl_bio(). The queue can die after blk_throtl_bio() is finished but before make_request_fn() grabs queue lock.

A driver could call blk_cleanup_queue(), mark the queue DEAD and then free the driver provided spin lock. So once queue is DEAD one could not rely on queue lock still being there. That's the reason I did not try to take queue lock again if queue is marked DEAD.

Now I see the change that blk_cleanup_queue will start poiting to internal queue lock (Thought it is racy). This will atleast make sure that some spinlock is around. So now this change should be fine.

Quoted 8 lines

* blk_throtl_bio() indicates return status both with its return value and in/out param **@bio. The former is used to indicate whether queue is found to be dead during throtl processing. The latter whether the bio is throttled.

There's no point in returning DEAD check result from blk_throtl_bio(). The queue can die after blk_throtl_bio() is finished but before make_request_fn() grabs queue lock.

The reason I was returning error in case of queue DEAD is that I wanted IO to now return with error instead of continuing to call q->make_request_fn(q, bio) which does not do queue dead check and assumes queue is still alive.

With this change, if queue is DEAD, bio will not be throttled and we will continue to submit bio to queue and I am not sure who will catch it in __make_request()?

Hello,

Quoted 9 lines · Vivek Goyal

A driver could call blk_cleanup_queue(), mark the queue DEAD and then free the driver provided spin lock. So once queue is DEAD one could not rely on queue lock still being there. That's the reason I did not try to take queue lock again if queue is marked DEAD.

Now I see the change that blk_cleanup_queue will start poiting to internal queue lock (Thought it is racy). This will atleast make sure that some spinlock is around. So now this change should be fine.

The problem with the current code is that all those are not properly synchronized. Drivers shouldn't destroy lock or any other stuff until blk_cleanup_queue() is complete and once queue cleanup is done block layer shouldn't call out to driver.

Currently, the code has different opportunistic checks which can catch most of those cases but unfortunatly I think it just makes the bugs more obscure.

That said, we probably should be switching to internal lock once clenaup is complete.

Quoted 6 lines

Currently, the code has different opportunistic checks which can catch most of those cases but unfortunatly I think it just makes the bugs more obscure.

That said, we probably should be switching to internal lock once clenaup is complete.

So even switching to internal lock is racy. Christoph suggeted to break down this sharing of queue lock and driver lock and suggested always use internal queue lock and modify drivers to use their own lock and manage it. It makes sense though it might be lot of work to fix drivers.

Hello,

Quoted 11 lines · Vivek Goyal

Quoted 6 lines

Currently, the code has different opportunistic checks which can catch most of those cases but unfortunatly I think it just makes the bugs more obscure.

That said, we probably should be switching to internal lock once clenaup is complete.

So even switching to internal lock is racy. Christoph suggeted to break down this sharing of queue lock and driver lock and suggested always use internal queue lock and modify drivers to use their own lock and manage it. It makes sense though it might be lot of work to fix drivers.

Hmmmm, yeah, right, switching itself would be racy. Maybe not sharing is the solution, I don't know. There's a way to make the switching safe tho. Sth like the following.

	lock_queue(q, flags)
	{
		spinlock_t *lock;

		local_irq_save(flags);
		lock = rcu_dereference_sched(q->queue_lock);
		spin_lock(lock);
	}

and on cleanup, do synchronize_sched() after lock switching. But yeah it still stinks.

Quoted 13 lines

Hmmmm, yeah, right, switching itself would be racy. Maybe not sharing is the solution, I don't know. There's a way to make the switching safe tho. Sth like the following.

	lock_queue(q, flags)
	{
		spinlock_t *lock;

		local_irq_save(flags);
		lock = rcu_dereference_sched(q->queue_lock);
		spin_lock(lock);
	}

Quoted 2 lines

and on cleanup, do synchronize_sched() after lock switching. But yeah it still stinks.

rcu protected spinlocks. Interesting. :-)

IIUC, it still leaves the window open that two callers think they have the spinlock. One is holding the driver provided lock and other is holding the queue private lock.

Currently get_request[_wait]() allocates request whether queue is dead or not. This patch makes get_request[_wait]() return NULL if @q is dead. blk_queue_bio() is updated to fail the submitted bio if request allocation fails. While at it, add docbook comments for get_request[_wait]().

Note that the current code has rather unclear (there are spurious DEAD tests scattered around) assumption that the owner of a queue guarantees that no request travels block layer if the queue is dead and this patch in itself doesn't change much; however, this will allow fixing the broken assumption in the next patch.

Signed-off-by Tejun Heo <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c |   54 ++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 16 deletions(-)

block/blk-core.c +38 -16

index 7a90317..fc10f53 100644
@@ -710,10 +710,19 @@ static bool blk_rq_should_init_elevator(struct bio *bio)
 	return true;
 }
 
-/*
- * Get a free request, queue_lock must be held.
- * Returns NULL on failure, with queue_lock held.
- * Returns !NULL on success, with queue_lock *not held*.
+/**
+ * get_request - get a free request
+ * @q: request_queue to allocate request from
+ * @rw_flags: RW and SYNC flags
+ * @bio: bio to allocate request for (can be %NULL)
+ * @gfp_mask: allocation mask
+ *
+ * Get a free request from @q.  This function may fail under memory
+ * pressure or if @q is dead.
+ *
+ * Must be callled with @q->queue_lock held and,
+ * Returns %NULL on failure, with @q->queue_lock held.
+ * Returns !%NULL on success, with @q->queue_lock *not held*.
  */
 static struct request *get_request(struct request_queue *q, int rw_flags,
 				   struct bio *bio, gfp_t gfp_mask)

@@ -724,6 +733,9 @@ static struct request *get_request(struct request_queue *q, int rw_flags,
 	const bool is_sync = rw_is_sync(rw_flags) != 0;
 	int may_queue;
 
+	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+		return NULL;
+
 	may_queue = elv_may_queue(q, rw_flags);
 	if (may_queue == ELV_MQUEUE_NO)
 		goto rq_starved;

@@ -816,11 +828,18 @@ out:
 	return rq;
 }
 
-/*
- * No available requests for this queue, wait for some requests to become
- * available.
+/**
+ * get_request_wait - get a free request with retry
+ * @q: request_queue to allocate request from
+ * @rw_flags: RW and SYNC flags
+ * @bio: bio to allocate request for (can be %NULL)
  *
- * Called with q->queue_lock held, and returns with it unlocked.
+ * Get a free request from @q.  This function keeps retrying under memory
+ * pressure and fails iff @q is dead.
+ *
+ * Must be callled with @q->queue_lock held and,
+ * Returns %NULL on failure, with @q->queue_lock held.
+ * Returns !%NULL on success, with @q->queue_lock *not held*.
  */
 static struct request *get_request_wait(struct request_queue *q, int rw_flags,
 					struct bio *bio)

@@ -834,6 +853,9 @@ static struct request *get_request_wait(struct request_queue *q, int rw_flags,
 		struct io_context *ioc;
 		struct request_list *rl = &q->rq;
 
+		if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+			return NULL;
+
 		prepare_to_wait_exclusive(&rl->wait[is_sync], &wait,
 				TASK_UNINTERRUPTIBLE);
 

@@ -864,19 +886,15 @@ struct request *blk_get_request(struct request_queue *q, int rw, gfp_t gfp_mask)
 {
 	struct request *rq;
 
-	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
-		return NULL;
-
 	BUG_ON(rw != READ && rw != WRITE);
 
 	spin_lock_irq(q->queue_lock);
-	if (gfp_mask & __GFP_WAIT) {
+	if (gfp_mask & __GFP_WAIT)
 		rq = get_request_wait(q, rw, NULL);
-	} else {
+	else
 		rq = get_request(q, rw, NULL, gfp_mask);
-		if (!rq)
-			spin_unlock_irq(q->queue_lock);
-	}
+	if (!rq)
+		spin_unlock_irq(q->queue_lock);
 	/* q->queue_lock is unlocked at this point */
 
 	return rq;

@@ -1300,6 +1318,10 @@ get_rq:
 	 * Returns with the queue unlocked.
 	 */
 	req = get_request_wait(q, rw_flags, bio);
+	if (unlikely(!req)) {
+		bio_endio(bio, -ENODEV);	/* @q is dead */
+		goto out_unlock;
+	}
 
 	/*
 	 * After dropping the lock and possibly sleeping here, our request

Signature · 1 line

1.7.3.1

request_queue is refcounted but actually depdends on lifetime management from the queue owner - on blk_cleanup_queue(), block layer expects that there's no request passing through request_queue and no new one will.

This is fundamentally broken. The queue owner (e.g. SCSI layer) doesn't have a way to know whether there are other active users before calling blk_cleanup_queue() and other users (e.g. bsg) don't have any guarantee that the queue is and would stay valid while it's holding a reference.

With delay added in blk_queue_bio() before queue_lock is grabbed, the following oops can be easily triggered when a device is removed with in-flight IOs.

sd 0:0:1:0: [sdb] Stopping disk
ata1.01: disabled

 general protection fault: 0000 [#1] PREEMPT SMP
 CPU 2
 Modules linked in:

Pid: 648, comm: test_rawio Not tainted 3.1.0-rc3-work+ #56 Bochs Bochs

 RIP: 0010:[<ffffffff8137d651>]  [<ffffffff8137d651>] elv_rqhash_find+0x61/0x100
 ...
 Process test_rawio (pid: 648, threadinfo ffff880019efa000, task ffff880019ef8a80)
 ...
 Call Trace:
  [<ffffffff8137d774>] elv_merge+0x84/0xe0
  [<ffffffff81385b54>] blk_queue_bio+0xf4/0x400
  [<ffffffff813838ea>] generic_make_request+0xca/0x100
  [<ffffffff81383994>] submit_bio+0x74/0x100
  [<ffffffff811c53ec>] dio_bio_submit+0xbc/0xc0
  [<ffffffff811c610e>] __blockdev_direct_IO+0x92e/0xb40
  [<ffffffff811c39f7>] blkdev_direct_IO+0x57/0x60
  [<ffffffff8113b1c5>] generic_file_aio_read+0x6d5/0x760
  [<ffffffff8118c1ca>] do_sync_read+0xda/0x120
  [<ffffffff8118ce55>] vfs_read+0xc5/0x180
  [<ffffffff8118cfaa>] sys_pread64+0x9a/0xb0
  [<ffffffff81afaf6b>] system_call_fastpath+0x16/0x1b

This happens because blk_queue_cleanup() destroys the queue and elevator whether IOs are in progress or not and DEAD tests are sprinkled in the request processing path without proper synchronization.

Similar problem exists for blk-throtl. On queue cleanup, blk-throtl is shutdown whether it has requests in it or not. Depending on timing, it either oopses or throttled bios are lost putting tasks which are waiting for bio completion into eternal D state.

The way it should work is having the usual clear distinction between shutdown and release. Shutdown drains all currently pending requests, marks the queue dead, and performs partial teardown of the now unnecessary part of the queue. Even after shutdown is complete, reference holders are still allowed to issue requests to the queue although they will be immmediately failed. The rest of teardown happens on release.

This patch makes the following changes to make blk_queue_cleanup() behave as proper shutdown.

* QUEUE_FLAG_DEAD is now set while holding both q->exit_mutex and queue_lock.

* Unsynchronized DEAD check in generic_make_request_checks() removed. This couldn't make any meaningful difference as the queue could die after the check.

* blk_drain_queue() updated such that it can drain all requests and is now called during cleanup.

* blk_throtl updated such that it checks DEAD on grabbing queue_lock, drains all throttled bios during cleanup and free td when queue is released.

This patch clashes a bit with the previous "fix" for getting rid of privately assigned queue locks. I've kept that single bit.

Quoted 2 lines · Jens Axboe

This patch clashes a bit with the previous "fix" for getting rid of privately assigned queue locks. I've kept that single bit.

Ah, okay, that's the private queue_lock thing Vivek mentioned in the earlier reply. Nice that it's already fixed.

void blk_cleanup_queue(struct request_queue *q)
{

block/blk-core.c +8 -10

-	/*
-	 * We know we have process context here, so we can be a little
-	 * cautious and ensure that pending block actions on this device
-	 * are done before moving on. Going into this function, we should
-	 * not have processes doing IO to this device.
-	 */
-	blk_sync_queue(q);
-
-	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);
+	/* mark @q DEAD, no new request or merges will be allowed afterwards */
 	mutex_lock(&q->sysfs_lock);
-	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
+	spin_lock_irq(q->queue_lock);
+	queue_flag_set(QUEUE_FLAG_NOMERGES, q);
+	queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
+	queue_flag_set(QUEUE_FLAG_DEAD, q);
+	spin_unlock_irq(q->queue_lock);
 	mutex_unlock(&q->sysfs_lock);
 
+	/* drain all requests queued before DEAD marking */
+	blk_drain_queue(q, true);

Quoted 8 lines

block/blk-throttle.c +0 -0

index 900a0c9..8edb949 100644
@@ -309,6 +309,10 @@ static struct throtl_grp * throtl_get_tg(struct throtl_data *td)
 	struct blkio_cgroup *blkcg;
 	struct request_queue *q = td->queue;
 

block/blk-throttle.c static struct throtl_grp * throtl_get_tg(struct throtl_data *td) · line 315

Quoted 4 lines

block/blk-throttle.c +4 -0

+	/* no throttling for dead queue */
+	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+		return NULL;
+

May be blk_throtl_bio() is probably a better place to do this check, just before callig throtl_get_tg().

Hello,

Quoted 8 lines · Vivek Goyal

Quoted 8 lines

block/blk-throttle.c +0 -0

index 900a0c9..8edb949 100644
@@ -309,6 +309,10 @@ static struct throtl_grp * throtl_get_tg(struct throtl_data *td)
 	struct blkio_cgroup *blkcg;
 	struct request_queue *q = td->queue;
 

Quoted 7 lines

block/blk-throttle.c static struct throtl_grp * throtl_get_tg(struct throtl_data *td) · line 315

Quoted 4 lines

block/blk-throttle.c +4 -0

+	/* no throttling for dead queue */
+	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+		return NULL;
+

May be blk_throtl_bio() is probably a better place to do this check, just before callig throtl_get_tg().

Hmmm... the thing is you need to check DEAD again once after releasing and re-grabbing the queuelock, os if we move the test into the caller, we end up having one in the caller and one in the callee, so I thought it would be better to keep them in the same function.

My opinion on
it isn't too strong tho. If you feel strong about it, please feel free to move it.

Quoted 4 lines

Hmmm... the thing is you need to check DEAD again once after releasing and re-grabbing the queuelock, os if we move the test into the caller, we end up having one in the caller and one in the callee, so I thought it would be better to keep them in the same function.

Sorry I did not get that why do we need to check again for DEAD after realeasing the queue lock. From throttling perspective, we grab lock once, if queue is DEAD, release lock and just return.

blk_throtl_bio() {

bunch_of_checks_without_queue_lock;

spin_lock()

  if (queue_dead) {
	spin_unlock()
	return;

}

throtl_get_tg();
}

This patchset is on top of the block/for-next cef1e172c7 "Merge branch 'for-3.2/mtip32xx' into for-next" and available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git block-ref