More device removal fixes

---
 block/blk-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

block/blk-core.c +1 -1

index a33870b..ef2e045 100644
@@ -362,7 +362,7 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
 	int i;
 
 	while (true) {
-		bool drain = false;
+		unsigned drain = 0;
 
 		spin_lock_irq(q->queue_lock);

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2012-10-26 12:02 UTC #5 lore raw

PATCH v5 2/7 block: Let blk_drain_queue() caller obtain the queue lock

Let the caller of blk_drain_queue() obtain the queue lock to improve readability of the patch called "Avoid that request_fn is invoked on a dead queue".

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Chanho Min <[email protected]>

---
 block/blk-core.c |   30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

block/blk-core.c +18 ~~-12~~

index ef2e045..76aff1d 100644
@@ -349,7 +349,7 @@ void blk_put_queue(struct request_queue *q)
 EXPORT_SYMBOL(blk_put_queue);
 
 /**
- * blk_drain_queue - drain requests from request_queue
+ * __blk_drain_queue - drain requests from request_queue
  * @q: queue to drain
  * @drain_all: whether to drain all requests or only the ones w/ ELVPRIV
  *

@@ -357,15 +357,17 @@ EXPORT_SYMBOL(blk_put_queue);
  * If not, only ELVPRIV requests are drained.  The caller is responsible
  * for ensuring that no new requests which need to be drained are queued.
  */
-void blk_drain_queue(struct request_queue *q, bool drain_all)
+static void __blk_drain_queue(struct request_queue *q, bool drain_all)
+	__releases(q->queue_lock)
+	__acquires(q->queue_lock)
 {
 	int i;
 
+	lockdep_assert_held(q->queue_lock);
+
 	while (true) {
 		unsigned drain = 0;
 
-		spin_lock_irq(q->queue_lock);
-
 		/*
 		 * The caller might be trying to drain @q before its
 		 * elevator is initialized.

@@ -401,11 +403,14 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
 			}
 		}
 
-		spin_unlock_irq(q->queue_lock);
-
 		if (!drain)
 			break;
+
+		spin_unlock_irq(q->queue_lock);
+
 		msleep(10);
+
+		spin_lock_irq(q->queue_lock);
 	}
 
 	/*

@@ -416,13 +421,9 @@ void blk_drain_queue(struct request_queue *q, bool drain_all)
 	if (q->request_fn) {
 		struct request_list *rl;
 
-		spin_lock_irq(q->queue_lock);
-
 		blk_queue_for_each_rl(rl, q)
 			for (i = 0; i < ARRAY_SIZE(rl->wait); i++)
 				wake_up_all(&rl->wait[i]);
-
-		spin_unlock_irq(q->queue_lock);
 	}
 }

@@ -446,7 +447,10 @@ void blk_queue_bypass_start(struct request_queue *q)
 	spin_unlock_irq(q->queue_lock);
 
 	if (drain) {
-		blk_drain_queue(q, false);
+		spin_lock_irq(q->queue_lock);
+		__blk_drain_queue(q, false);
+		spin_unlock_irq(q->queue_lock);
+
 		/* ensure blk_queue_bypass() is %true inside RCU read lock */
 		synchronize_rcu();
 	}

@@ -504,7 +508,9 @@ void blk_cleanup_queue(struct request_queue *q)
 	mutex_unlock(&q->sysfs_lock);
 
 	/* drain all requests queued before DEAD marking */
-	blk_drain_queue(q, true);
+	spin_lock_irq(lock);
+	__blk_drain_queue(q, true);
+	spin_unlock_irq(lock);
 
 	/* @q won't process any more request, flush async actions */
 	del_timer_sync(&q->backing_dev_info.laptop_mode_wb_timer);

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2012-10-26 12:02 UTC #7 lore raw

PATCH v5 3/7 block: Rename queue dead flag

QUEUE_FLAG_DEAD is used to indicate that queuing new requests must stop. After this flag has been set queue draining starts. However, during the queue draining phase it is still safe to invoke the queue's request_fn, so QUEUE_FLAG_DYING is a better name for this flag.

This patch has been generated by running the following command over the kernel source tree:

git grep -lEw 'blk_queue_dead|QUEUE_FLAG_DEAD' |

    xargs sed -i.tmp -e 's/blk_queue_dead/blk_queue_dying/g'  \
        -e 's/QUEUE_FLAG_DEAD/QUEUE_FLAG_DYING/g';            \
sed -i.tmp -e "s/QUEUE_FLAG_DYING$(printf \\t)*5/QUEUE_FLAG_DYING$(printf \\t)5/g" \
    include/linux/blkdev.h;                                   \

sed -i.tmp -e 's/ DEAD/ DYING/g' block/blk-core.c

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Chanho Min <[email protected]>

---
 block/blk-cgroup.c      |    2 +-
 block/blk-core.c        |   22 +++++++++++-----------
 block/blk-exec.c        |    2 +-
 block/blk-sysfs.c       |    4 ++--
 block/blk-throttle.c    |    2 +-
 block/blk.h             |    2 +-
 drivers/scsi/scsi_lib.c |    2 +-
 include/linux/blkdev.h  |    4 ++--
 8 files changed, 20 insertions(+), 20 deletions(-)

block/blk-cgroup.c +1 -1

index cafcd74..396c319 100644
@@ -231,7 +231,7 @@ struct blkcg_gq *blkg_lookup_create(struct blkcg *blkcg,
 	 * we shouldn't allow anything to go through for a bypassing queue.
 	 */
 	if (unlikely(blk_queue_bypass(q)))
-		return ERR_PTR(blk_queue_dead(q) ? -EINVAL : -EBUSY);
+		return ERR_PTR(blk_queue_dying(q) ? -EINVAL : -EBUSY);
 	return __blkg_lookup_create(blkcg, q, NULL);
 }
 EXPORT_SYMBOL_GPL(blkg_lookup_create);

block/blk-core.c +11 ~~-11~~

index 76aff1d..986e38e 100644
@@ -477,16 +477,16 @@ EXPORT_SYMBOL_GPL(blk_queue_bypass_end);
  * blk_cleanup_queue - shutdown a request queue
  * @q: request queue to shutdown
  *
- * Mark @q DEAD, drain all pending requests, destroy and put it.  All
+ * Mark @q DYING, drain all pending requests, destroy and put it.  All
  * future requests will be failed immediately with -ENODEV.
  */
 void blk_cleanup_queue(struct request_queue *q)
 {
 	spinlock_t *lock = q->queue_lock;
 
-	/* mark @q DEAD, no new request or merges will be allowed afterwards */
+	/* mark @q DYING, no new request or merges will be allowed afterwards */
 	mutex_lock(&q->sysfs_lock);
-	queue_flag_set_unlocked(QUEUE_FLAG_DEAD, q);
+	queue_flag_set_unlocked(QUEUE_FLAG_DYING, q);
 	spin_lock_irq(lock);
 
 	/*

@@ -503,11 +503,11 @@ void blk_cleanup_queue(struct request_queue *q)
 
 	queue_flag_set(QUEUE_FLAG_NOMERGES, q);
 	queue_flag_set(QUEUE_FLAG_NOXMERGES, q);
-	queue_flag_set(QUEUE_FLAG_DEAD, q);
+	queue_flag_set(QUEUE_FLAG_DYING, q);
 	spin_unlock_irq(lock);
 	mutex_unlock(&q->sysfs_lock);
 
-	/* drain all requests queued before DEAD marking */
+	/* drain all requests queued before DYING marking */
 	spin_lock_irq(lock);
 	__blk_drain_queue(q, true);
 	spin_unlock_irq(lock);

@@ -722,7 +722,7 @@ EXPORT_SYMBOL(blk_init_allocated_queue);
 
 bool blk_get_queue(struct request_queue *q)
 {
-	if (likely(!blk_queue_dead(q))) {
+	if (likely(!blk_queue_dying(q))) {
 		__blk_get_queue(q);
 		return true;
 	}

@@ -876,7 +876,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags,
 	const bool is_sync = rw_is_sync(rw_flags) != 0;
 	int may_queue;
 
-	if (unlikely(blk_queue_dead(q)))
+	if (unlikely(blk_queue_dying(q)))
 		return NULL;
 
 	may_queue = elv_may_queue(q, rw_flags);

@@ -1056,7 +1056,7 @@ retry:
 	if (rq)
 		return rq;
 
-	if (!(gfp_mask & __GFP_WAIT) || unlikely(blk_queue_dead(q))) {
+	if (!(gfp_mask & __GFP_WAIT) || unlikely(blk_queue_dying(q))) {
 		blk_put_rl(rl);
 		return NULL;
 	}

@@ -1916,7 +1916,7 @@ int blk_insert_cloned_request(struct request_queue *q, struct request *rq)
 		return -EIO;
 
 	spin_lock_irqsave(q->queue_lock, flags);
-	if (unlikely(blk_queue_dead(q))) {
+	if (unlikely(blk_queue_dying(q))) {
 		spin_unlock_irqrestore(q->queue_lock, flags);
 		return -ENODEV;
 	}

@@ -2892,7 +2892,7 @@ static void queue_unplugged(struct request_queue *q, unsigned int depth,
 	/*
 	 * Don't mess with dead queue.
 	 */
-	if (unlikely(blk_queue_dead(q))) {
+	if (unlikely(blk_queue_dying(q))) {
 		spin_unlock(q->queue_lock);
 		return;
 	}

@@ -3001,7 +3001,7 @@ void blk_flush_plug_list(struct blk_plug *plug, bool from_schedule)
 		/*
 		 * Short-circuit if @q is dead
 		 */
-		if (unlikely(blk_queue_dead(q))) {
+		if (unlikely(blk_queue_dying(q))) {
 			__blk_end_request_all(rq, -ENODEV);
 			continue;
 		}

block/blk-exec.c +1 -1

index 8b6dc5b..4aec98d 100644
@@ -60,7 +60,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 
 	spin_lock_irq(q->queue_lock);
 
-	if (unlikely(blk_queue_dead(q))) {
+	if (unlikely(blk_queue_dying(q))) {
 		rq->errors = -ENXIO;
 		if (rq->end_io)
 			rq->end_io(rq, rq->errors);

block/blk-sysfs.c +2 -2

index ce62046..7881477 100644
@@ -466,7 +466,7 @@ queue_attr_show(struct kobject *kobj, struct attribute *attr, char *page)
 	if (!entry->show)
 		return -EIO;
 	mutex_lock(&q->sysfs_lock);
-	if (blk_queue_dead(q)) {
+	if (blk_queue_dying(q)) {
 		mutex_unlock(&q->sysfs_lock);
 		return -ENOENT;
 	}

@@ -488,7 +488,7 @@ queue_attr_store(struct kobject *kobj, struct attribute *attr,
 
 	q = container_of(kobj, struct request_queue, kobj);
 	mutex_lock(&q->sysfs_lock);
-	if (blk_queue_dead(q)) {
+	if (blk_queue_dying(q)) {
 		mutex_unlock(&q->sysfs_lock);
 		return -ENOENT;
 	}

block/blk-throttle.c +1 -1

index a9664fa..3114622 100644
@@ -302,7 +302,7 @@ static struct throtl_grp *throtl_lookup_create_tg(struct throtl_data *td,
 		/* if %NULL and @q is alive, fall back to root_tg */
 		if (!IS_ERR(blkg))
 			tg = blkg_to_tg(blkg);
-		else if (!blk_queue_dead(q))
+		else if (!blk_queue_dying(q))
 			tg = td_root_tg(td);
 	}

block/blk.h +1 -1

index ca51543..2218a8a 100644
@@ -96,7 +96,7 @@ static inline struct request *__elv_next_request(struct request_queue *q)
 			q->flush_queue_delayed = 1;
 			return NULL;
 		}
-		if (unlikely(blk_queue_dead(q)) ||
+		if (unlikely(blk_queue_dying(q)) ||
 		    !q->elevator->type->ops.elevator_dispatch_fn(q, 0))
 			return NULL;
 	}

drivers/scsi/scsi_lib.c +1 -1

index da36a3a..f29a1a9 100644
@@ -1406,7 +1406,7 @@ static int scsi_lld_busy(struct request_queue *q)
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost;
 
-	if (blk_queue_dead(q))
+	if (blk_queue_dying(q))
 		return 0;
 
 	shost = sdev->host;

include/linux/blkdev.h +2 -2

index 1756001..aba8246 100644
@@ -437,7 +437,7 @@ struct request_queue {
 #define QUEUE_FLAG_STOPPED	2	/* queue is stopped */
 #define	QUEUE_FLAG_SYNCFULL	3	/* read queue has been filled */
 #define QUEUE_FLAG_ASYNCFULL	4	/* write queue has been filled */
-#define QUEUE_FLAG_DEAD		5	/* queue being torn down */
+#define QUEUE_FLAG_DYING	5	/* queue being torn down */
 #define QUEUE_FLAG_BYPASS	6	/* act as dumb FIFO queue */
 #define QUEUE_FLAG_BIDI		7	/* queue supports bidi requests */
 #define QUEUE_FLAG_NOMERGES     8	/* disable merge attempts */

@@ -521,7 +521,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 
 #define blk_queue_tagged(q)	test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)
 #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
-#define blk_queue_dead(q)	test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)
+#define blk_queue_dying(q)	test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)
 #define blk_queue_bypass(q)	test_bit(QUEUE_FLAG_BYPASS, &(q)->queue_flags)
 #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)
 #define blk_queue_noxmerges(q)	\

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2012-10-26 12:03 UTC #8 lore raw

PATCH v5 4/7 block: Avoid that request_fn is invoked on a dead queue

A block driver may start cleaning up resources needed by its request_fn as soon as blk_cleanup_queue() finished, so request_fn must not be invoked after draining finished.

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Chanho Min <[email protected]>

---
 block/blk-core.c       |   26 +++++++++++++++++++++++---
 block/blk-exec.c       |    2 +-
 block/blk.h            |    2 ++
 include/linux/blkdev.h |    2 ++
 4 files changed, 28 insertions(+), 4 deletions(-)

block/blk-core.c +23 -3

index 986e38e..2f1be7a 100644
@@ -293,6 +293,25 @@ void blk_sync_queue(struct request_queue *q)
 EXPORT_SYMBOL(blk_sync_queue);
 
 /**
+ * __blk_run_queue_uncond - run a queue whether or not it has been stopped
+ * @q:	The queue to run
+ *
+ * Description:
+ *    Invoke request handling on a queue if there are any pending requests.
+ *    May be used to restart request handling after a request has completed.
+ *    This variant runs the queue whether or not the queue has been
+ *    stopped. Must be called with the queue lock held and interrupts
+ *    disabled. See also @blk_run_queue.
+ */
+void inline __blk_run_queue_uncond(struct request_queue *q)
+{
+	if (unlikely(blk_queue_dead(q)))
+		return;
+
+	q->request_fn(q);
+}
+
+/**
  * __blk_run_queue - run a single device queue
  * @q:	The queue to run
  *

@@ -305,7 +324,7 @@ void __blk_run_queue(struct request_queue *q)
 	if (unlikely(blk_queue_stopped(q)))
 		return;
 
-	q->request_fn(q);
+	__blk_run_queue_uncond(q);
 }
 EXPORT_SYMBOL(__blk_run_queue);

@@ -477,8 +496,8 @@ EXPORT_SYMBOL_GPL(blk_queue_bypass_end);
  * blk_cleanup_queue - shutdown a request queue
  * @q: request queue to shutdown
  *
- * Mark @q DYING, drain all pending requests, destroy and put it.  All
- * future requests will be failed immediately with -ENODEV.
+ * Mark @q DYING, drain all pending requests, mark @q DEAD, destroy and
+ * put it.  All future requests will be failed immediately with -ENODEV.
  */
 void blk_cleanup_queue(struct request_queue *q)
 {

@@ -510,6 +529,7 @@ void blk_cleanup_queue(struct request_queue *q)
 	/* drain all requests queued before DYING marking */
 	spin_lock_irq(lock);
 	__blk_drain_queue(q, true);
+	queue_flag_set(QUEUE_FLAG_DEAD, q);
 	spin_unlock_irq(lock);
 
 	/* @q won't process any more request, flush async actions */

block/blk-exec.c +1 -1

index 4aec98d..1320e74 100644
@@ -72,7 +72,7 @@ void blk_execute_rq_nowait(struct request_queue *q, struct gendisk *bd_disk,
 	__blk_run_queue(q);
 	/* the queue is stopped so it won't be run */
 	if (rq->cmd_type == REQ_TYPE_PM_RESUME)
-		q->request_fn(q);
+		__blk_run_queue_uncond(q);
 	spin_unlock_irq(q->queue_lock);
 }
 EXPORT_SYMBOL_GPL(blk_execute_rq_nowait);

block/blk.h +2 -0

index 2218a8a..47fdfdd 100644
@@ -145,6 +145,8 @@ int blk_try_merge(struct request *rq, struct bio *bio);
 
 void blk_queue_congestion_threshold(struct request_queue *q);
 
+void __blk_run_queue_uncond(struct request_queue *q);
+
 int blk_dev_init(void);

include/linux/blkdev.h +2 -0

index aba8246..8bc46c2 100644
@@ -452,6 +452,7 @@ struct request_queue {
 #define QUEUE_FLAG_ADD_RANDOM  16	/* Contributes to random pool */
 #define QUEUE_FLAG_SECDISCARD  17	/* supports SECDISCARD */
 #define QUEUE_FLAG_SAME_FORCE  18	/* force complete on same CPU */
+#define QUEUE_FLAG_DEAD        19	/* queue tear-down finished */
 
 #define QUEUE_FLAG_DEFAULT	((1 << QUEUE_FLAG_IO_STAT) |		\
 				 (1 << QUEUE_FLAG_STACKABLE)	|	\

@@ -522,6 +523,7 @@ static inline void queue_flag_clear(unsigned int flag, struct request_queue *q)
 #define blk_queue_tagged(q)	test_bit(QUEUE_FLAG_QUEUED, &(q)->queue_flags)
 #define blk_queue_stopped(q)	test_bit(QUEUE_FLAG_STOPPED, &(q)->queue_flags)
 #define blk_queue_dying(q)	test_bit(QUEUE_FLAG_DYING, &(q)->queue_flags)
+#define blk_queue_dead(q)	test_bit(QUEUE_FLAG_DEAD, &(q)->queue_flags)
 #define blk_queue_bypass(q)	test_bit(QUEUE_FLAG_BYPASS, &(q)->queue_flags)
 #define blk_queue_nomerges(q)	test_bit(QUEUE_FLAG_NOMERGES, &(q)->queue_flags)
 #define blk_queue_noxmerges(q)	\

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2012-10-26 12:04 UTC #10 lore raw

PATCH v5 5/7 block: Make blk_cleanup_queue() wait until request_fn finished

Some request_fn implementations, e.g. scsi_request_fn(), unlock the queue lock. Make sure that blk_cleanup_queue() waits until all active request_fn invocations have finished. A block driver may start cleaning up resources needed by its request_fn as soon as blk_cleanup_queue() finished, so blk_cleanup_queue() must wait for all outstanding request_fn invocations to finish.

Reported-by Chanho Min <[email protected]>

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

---
 block/blk-core.c       |    3 +++
 include/linux/blkdev.h |    6 ++++++
 2 files changed, 9 insertions(+)

block/blk-core.c +3 -0

index 2f1be7a..7662d83 100644
@@ -308,7 +308,9 @@ void inline __blk_run_queue_uncond(struct request_queue *q)
 	if (unlikely(blk_queue_dead(q)))
 		return;
 
+	q->request_fn_active++;
 	q->request_fn(q);
+	q->request_fn_active--;
 }
 
 /**

@@ -407,6 +409,7 @@ static void __blk_drain_queue(struct request_queue *q, bool drain_all)
 			__blk_run_queue(q);
 
 		drain |= q->nr_rqs_elvpriv;
+		drain |= q->request_fn_active;
 
 		/*
 		 * Unfortunately, requests are queued at and tracked from

include/linux/blkdev.h +6 -0

index 8bc46c2..c9d233e 100644
@@ -378,6 +378,12 @@ struct request_queue {
 
 	unsigned int		nr_sorted;
 	unsigned int		in_flight[2];
+	/*
+	 * Number of active block driver functions for which blk_drain_queue()
+	 * must wait. Must be incremented around functions that unlock the
+	 * queue_lock internally, e.g. scsi_request_fn().
+	 */
+	unsigned int		request_fn_active;
 
 	unsigned int		rq_timeout;
 	struct timer_list	timeout;

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2012-10-26 12:05 UTC #12 lore raw

PATCH v5 6/7 Fix race between starved list processing and device removal

The SCSI core maintains a "starved list" per SCSI host. This is a list of devices for which one or more requests have been queued but that have not yet been passed to the SCSI LLD. The function scsi_run_queue() examines all SCSI devices on the starved list. Since scsi_remove_device() can be invoked concurrently with scsi_run_queue() it is important to avoid that a SCSI device is accessed by that function after it has been freed. Avoid that the sdev reference count can drop to zero before the queue is run by scsi_run_queue() by inserting a get_device() / put_device() pair in that function. Move the code for removing a device from the starved list from scsi_device_dev_release_usercontext() to __scsi_remove_device() such that it is guaranteed that the newly added get_device() call succeeds.

Reported-and-tested-by Chanho Min <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Reviewed-by Mike Christie <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    5 +++++
 drivers/scsi/scsi_sysfs.c |    7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

drivers/scsi/scsi_lib.c +5 -0

index f29a1a9..c5d4ec2 100644
@@ -452,10 +452,15 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
+		get_device(&sdev->sdev_gendev);
 		spin_unlock(shost->host_lock);
+
 		spin_lock(sdev->request_queue->queue_lock);
 		__blk_run_queue(sdev->request_queue);
 		spin_unlock(sdev->request_queue->queue_lock);
+
+		put_device(&sdev->sdev_gendev);
+
 		spin_lock(shost->host_lock);
 	}
 	/* put any unprocessed entries back */

drivers/scsi/scsi_sysfs.c +6 -1

index ce5224c..2661a957 100644
@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);

@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

@@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);
 
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	if (sdev->host->hostt->slave_destroy)
 		sdev->host->hostt->slave_destroy(sdev);
 	transport_destroy_device(dev);

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2012-10-26 12:05 UTC #21 lore raw

PATCH v5 7/7 Remove get_device() / put_device() pair from scsi_request_fn()

Since all scsi_request_fn() callers hold a reference on the SCSI device that function is invoked for and since blk_cleanup_queue() waits until scsi_request_fn() has finished it is safe to remove the get_device() / put_device() pair from scsi_request_fn().

Cc James Bottomley <[email protected]>

Cc Mike Christie <[email protected]>

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

---
 drivers/scsi/scsi_lib.c |   12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

drivers/scsi/scsi_lib.c +3 -9

index c5d4ec2..71bddec 100644
@@ -1516,16 +1516,14 @@ static void scsi_softirq_done(struct request *rq)
  * Lock status: IO request lock assumed to be held when called.
  */
 static void scsi_request_fn(struct request_queue *q)
+	__releases(q->queue_lock)
+	__acquires(q->queue_lock)
 {
 	struct scsi_device *sdev = q->queuedata;
 	struct Scsi_Host *shost;
 	struct scsi_cmnd *cmd;
 	struct request *req;
 
-	if(!get_device(&sdev->sdev_gendev))
-		/* We must be tearing the block queue down already */
-		return;
-
 	/*
 	 * To start with, we keep looping until the queue is empty, or until
 	 * the host is no longer able to accept any more requests.

@@ -1634,11 +1632,7 @@ out_delay:
 	if (sdev->device_busy == 0)
 		blk_delay_queue(q, SCSI_QUEUE_DELAY);
 out:
-	/* must be careful here...if we trigger the ->remove() function
-	 * we cannot be holding the q lock */
-	spin_unlock_irq(q->queue_lock);
-	put_device(&sdev->sdev_gendev);
-	spin_lock_irq(q->queue_lock);
+	;
 }
 
 u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)

Signature · 1 line

1.7.10.4

Zhuang, Jin Can

Reviewed-by Mike Christie on v5 6/7

2012-10-28 18:02 UTC #13 lore raw

RE: [PATCH 6/7] Fix race between starved list processing and device removal

I recently ran into the same issue
The test I did is plug/unplug u-disk in an interval of 1 second. And I found when sdev1 is being removed, scsi_run_queue is triggered by sdev2, which then accesses all the starving scsi device including sdev1.

I have adopted the solution below which works fine for me so far.
But there's one thing to fix in the patch below. When it put_device in scsi_run_queue, irq is disabled. As put_device may get into sleep, irq should be enabled before it's called. So I change it to:

		spin_unlock_irq(sdev->request_queue->queue_lock);

		put_device(&sdev->sdev_gendev);

spin_lock_irq(shost->host_lock);

-Jincan

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Bart Van Assche
Sent: Friday, October 26, 2012 8:05 PM
To: linux-scsi
Cc: James Bottomley; Mike Christie; Jens Axboe; Tejun Heo; Chanho Min
Subject: [PATCH 6/7] Fix race between starved list processing and device removal

The SCSI core maintains a "starved list" per SCSI host. This is a list of devices for which one or more requests have been queued but that have not yet been passed to the SCSI LLD. The function scsi_run_queue() examines all SCSI devices on the starved list.
Since scsi_remove_device() can be invoked concurrently with
scsi_run_queue() it is important to avoid that a SCSI device is accessed by that function after it has been freed. Avoid that the sdev reference count can drop to zero before the queue is run by scsi_run_queue() by inserting a get_device() / put_device() pair in that function. Move the code for removing a device from the starved list from scsi_device_dev_release_usercontext() to __scsi_remove_device() such that it is guaranteed that the newly added get_device() call succeeds.

Reported-and-tested-by Chanho Min <[email protected]>

Reference: http://lkml.org/lkml/2012/8/2/96

Cc Jens Axboe <[email protected]>

Cc Tejun Heo <[email protected]>

Reviewed-by Mike Christie <[email protected]>

---
 drivers/scsi/scsi_lib.c   |    5 +++++
 drivers/scsi/scsi_sysfs.c |    7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

drivers/scsi/scsi_lib.c +5 -0

@@ -452,10 +452,15 @@ static void scsi_run_queue(struct request_queue *q)
 			continue;
 		}
 
+		get_device(&sdev->sdev_gendev);
 		spin_unlock(shost->host_lock);
+
 		spin_lock(sdev->request_queue->queue_lock);
 		__blk_run_queue(sdev->request_queue);
 		spin_unlock(sdev->request_queue->queue_lock);
+
+		put_device(&sdev->sdev_gendev);
+
 		spin_lock(shost->host_lock);
 	}
 	/* put any unprocessed entries back */ diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index ce5224c..2661a957 100644

drivers/scsi/scsi_sysfs.c +2 -1

@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);

@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)  void __scsi_remove_device(struct scsi_device *sdev)  {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0) @@ -977,6 +978,10 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);

+ spin_lock_irqsave(shost->host_lock, flags);
+ list_del(&sdev->starved_entry);
+ spin_unlock_irqrestore(shost->host_lock, flags);
+
if (sdev->host->hostt->slave_destroy)
sdev->host->hostt->slave_destroy(sdev);
transport_destroy_device(dev);
--
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Tejun Heo

maintainer

2012-10-29 01:47 UTC #2 lore raw

Re: [PATCH 1/7] block: Avoid that blk_drain_queue() finishes early

Hello,

Quoted 4 lines · Bart Van Assche

Code like "drain |= q->nr_rqs[i]" might result in blk_drain_queue() to finish early if the expression at the RHS is a multiple of 256 since the drain variable is only eight bits wide. Avoid this by changing the type of the drain variable from bool into unsigned.

No, it doesn't happen that way. One of the reasons we have bool at all is to avoid this type of problems caused by implicit type-casting.

Why do you keep pushing this? It's WRONG. Please drop it.

Thanks.

Signature · 1 line

tejun

Tejun Heo

maintainer

2012-10-29 01:52 UTC #3 lore raw

Re: [PATCH 1/7] block: Avoid that blk_drain_queue() finishes early

Quoted 8 lines · Tejun Heo

Hello,
Quoted 4 lines · Bart Van Assche
Code like "drain |= q->nr_rqs[i]" might result in blk_drain_queue() to finish early if the expression at the RHS is a multiple of 256 since the drain variable is only eight bits wide. Avoid this by changing the type of the drain variable from bool into unsigned.

Quoted 6 lines

No, it doesn't happen that way. One of the reasons we have bool at all is to avoid this type of problems caused by implicit type-casting.
Why do you keep pushing this? It's WRONG. Please drop it.
From C99 std draft.

6.3 Conversions
6.3.1.2 Boolean type

When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1.

It doesn't care the width or signedness of the type being converted. If the origin value equals zero, it converts to 0; otherwise 1.

Thanks.

Signature · 1 line

tejun

Tejun Heo

maintainer Acked-by on v5 2/7

2012-10-29 01:55 UTC #6 lore raw

Re: [PATCH 2/7] block: Let blk_drain_queue() caller obtain the queue lock

Quoted 10 lines · Bart Van Assche

Let the caller of blk_drain_queue() obtain the queue lock to improve readability of the patch called "Avoid that request_fn is invoked on a dead queue".
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Cc Chanho Min <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>

Nit: probably better to make this change right before the said patch.

Thanks.

Signature · 1 line

tejun

Tejun Heo

maintainer

2012-10-29 01:59 UTC #9 lore raw

Re: [PATCH 4/7] block: Avoid that request_fn is invoked on a dead queue

Hello,

Quoted 3 lines · Bart Van Assche

A block driver may start cleaning up resources needed by its request_fn as soon as blk_cleanup_queue() finished, so request_fn must not be invoked after draining finished.

Please be a lot more detailed. Please describe an example scenario where things go wrong, explain why that's happening and how it's fixed.

Quoted 7 lines

block/blk-core.c +7 -0

+void inline __blk_run_queue_uncond(struct request_queue *q)
+{
+	if (unlikely(blk_queue_dead(q)))
+		return;
+
+	q->request_fn(q);
+}

...

block/blk-core.c void blk_cleanup_queue(struct request_queue *q) · line 533

Quoted 6 lines

block/blk-core.c +1 -0

@@ -510,6 +529,7 @@ void blk_cleanup_queue(struct request_queue *q)
 	/* drain all requests queued before DYING marking */
 	spin_lock_irq(lock);
 	__blk_drain_queue(q, true);
+	queue_flag_set(QUEUE_FLAG_DEAD, q);
 	spin_unlock_irq(lock);

And some comments explaining what the dead thing is doing would be nice too; other than that, looks good to me.

Thanks!

Signature · 1 line

tejun

Tejun Heo

maintainer

2012-10-29 02:00 UTC #11 lore raw

Re: [PATCH 5/7] block: Make blk_cleanup_queue() wait until request_fn finished

Quoted 6 lines · Bart Van Assche

Some request_fn implementations, e.g. scsi_request_fn(), unlock the queue lock. Make sure that blk_cleanup_queue() waits until all active request_fn invocations have finished. A block driver may start cleaning up resources needed by its request_fn as soon as blk_cleanup_queue() finished, so blk_cleanup_queue() must wait for all outstanding request_fn invocations to finish.

Ditto. More verbose explanation *please*.

Quoted 16 lines

Reported-by Chanho Min <[email protected]>
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>
---
 block/blk-core.c       |    3 +++
 include/linux/blkdev.h |    6 ++++++
 2 files changed, 9 insertions(+)
block/blk-core.c +0 -0
index 2f1be7a..7662d83 100644

block/blk-core.c void inline __blk_run_queue_uncond(struct request_queue *q) · line 314

Quoted 7 lines

block/blk-core.c +2 -0

@@ -308,7 +308,9 @@ void inline __blk_run_queue_uncond(struct request_queue *q)
 	if (unlikely(blk_queue_dead(q)))
 		return;
 
+	q->request_fn_active++;
 	q->request_fn(q);
+	q->request_fn_active--;
 }

Here too.

Thanks.

Signature · 1 line

tejun

Tejun Heo

maintainer Acked-by on v5 6/7

2012-10-29 02:07 UTC #20 lore raw

Re: [PATCH 6/7] Fix race between starved list processing and device removal

Hello, Bart.

Quoted 4 lines · Bart Van Assche

The SCSI core maintains a "starved list" per SCSI host. This is a list of devices for which one or more requests have been queued but that have not yet been passed to the SCSI LLD. The function scsi_run_queue() examines all SCSI devices on the starved list.

New paragraph.

Quoted 3 lines

Since scsi_remove_device() can be invoked concurrently with scsi_run_queue() it is important to avoid that a SCSI device is accessed by that function after it has been freed.

New paragraph.

Quoted 8 lines

Avoid that the
sdev reference count can drop to zero before the queue is run by scsi_run_queue() by inserting a get_device() / put_device() pair in that function. Move the code for removing a device from the starved list from scsi_device_dev_release_usercontext() to __scsi_remove_device() such that it is guaranteed that the newly added get_device() call succeeds.

Quoted 6 lines

Reported-and-tested-by Chanho Min <[email protected]>
Reference: http://lkml.org/lkml/2012/8/2/96
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Reviewed-by Mike Christie <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>

Heh, for some reason, the commit message is a hard read for me but I think it should do.

Thanks!

Signature · 1 line

tejun

Tejun Heo

maintainer Acked-by on v5 7/7

2012-10-29 02:08 UTC #22 lore raw

Re: [PATCH 7/7] Remove get_device() / put_device() pair from scsi_request_fn()

Quoted 1 line · Bart Van Assche

Since all scsi_request_fn() callers hold a reference on the

Maybe s/Since/Now that/

Quoted 10 lines

SCSI device that function is invoked for and since blk_cleanup_queue() waits until scsi_request_fn() has finished it is safe to remove the get_device() / put_device() pair from scsi_request_fn().
Cc James Bottomley <[email protected]>
Cc Mike Christie <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Tejun Heo <[email protected]>
Signed-off-by Bart Van Assche <[email protected]>

In reply to Zhuang, Jin Can

Thanks.

Signature · 1 line

tejun

Bart Van Assche

author

2012-10-29 14:32 UTC #14 lore raw

Re: [PATCH 6/7] Fix race between starved list processing and device removal

Quoted 9 lines · Jin Can

I recently ran into the same issue
The test I did is plug/unplug u-disk in an interval of 1 second. And I found when sdev1 is being removed, scsi_run_queue is triggered by sdev2, which then accesses all the starving scsi device including sdev1.
I have adopted the solution below which works fine for me so far. But there's one thing to fix in the patch below. When it put_device in scsi_run_queue, irq is disabled. As put_device may get into sleep, irq should be enabled before it's called.

Hello Jincan,

Thanks for testing and the feedback. However, are you sure that put_device() for a SCSI device may sleep ? Have you noticed the execute_in_process_context() call in scsi_device_dev_release() ?

Bart.

Bart Van Assche

author

2012-10-29 14:35 UTC #4 lore raw

Re: [PATCH 1/7] block: Avoid that blk_drain_queue() finishes early

In reply to Tejun Heo

Quoted 14 lines · Tejun Heo

Quoted 6 lines · Tejun Heo
Quoted 4 lines · Bart Van Assche
Code like "drain |= q->nr_rqs[i]" might result in blk_drain_queue() to finish early if the expression at the RHS is a multiple of 256 since the drain variable is only eight bits wide. Avoid this by changing the type of the drain variable from bool into unsigned.
Quoted 6 lines
No, it doesn't happen that way. One of the reasons we have bool at all is to avoid this type of problems caused by implicit type-casting.
Why do you keep pushing this? It's WRONG. Please drop it.
From C99 std draft.

Quoted 8 lines

6.3 Conversions
6.3.1.2 Boolean type
When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1.
It doesn't care the width or signedness of the type being converted. If the origin value equals zero, it converts to 0; otherwise 1.

For one or another reason I was assuming that bool was a typedef for unsigned char. But you are right, it's a synonym for the C99 type _Bool so I'll drop this patch.

Bart.

Zhuang, Jin Can

2012-10-30 05:40 UTC #15 lore raw

RE: [PATCH 6/7] Fix race between starved list processing and device removal

In reply to a message not in the archive

Hi Bart,

Yes. Here's the warning.
For the trace below, I used scsi_device_get/scsi_device_put() in scsi_run_queue(). (A little different from your patch). But I think it's the same.

10-23 18:15:53.309     8     8 I KERNEL  : [  268.994556] BUG: sleeping function called from invalid context at linux-2.6/kernel/workqueue.c:2500
10-23 18:15:53.309     8     8 I KERNEL  : [  269.006898] in_atomic(): 0, irqs_disabled(): 1, pid: 8, name: kworker/0:1
10-23 18:15:53.309     8     8 I KERNEL  : [  269.013689] Pid: 8, comm: kworker/0:1 Tainted: G        WC  3.0.34-140359-g85a6d67-dirty #43
10-23 18:15:53.309     8     8 I KERNEL  : [  269.022113] Call Trace:
10-23 18:15:53.309     8     8 I KERNEL  : [  269.024567]  [<c1859ea5>] ? printk+0x1d/0x1f
10-23 18:15:53.309     8     8 I KERNEL  : [  269.028828]  [<c123464a>] __might_sleep+0x10a/0x110
10-23 18:15:53.309     8     8 I KERNEL  : [  269.033695]  [<c12628a3>] wait_on_work+0x23/0x1a0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.038390]  [<c1863ee6>] ? _raw_spin_unlock_irqrestore+0x26/0x50
10-23 18:15:53.309     8     8 I KERNEL  : [  269.044476]  [<c152fd66>] ? __pm_runtime_idle+0x66/0xf0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.049706]  [<c165ae3e>] ? ram_console_write+0x4e/0xa0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.054913]  [<c126472a>] __cancel_work_timer+0x6a/0x110
10-23 18:15:53.309     8     8 I KERNEL  : [  269.060217]  [<c12647ff>] cancel_work_sync+0xf/0x20
10-23 18:15:53.309     8     8 I KERNEL  : [  269.065087]  [<c1548d5d>] scsi_device_dev_release_usercontext+0x6d/0x100
10-23 18:15:53.309     8     8 I KERNEL  : [  269.071785]  [<c12626a2>] execute_in_process_context+0x42/0x50
10-23 18:15:53.309     8     8 I KERNEL  : [  269.077609]  [<c1548cc8>] scsi_device_dev_release+0x18/0x20
10-23 18:15:53.309     8     8 I KERNEL  : [  269.083174]  [<c15234a0>] device_release+0x20/0x80
10-23 18:15:53.309     8     8 I KERNEL  : [  269.087958]  [<c124a21e>] ? vprintk+0x2be/0x4e0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.092479]  [<c148d1b4>] kobject_release+0x84/0x1f0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.097439]  [<c1863d22>] ? _raw_spin_lock_irq+0x22/0x30
10-23 18:15:53.309     8     8 I KERNEL  : [  269.102732]  [<c148d130>] ? kobject_del+0x70/0x70
10-23 18:15:53.309     8     8 I KERNEL  : [  269.107430]  [<c148e8ec>] kref_put+0x2c/0x60
10-23 18:15:53.309     8     8 I KERNEL  : [  269.111688]  [<c148d06d>] kobject_put+0x1d/0x50
10-23 18:15:53.309     8     8 I KERNEL  : [  269.116209]  [<c15232a4>] put_device+0x14/0x20
10-23 18:15:53.309     8     8 I KERNEL  : [  269.120646]  [<c153daa7>] scsi_device_put+0x37/0x60
10-23 18:15:53.309     8     8 I KERNEL  : [  269.125515]  [<c1543cc7>] scsi_run_queue+0x247/0x320
10-23 18:15:53.309     8     8 I KERNEL  : [  269.130470]  [<c1545903>] scsi_requeue_run_queue+0x13/0x20
10-23 18:15:53.309     8     8 I KERNEL  : [  269.135941]  [<c1263efe>] process_one_work+0xfe/0x3f0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.140997]  [<c15458f0>] ? scsi_softirq_done+0x120/0x120
10-23 18:15:53.309     8     8 I KERNEL  : [  269.146384]  [<c12644f1>] worker_thread+0x121/0x2f0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.151254]  [<c12643d0>] ? rescuer_thread+0x1e0/0x1e0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.156383]  [<c1267ffd>] kthread+0x6d/0x80
10-23 18:15:53.309     8     8 I KERNEL  : [  269.160558]  [<c1267f90>] ? __init_kthread_worker+0x30/0x30
10-23 18:15:53.309     8     8 I KERNEL  : [  269.166124]  [<c186a27a>] kernel_thread_helper+0x6/0x10

-Jincan

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Bart Van Assche
Sent: Monday, October 29, 2012 10:32 PM
To: Zhuang, Jin Can
Cc: linux-scsi; James Bottomley; Mike Christie; Jens Axboe; Tejun Heo; Chanho Min
Subject: Re: [PATCH 6/7] Fix race between starved list processing and device removal

Quoted 7 lines · Jin Can

I recently ran into the same issue
The test I did is plug/unplug u-disk in an interval of 1 second. And
I found when sdev1 is being removed, scsi_run_queue is triggered by > sdev2, which then accesses all the starving scsi device including sdev1.
I have adopted the solution below which works fine for me so far.
But there's one thing to fix in the patch below. When it put_device
in scsi_run_queue, irq is disabled. As put_device may get into sleep, > irq should be enabled before it's called.

Hello Jincan,

Thanks for testing and the feedback. However, are you sure that put_device() for a SCSI device may sleep ? Have you noticed the execute_in_process_context() call in scsi_device_dev_release() ?

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Bart Van Assche

author

2012-11-02 10:48 UTC #16 lore raw

Re: [PATCH 6/7] Fix race between starved list processing and device removal

Quoted 18 lines · Jin Can

Yes. Here's the warning.
For the trace below, I used scsi_device_get/scsi_device_put() in scsi_run_queue(). (A little different from your patch). But I think it's the same.

10-23 18:15:53.309     8     8 I KERNEL  : [  268.994556] BUG: sleeping function called from invalid context at linux-2.6/kernel/workqueue.c:2500
10-23 18:15:53.309     8     8 I KERNEL  : [  269.006898] in_atomic(): 0, irqs_disabled(): 1, pid: 8, name: kworker/0:1
10-23 18:15:53.309     8     8 I KERNEL  : [  269.013689] Pid: 8, comm: kworker/0:1 Tainted: G        WC  3.0.34-140359-g85a6d67-dirty #43
10-23 18:15:53.309     8     8 I KERNEL  : [  269.022113] Call Trace:
10-23 18:15:53.309     8     8 I KERNEL  : [  269.028828]  [<c123464a>] __might_sleep+0x10a/0x110
10-23 18:15:53.309     8     8 I KERNEL  : [  269.033695]  [<c12628a3>] wait_on_work+0x23/0x1a0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.054913]  [<c126472a>] __cancel_work_timer+0x6a/0x110
10-23 18:15:53.309     8     8 I KERNEL  : [  269.060217]  [<c12647ff>] cancel_work_sync+0xf/0x20
10-23 18:15:53.309     8     8 I KERNEL  : [  269.065087]  [<c1548d5d>] scsi_device_dev_release_usercontext+0x6d/0x100
10-23 18:15:53.309     8     8 I KERNEL  : [  269.071785]  [<c12626a2>] execute_in_process_context+0x42/0x50
10-23 18:15:53.309     8     8 I KERNEL  : [  269.077609]  [<c1548cc8>] scsi_device_dev_release+0x18/0x20
10-23 18:15:53.309     8     8 I KERNEL  : [  269.083174]  [<c15234a0>] device_release+0x20/0x80
10-23 18:15:53.309     8     8 I KERNEL  : [  269.092479]  [<c148d1b4>] kobject_release+0x84/0x1f0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.107430]  [<c148e8ec>] kref_put+0x2c/0x60
10-23 18:15:53.309     8     8 I KERNEL  : [  269.111688]  [<c148d06d>] kobject_put+0x1d/0x50

Quoted 8 lines

10-23 18:15:53.309     8     8 I KERNEL  : [  269.116209]  [<c15232a4>] put_device+0x14/0x20
10-23 18:15:53.309     8     8 I KERNEL  : [  269.120646]  [<c153daa7>] scsi_device_put+0x37/0x60
10-23 18:15:53.309     8     8 I KERNEL  : [  269.125515]  [<c1543cc7>] scsi_run_queue+0x247/0x320
10-23 18:15:53.309     8     8 I KERNEL  : [  269.130470]  [<c1545903>] scsi_requeue_run_queue+0x13/0x20
10-23 18:15:53.309     8     8 I KERNEL  : [  269.135941]  [<c1263efe>] process_one_work+0xfe/0x3f0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.146384]  [<c12644f1>] worker_thread+0x121/0x2f0
10-23 18:15:53.309     8     8 I KERNEL  : [  269.156383]  [<c1267ffd>] kthread+0x6d/0x80
10-23 18:15:53.309     8     8 I KERNEL  : [  269.166124]  [<c186a27a>] kernel_thread_helper+0x6/0x10

Thanks for the feedback. Something that kept me busy since I posted the patch at the start of this thread is how to avoid adding two atomic operations in a hot path (the get_device() and put_device() calls in scsi_run_queue()). The patch below should realize that. However, since I haven't been able so far to trigger the above call trace that means that the test I ran wasn't sufficient to trigger all code paths. So it would be appreciated if anyone could help testing the patch below.

[PATCH] Fix race between starved list processing and device removal

---
 block/blk-core.c          |    9 +++++----
 drivers/scsi/scsi_lib.c   |   20 ++++++++++++++------
 drivers/scsi/scsi_sysfs.c |    9 ++++++++-
 3 files changed, 27 insertions(+), 11 deletions(-)

block/blk-core.c +5 -4

index e4f4e06..565484f 100644
@@ -407,10 +407,11 @@ static void __blk_drain_queue(struct request_queue *q, bool drain_all)
 
 		/*
 		 * This function might be called on a queue which failed
-		 * driver init after queue creation or is not yet fully
-		 * active yet.  Some drivers (e.g. fd and loop) get unhappy
-		 * in such cases.  Kick queue iff dispatch queue has
-		 * something on it and @q has request_fn set.
+		 * driver init after queue creation, is not yet fully active
+		 * or is being cleaned up and doesn't make progress anymore
+		 * (e.g. a SCSI device in state SDEV_DEL). Kick queue iff
+		 * dispatch queue has something on it and @q has request_fn
+		 * set.
 		 */
 		if (!list_empty(&q->queue_head) && q->request_fn)
 			__blk_run_queue(q);

drivers/scsi/scsi_lib.c +14 -6

index 488035b..1763181 100644
@@ -447,8 +447,9 @@ static void scsi_run_queue(struct request_queue *q)
 				  struct scsi_device, starved_entry);
 		list_del_init(&sdev->starved_entry);
 		if (scsi_target_is_busy(scsi_target(sdev))) {
-			list_move_tail(&sdev->starved_entry,
-				       &shost->starved_list);
+			if (sdev->sdev_state != SDEV_DEL)
+				list_add_tail(&sdev->starved_entry,
+					      &shost->starved_list);
 			continue;
 		}

@@ -1344,7 +1345,9 @@ static inline int scsi_target_queue_ready(struct Scsi_Host *shost,
 	}
 
 	if (scsi_target_is_busy(starget)) {
-		list_move_tail(&sdev->starved_entry, &shost->starved_list);
+		if (sdev->sdev_state != SDEV_DEL)
+			list_move_tail(&sdev->starved_entry,
+				       &shost->starved_list);
 		return 0;
 	}

@@ -1377,8 +1380,11 @@ static inline int scsi_host_queue_ready(struct request_queue *q,
 		}
 	}
 	if (scsi_host_is_busy(shost)) {
-		if (list_empty(&sdev->starved_entry))
-			list_add_tail(&sdev->starved_entry, &shost->starved_list);
+		if (list_empty(&sdev->starved_entry) &&
+		    sdev->sdev_state != SDEV_DEL) {
+			list_add_tail(&sdev->starved_entry,
+				      &shost->starved_list);
+		}
 		return 0;
 	}

@@ -1571,9 +1577,11 @@ static void scsi_request_fn(struct request_queue *q)
 		 * a run when a tag is freed.
 		 */
 		if (blk_queue_tagged(q) && !blk_rq_tagged(req)) {
-			if (list_empty(&sdev->starved_entry))
+			if (list_empty(&sdev->starved_entry) &&
+			    sdev->sdev_state != SDEV_DEL) {
 				list_add_tail(&sdev->starved_entry,
 					      &shost->starved_list);
+			}
 			goto not_ready;
 		}

drivers/scsi/scsi_sysfs.c +8 -1

index ce5224c..2f0f31e 100644
@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 	starget->reap_ref++;
 	list_del(&sdev->siblings);
 	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
 	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
 	cancel_work_sync(&sdev->event_work);

@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 void __scsi_remove_device(struct scsi_device *sdev)
 {
 	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
 	if (sdev->is_visible) {
 		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

@@ -973,7 +974,13 @@ void __scsi_remove_device(struct scsi_device *sdev)
 	 * scsi_run_queue() invocations have finished before tearing down the
 	 * device.
 	 */
+
 	scsi_device_set_state(sdev, SDEV_DEL);
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
 	blk_cleanup_queue(sdev->request_queue);
 	cancel_work_sync(&sdev->requeue_work);

Signature · 1 line

1.7.10.4

Bart Van Assche

author

2012-11-21 11:06 UTC #17 lore raw

Re: [PATCH 6/7] Fix race between starved list processing and device removal

Quoted 27 lines · Bart Van Assche

[PATCH] Fix race between starved list processing and device removal [ ... ]

drivers/scsi/scsi_sysfs.c +2 -1

index ce5224c..2f0f31e 100644
@@ -348,7 +348,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
  	starget->reap_ref++;
  	list_del(&sdev->siblings);
  	list_del(&sdev->same_target_siblings);
-	list_del(&sdev->starved_entry);
  	spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
  	cancel_work_sync(&sdev->event_work);

@@ -956,6 +955,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
  void __scsi_remove_device(struct scsi_device *sdev)
  {
  	struct device *dev = &sdev->sdev_gendev;
+	struct Scsi_Host *shost = sdev->host;
+	unsigned long flags;
 
  	if (sdev->is_visible) {
  		if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)

@@ -973,7 +974,13 @@ void __scsi_remove_device(struct scsi_device *sdev)
  	 * scsi_run_queue() invocations have finished before tearing down the
  	 * device.
  	 */

drivers/scsi/scsi_sysfs.c void __scsi_remove_device(struct scsi_device *sdev) · line 986

Quoted 10 lines

drivers/scsi/scsi_sysfs.c +6 -0

+
  	scsi_device_set_state(sdev, SDEV_DEL);
+
+	spin_lock_irqsave(shost->host_lock, flags);
+	list_del(&sdev->starved_entry);
+	spin_unlock_irqrestore(shost->host_lock, flags);
+
  	blk_cleanup_queue(sdev->request_queue);
  	cancel_work_sync(&sdev->requeue_work);

Please ignore this patch. Even with this patch applied there is still a race condition present, namely that the __blk_run_queue() call in scsi_run_queue() can get invoked after __scsi_remove_device() invoked put_device().

Bart.

Bart Van Assche

author

2012-11-21 12:10 UTC #19 lore raw

Re: [PATCH 6/7] Fix race between starved list processing and device removal

Quoted 13 lines · Chanho Min

Quoted 3 lines
Yes. Here's the warning.
For the trace below, I used scsi_device_get/scsi_device_put() in scsi_run_queue(). (A little different from your patch). But I think it's the same.
I think it's correct. cancel_work_sync can sleep. It is caught under CONFIG_DEBUG_ATOMIC_SLEEP. What if we only enable irq at cancel_work_sync as the patch bellows?
drivers/scsi/scsi_sysfs.c +0 -0
index bb7c482..6e17db9 100644

drivers/scsi/scsi_sysfs.c static void scsi_device_dev_release_usercontext(struct work_struct *work) · line 358

Quoted 9 lines

drivers/scsi/scsi_sysfs.c +2 -0

@@ -350,7 +350,9 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
         list_del(&sdev->starved_entry);
         spin_unlock_irqrestore(sdev->host->host_lock, flags);
 
+       local_irq_enable();
         cancel_work_sync(&sdev->event_work);
+       local_irq_restore(flags);
 
         list_for_each_safe(this, tmp, &sdev->event_list) {
                 struct scsi_event *evt;

As far as I can see this should work but unfortunately this change creates a nontrivial dependency between scsi_run_queue() and scsi_device_dev_release_usercontext(). Personally I would prefer something like this follow-up patch:

drivers/scsi/scsi_lib.c +3 -6

index 71bddec..20ea2e9 100644
@@ -453,15 +453,12 @@ static void scsi_run_queue(struct request_queue *q)
                }
 
                get_device(&sdev->sdev_gendev);
-               spin_unlock(shost->host_lock);
-
-               spin_lock(sdev->request_queue->queue_lock);
-               __blk_run_queue(sdev->request_queue);
-               spin_unlock(sdev->request_queue->queue_lock);
+               spin_unlock_irqrestore(shost->host_lock, flags);
 
+               blk_run_queue(sdev->request_queue);
                put_device(&sdev->sdev_gendev);
 
-               spin_lock(shost->host_lock);
+               spin_lock_irqsave(shost->host_lock, flags);
        }
        /* put any unprocessed entries back */
        list_splice(&starved_list, &shost->starved_list);

Bart.

Bart Van Assche

author

2012-11-23 10:38 UTC #23 lore raw

Quoted 2 lines · Bart Van Assche

Fix a few race conditions that can be triggered by removing a device:
[ ... ]

Hello,

I'd like to add the patch below to this series. This is something I came up with after analyzing why a crash was triggered during an SRP failover test. One of the functions in the crash call stack was blk_delay_work().

Bart.

[PATCH] block: Avoid scheduling delayed work on a dead queue

Running a queue must continue after it has been marked dying until it has been marked dead. So the function blk_run_queue_async() must not schedule delayed work after blk_cleanup_queue() has marked a queue dead. Hence add a test for that queue state in blk_run_queue_async() and make sure that queue_unplugged() invokes that function with the queue lock held. This avoids that the queue state can change after it has been tested and before mod_delayed_work() is invoked. Drop the queue dying test in queue_unplugged() since it is now
superfluous: __blk_run_queue() already tests whether or not the queue is dead.