| Message ID | 20170925202924.16603-6-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 25, 2017 at 01:29:22PM -0700, Bart Van Assche wrote: > Avoid that it can take 200 ms too long to wait for requests to > finish. Note: blk_mq_freeze_queue() uses a wait queue to wait > for ongoing requests to finish. > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Martin K. Petersen <martin.petersen@oracle.com> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Johannes Thumshirn <jthumshirn@suse.de> > --- > drivers/scsi/scsi_lib.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 62f905b22821..c261498606c4 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -2927,6 +2927,7 @@ static void scsi_wait_for_queuecommand(struct scsi_device *sdev) > int > scsi_device_quiesce(struct scsi_device *sdev) > { > + struct request_queue *q = sdev->request_queue; > int err; > > mutex_lock(&sdev->state_mutex); > @@ -2936,11 +2937,8 @@ scsi_device_quiesce(struct scsi_device *sdev) > if (err) > return err; > > - scsi_run_queue(sdev->request_queue); > - while (atomic_read(&sdev->device_busy)) { > - msleep_interruptible(200); > - scsi_run_queue(sdev->request_queue); > - } > + blk_mq_freeze_queue(q); > + blk_mq_unfreeze_queue(q); > return 0; I see the serious issue now with this patch, and it should have been found much earlier, just because the setting quiesce isn't shown in the patch. Sorry for finding it so late. Once the patch is applied, scsi_device_quiesce() becomes the following shape: mutex_lock(&sdev->state_mutex); err = scsi_device_set_state(sdev, SDEV_QUIESCE); if (err == 0) blk_set_preempt_only(q, true); mutex_unlock(&sdev->state_mutex); if (err) return err; blk_mq_freeze_queue(q); blk_mq_unfreeze_queue(q); Any requests allocated before scsi_device_set_state() and dispatched after scsi_device_set_state() can't be drained up any more, then blk_mq_freeze_queue() will wait forever for the drain up, so not only this patchset can't fix the current quiesce issue, but also introduces new I/O hang in scsi_device_quiesce(). Now this approach looks broken fundamentally. NAK.
On Wed, 2017-09-27 at 06:23 +0800, Ming Lei wrote: > mutex_lock(&sdev->state_mutex); > err = scsi_device_set_state(sdev, SDEV_QUIESCE); > if (err == 0) > blk_set_preempt_only(q, true); > mutex_unlock(&sdev->state_mutex); > > if (err) > return err; > > blk_mq_freeze_queue(q); > blk_mq_unfreeze_queue(q); > > Any requests allocated before scsi_device_set_state() and > dispatched after scsi_device_set_state() can't be drained up > any more, then blk_mq_freeze_queue() will wait forever for the > drain up, so not only this patchset can't fix the current quiesce > issue, but also introduces new I/O hang in scsi_device_quiesce(). That's a good catch, but fortunately this is very easy to fix: move the blk_mq_freeze_queue() call before the mutex_lock() and scsi_device_set_state() calls. The error path will also need some adjustment. Bart.
On Wed, Sep 27, 2017 at 03:43:11AM +0000, Bart Van Assche wrote: > On Wed, 2017-09-27 at 06:23 +0800, Ming Lei wrote: > > mutex_lock(&sdev->state_mutex); > > err = scsi_device_set_state(sdev, SDEV_QUIESCE); > > if (err == 0) > > blk_set_preempt_only(q, true); > > mutex_unlock(&sdev->state_mutex); > > > > if (err) > > return err; > > > > blk_mq_freeze_queue(q); > > blk_mq_unfreeze_queue(q); > > > > Any requests allocated before scsi_device_set_state() and > > dispatched after scsi_device_set_state() can't be drained up > > any more, then blk_mq_freeze_queue() will wait forever for the > > drain up, so not only this patchset can't fix the current quiesce > > issue, but also introduces new I/O hang in scsi_device_quiesce(). > > That's a good catch, but fortunately this is very easy to fix: move the > blk_mq_freeze_queue() call before the mutex_lock() and scsi_device_set_state() > calls. The error path will also need some adjustment. I am also working towards that direction, patches have been ready.
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 62f905b22821..c261498606c4 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2927,6 +2927,7 @@ static void scsi_wait_for_queuecommand(struct scsi_device *sdev) int scsi_device_quiesce(struct scsi_device *sdev) { + struct request_queue *q = sdev->request_queue; int err; mutex_lock(&sdev->state_mutex); @@ -2936,11 +2937,8 @@ scsi_device_quiesce(struct scsi_device *sdev) if (err) return err; - scsi_run_queue(sdev->request_queue); - while (atomic_read(&sdev->device_busy)) { - msleep_interruptible(200); - scsi_run_queue(sdev->request_queue); - } + blk_mq_freeze_queue(q); + blk_mq_unfreeze_queue(q); return 0; } EXPORT_SYMBOL(scsi_device_quiesce);
Avoid that it can take 200 ms too long to wait for requests to finish. Note: blk_mq_freeze_queue() uses a wait queue to wait for ongoing requests to finish. Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: Ming Lei <ming.lei@redhat.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Hannes Reinecke <hare@suse.com> Cc: Johannes Thumshirn <jthumshirn@suse.de> --- drivers/scsi/scsi_lib.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)