| Message ID | 20211103034305.3691555-4-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block: fix concurrent quiesce | expand |
On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote: [...] > +void scsi_start_queue(struct scsi_device *sdev) > +{ > + if (cmpxchg(&sdev->queue_stopped, 1, 0)) > + blk_mq_unquiesce_queue(sdev->request_queue); > +} > + > +static void scsi_stop_queue(struct scsi_device *sdev, bool nowait) > +{ > + if (!cmpxchg(&sdev->queue_stopped, 0, 1)) { > + if (nowait) > + blk_mq_quiesce_queue_nowait(sdev- > >request_queue); > + else > + blk_mq_quiesce_queue(sdev->request_queue); > + } else { > + if (!nowait) > + blk_mq_wait_quiesce_done(sdev->request_queue); > + } > +} This looks counter intuitive. I assume it's done so that if we call scsi_stop_queue when the queue has already been stopped, it waits until the queue is actually quiesced before returning so the behaviour is the same in the !nowait case? Some sort of comment explaining that would be useful. James
Hello James, On Mon, Nov 08, 2021 at 11:42:01AM -0500, James Bottomley wrote: > On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote: > [...] > > +void scsi_start_queue(struct scsi_device *sdev) > > +{ > > + if (cmpxchg(&sdev->queue_stopped, 1, 0)) > > + blk_mq_unquiesce_queue(sdev->request_queue); > > +} > > + > > +static void scsi_stop_queue(struct scsi_device *sdev, bool nowait) > > +{ > > + if (!cmpxchg(&sdev->queue_stopped, 0, 1)) { > > + if (nowait) > > + blk_mq_quiesce_queue_nowait(sdev- > > >request_queue); > > + else > > + blk_mq_quiesce_queue(sdev->request_queue); > > + } else { > > + if (!nowait) > > + blk_mq_wait_quiesce_done(sdev->request_queue); > > + } > > +} > > This looks counter intuitive. I assume it's done so that if we call > scsi_stop_queue when the queue has already been stopped, it waits until The motivation is to balance blk_mq_quiesce_queue_nowait()/blk_mq_quiesce_queue() and blk_mq_unquiesce_queue(). That needs one extra mutex to cover the quiesce action and update the flag, but we can't hold the mutex in scsi_internal_device_block_nowait(), so take this way with the atomic flag. > the queue is actually quiesced before returning so the behaviour is the > same in the !nowait case? Some sort of comment explaining that would > be useful. I will add comment on the current usage. Thanks, Ming
Hello James, On Tue, Nov 09, 2021 at 08:44:06AM +0800, Ming Lei wrote: > Hello James, > > On Mon, Nov 08, 2021 at 11:42:01AM -0500, James Bottomley wrote: > > On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote: > > [...] > > > +void scsi_start_queue(struct scsi_device *sdev) > > > +{ > > > + if (cmpxchg(&sdev->queue_stopped, 1, 0)) > > > + blk_mq_unquiesce_queue(sdev->request_queue); > > > +} > > > + > > > +static void scsi_stop_queue(struct scsi_device *sdev, bool nowait) > > > +{ > > > + if (!cmpxchg(&sdev->queue_stopped, 0, 1)) { > > > + if (nowait) > > > + blk_mq_quiesce_queue_nowait(sdev- > > > >request_queue); > > > + else > > > + blk_mq_quiesce_queue(sdev->request_queue); > > > + } else { > > > + if (!nowait) > > > + blk_mq_wait_quiesce_done(sdev->request_queue); > > > + } > > > +} > > > > This looks counter intuitive. I assume it's done so that if we call > > scsi_stop_queue when the queue has already been stopped, it waits until > > The motivation is to balance blk_mq_quiesce_queue_nowait()/blk_mq_quiesce_queue() > and blk_mq_unquiesce_queue(). > > That needs one extra mutex to cover the quiesce action and update > the flag, but we can't hold the mutex in scsi_internal_device_block_nowait(), > so take this way with the atomic flag. > > > the queue is actually quiesced before returning so the behaviour is the > > same in the !nowait case? Some sort of comment explaining that would > > be useful. > > I will add comment on the current usage. Are you fine with the following comment? diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index e8925a35cb3a..9e3bf028f95a 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2661,6 +2661,13 @@ void scsi_start_queue(struct scsi_device *sdev) static void scsi_stop_queue(struct scsi_device *sdev, bool nowait) { + /* + * The atomic variable of ->queue_stopped covers that + * blk_mq_quiesce_queue* is balanced with blk_mq_unquiesce_queue. + * + * However, we still need to wait until quiesce is done + * in case that queue has been stopped. + */ if (!cmpxchg(&sdev->queue_stopped, 0, 1)) { if (nowait) blk_mq_quiesce_queue_nowait(sdev->request_queue); Thanks, Ming
On Tue, 2021-11-09 at 11:18 +0800, Ming Lei wrote: > Hello James, > > On Tue, Nov 09, 2021 at 08:44:06AM +0800, Ming Lei wrote: > > Hello James, > > > > On Mon, Nov 08, 2021 at 11:42:01AM -0500, James Bottomley wrote: > > > On Wed, 2021-11-03 at 11:43 +0800, Ming Lei wrote: > > > [...] > > > > +void scsi_start_queue(struct scsi_device *sdev) > > > > +{ > > > > + if (cmpxchg(&sdev->queue_stopped, 1, 0)) > > > > + blk_mq_unquiesce_queue(sdev->request_queue); > > > > +} > > > > + > > > > +static void scsi_stop_queue(struct scsi_device *sdev, bool > > > > nowait) > > > > +{ > > > > + if (!cmpxchg(&sdev->queue_stopped, 0, 1)) { > > > > + if (nowait) > > > > + blk_mq_quiesce_queue_nowait(sdev- > > > > > request_queue); > > > > + else > > > > + blk_mq_quiesce_queue(sdev- > > > > >request_queue); > > > > + } else { > > > > + if (!nowait) > > > > + blk_mq_wait_quiesce_done(sdev- > > > > >request_queue); > > > > + } > > > > +} > > > > > > This looks counter intuitive. I assume it's done so that if we > > > call > > > scsi_stop_queue when the queue has already been stopped, it waits > > > until > > > > The motivation is to balance > > blk_mq_quiesce_queue_nowait()/blk_mq_quiesce_queue() > > and blk_mq_unquiesce_queue(). > > > > That needs one extra mutex to cover the quiesce action and update > > the flag, but we can't hold the mutex in > > scsi_internal_device_block_nowait(), > > so take this way with the atomic flag. > > > > > the queue is actually quiesced before returning so the behaviour > > > is the > > > same in the !nowait case? Some sort of comment explaining that > > > would > > > be useful. > > > > I will add comment on the current usage. > > Are you fine with the following comment? > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index e8925a35cb3a..9e3bf028f95a 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -2661,6 +2661,13 @@ void scsi_start_queue(struct scsi_device > *sdev) > > static void scsi_stop_queue(struct scsi_device *sdev, bool nowait) > { > + /* > + * The atomic variable of ->queue_stopped covers that > + * blk_mq_quiesce_queue* is balanced with > blk_mq_unquiesce_queue. > + * > + * However, we still need to wait until quiesce is done > + * in case that queue has been stopped. > + */ > if (!cmpxchg(&sdev->queue_stopped, 0, 1)) { > if (nowait) > blk_mq_quiesce_queue_nowait(sdev- > >request_queue); Yes, that looks fine ... it will at least act as a caution for maintainers who come after us. James
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 1cd3ef9056d5..e8925a35cb3a 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2653,6 +2653,25 @@ static int __scsi_internal_device_block_nowait(struct scsi_device *sdev) return 0; } +void scsi_start_queue(struct scsi_device *sdev) +{ + if (cmpxchg(&sdev->queue_stopped, 1, 0)) + blk_mq_unquiesce_queue(sdev->request_queue); +} + +static void scsi_stop_queue(struct scsi_device *sdev, bool nowait) +{ + if (!cmpxchg(&sdev->queue_stopped, 0, 1)) { + if (nowait) + blk_mq_quiesce_queue_nowait(sdev->request_queue); + else + blk_mq_quiesce_queue(sdev->request_queue); + } else { + if (!nowait) + blk_mq_wait_quiesce_done(sdev->request_queue); + } +} + /** * scsi_internal_device_block_nowait - try to transition to the SDEV_BLOCK state * @sdev: device to block @@ -2677,7 +2696,7 @@ int scsi_internal_device_block_nowait(struct scsi_device *sdev) * request queue. */ if (!ret) - blk_mq_quiesce_queue_nowait(sdev->request_queue); + scsi_stop_queue(sdev, true); return ret; } EXPORT_SYMBOL_GPL(scsi_internal_device_block_nowait); @@ -2704,19 +2723,12 @@ static int scsi_internal_device_block(struct scsi_device *sdev) mutex_lock(&sdev->state_mutex); err = __scsi_internal_device_block_nowait(sdev); if (err == 0) - blk_mq_quiesce_queue(sdev->request_queue); + scsi_stop_queue(sdev, false); mutex_unlock(&sdev->state_mutex); return err; } -void scsi_start_queue(struct scsi_device *sdev) -{ - struct request_queue *q = sdev->request_queue; - - blk_mq_unquiesce_queue(q); -} - /** * scsi_internal_device_unblock_nowait - resume a device after a block request * @sdev: device to resume diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 430b73bd02ac..d1c6fc83b1e3 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -207,6 +207,7 @@ struct scsi_device { * creation time */ unsigned ignore_media_change:1; /* Ignore MEDIA CHANGE on resume */ + unsigned int queue_stopped; /* request queue is quiesced */ bool offline_already; /* Device offline message logged */ atomic_t disk_events_disable_depth; /* disable depth for disk events */
For fixing queue quiesce race between driver and block layer(elevator switch, update nr_requests, ...), we need to support concurrent quiesce and unquiesce, which requires the two call balanced. It isn't easy to audit that in all scsi drivers, especially the two may be called from different contexts, so do it in scsi core with one per-device atomic variable to balance quiesce and unquiesce. Reported-by: Yi Zhang <yi.zhang@redhat.com> Fixes: e70feb8b3e68 ("blk-mq: support concurrent queue quiesce/unquiesce") Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/scsi/scsi_lib.c | 30 +++++++++++++++++++++--------- include/scsi/scsi_device.h | 1 + 2 files changed, 22 insertions(+), 9 deletions(-)