| Message ID | 20170711182103.11461-2-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 12, 2017 at 02:20:58AM +0800, Ming Lei wrote: > This interfaces will be removed soon, so use quiesce and > unquiesce instead, which should be more safe. 'should be'? That does not sound encouraging? > > The only one usage will be removed in the following > congestion control patches. > > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: "Roger Pau Monné" <roger.pau@citrix.com> > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Juergen Gross <jgross@suse.com> > Cc: xen-devel@lists.xenproject.org > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > drivers/block/xen-blkfront.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > > diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c > index c852ed3c01d5..1578befda635 100644 > --- a/drivers/block/xen-blkfront.c > +++ b/drivers/block/xen-blkfront.c > @@ -1187,7 +1187,7 @@ static void xlvbd_release_gendisk(struct blkfront_info *info) > return; > > /* No more blkif_request(). */ > - blk_mq_stop_hw_queues(info->rq); > + blk_mq_quiesce_queue(info->rq); > > for (i = 0; i < info->nr_rings; i++) { > struct blkfront_ring_info *rinfo = &info->rinfo[i]; > @@ -1216,8 +1216,10 @@ static void xlvbd_release_gendisk(struct blkfront_info *info) > /* Already hold rinfo->ring_lock. */ > static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo) > { > - if (!RING_FULL(&rinfo->ring)) > + if (!RING_FULL(&rinfo->ring)) { > blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true); > + blk_mq_kick_requeue_list(rinfo->dev_info->rq); > + } > } > > static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > @@ -1225,7 +1227,8 @@ static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > unsigned long flags; > > spin_lock_irqsave(&rinfo->ring_lock, flags); > - kick_pending_request_queues_locked(rinfo); > + if (!RING_FULL(&rinfo->ring)) > + blk_mq_run_hw_queues(rinfo->dev_info->rq, true); > spin_unlock_irqrestore(&rinfo->ring_lock, flags); > } > > @@ -1346,7 +1349,7 @@ static void blkif_free(struct blkfront_info *info, int suspend) > BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED; > /* No more blkif_request(). */ > if (info->rq) > - blk_mq_stop_hw_queues(info->rq); > + blk_mq_quiesce_queue(info->rq); > > for (i = 0; i < info->nr_rings; i++) > blkif_free_ring(&info->rinfo[i]); > @@ -2018,22 +2021,13 @@ static int blkif_recover(struct blkfront_info *info) > /* Now safe for us to use the shared ring */ > info->connected = BLKIF_STATE_CONNECTED; > > - for (r_index = 0; r_index < info->nr_rings; r_index++) { > - struct blkfront_ring_info *rinfo; > - > - rinfo = &info->rinfo[r_index]; > - /* Kick any other new requests queued since we resumed */ > - kick_pending_request_queues(rinfo); > - } > - > list_for_each_entry_safe(req, n, &info->requests, queuelist) { > /* Requeue pending requests (flush or discard) */ > list_del_init(&req->queuelist); > BUG_ON(req->nr_phys_segments > segs); > blk_mq_requeue_request(req, false); > } > - blk_mq_start_stopped_hw_queues(info->rq, true); > - blk_mq_kick_requeue_list(info->rq); > + blk_mq_unquiesce_queue(info->rq); > > while ((bio = bio_list_pop(&info->bio_list)) != NULL) { > /* Traverse the list of pending bios and re-queue them */ > -- > 2.9.4 >
On Wed, 2017-07-12 at 02:20 +0800, Ming Lei wrote: > This interfaces will be removed soon, so use quiesce and > unquiesce instead, which should be more safe. > > The only one usage will be removed in the following > congestion control patches. Hello Ming, The title of this patch is misleading since this patch does not touch the calls related to queue congestion (blk_mq_stop_hw_queue() and blk_mq_start_stopped_hw_queues()). I assume that you meant that this patch avoids that the xen-blkfront driver uses blk_mq_(start|stop)_hw_queues() (with queues in plural form)? Can you please reflect that in the subject of this and related patches? Additionally, it's probably a good idea that this is not just an interface change but that this kind of patches fix a (hard to trigger?) race condition. > static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo) > { > - if (!RING_FULL(&rinfo->ring)) > + if (!RING_FULL(&rinfo->ring)) { > blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true); > + blk_mq_kick_requeue_list(rinfo->dev_info->rq); > + } > } > > static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > @@ -1225,7 +1227,8 @@ static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > unsigned long flags; > > spin_lock_irqsave(&rinfo->ring_lock, flags); > - kick_pending_request_queues_locked(rinfo); > + if (!RING_FULL(&rinfo->ring)) > + blk_mq_run_hw_queues(rinfo->dev_info->rq, true); > spin_unlock_irqrestore(&rinfo->ring_lock, flags); > } Why do some kick_pending_request_queues_locked() kick the requeue list and why has the above kick_pending_request_queues_locked() call been converted into a blk_mq_run_hw_queues() call and thereby ignores the requeue list? Thanks, Bart.
On Wed, Jul 12, 2017 at 02:20:58AM +0800, Ming Lei wrote: > This interfaces will be removed soon, so use quiesce and > unquiesce instead, which should be more safe. > > The only one usage will be removed in the following > congestion control patches. Would it be better to simply fix blk_mq_{start/stop}_stopped_hw_queues rather than introducing a new interface? Roger.
On Tue, Jul 11, 2017 at 02:41:05PM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Jul 12, 2017 at 02:20:58AM +0800, Ming Lei wrote: > > This interfaces will be removed soon, so use quiesce and > > unquiesce instead, which should be more safe. > > 'should be'? That does not sound encouraging? Sorry for the mistake, and will fix it in V2, definitely quiesce is the preferred interface, also stop queue may not do what you want to do as you can see from comment of blk_mq_stop_hw_queues, and has caused much trouble already. And I appreciate if you guys may review on this patch itself. BTW, this patch should have been included in Sagi Grimberg's patchset of "[PATCH v3 0/8] correct quiescing in several block drivers": http://marc.info/?t=149927415900006&r=1&w=2 Thanks, Ming
On Tue, Jul 11, 2017 at 06:41:29PM +0000, Bart Van Assche wrote: > On Wed, 2017-07-12 at 02:20 +0800, Ming Lei wrote: > > This interfaces will be removed soon, so use quiesce and > > unquiesce instead, which should be more safe. > > > > The only one usage will be removed in the following > > congestion control patches. > > Hello Ming, > > The title of this patch is misleading since this patch does not touch the calls > related to queue congestion (blk_mq_stop_hw_queue() and > blk_mq_start_stopped_hw_queues()). I assume that you meant that this patch avoids > that the xen-blkfront driver uses blk_mq_(start|stop)_hw_queues() (with queues in > plural form)? Can you please reflect that in the subject of this and related > patches? OK, will do it in V2. > > Additionally, it's probably a good idea that this is not just an interface change > but that this kind of patches fix a (hard to trigger?) race condition. > > > static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo) > > { > > - if (!RING_FULL(&rinfo->ring)) > > + if (!RING_FULL(&rinfo->ring)) { > > blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true); > > + blk_mq_kick_requeue_list(rinfo->dev_info->rq); > > + } > > } > > > > static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > > @@ -1225,7 +1227,8 @@ static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > > unsigned long flags; > > > > spin_lock_irqsave(&rinfo->ring_lock, flags); > > - kick_pending_request_queues_locked(rinfo); > > + if (!RING_FULL(&rinfo->ring)) > > + blk_mq_run_hw_queues(rinfo->dev_info->rq, true); > > spin_unlock_irqrestore(&rinfo->ring_lock, flags); > > } > > Why do some kick_pending_request_queues_locked() kick the requeue list and why > has the above kick_pending_request_queues_locked() call been converted into a > blk_mq_run_hw_queues() call and thereby ignores the requeue list? kick_pending_request_queues_locked() is used in req completion path, which belongs to congestion control, so this patch doesn't touch kick_pending_request_queues_locked(), which will be switched to generic congestion control in patch 5. For kick_pending_request_queues(), this patch replaces blk_mq_start_stopped_hw_queues() with blk_mq_run_hw_queues() because run queue is often the real purpose of this function, especially in non-congestion control path. Thanks, Ming
On Tue, Jul 11, 2017 at 06:41:29PM +0000, Bart Van Assche wrote: > On Wed, 2017-07-12 at 02:20 +0800, Ming Lei wrote: > > This interfaces will be removed soon, so use quiesce and > > unquiesce instead, which should be more safe. > > > > The only one usage will be removed in the following > > congestion control patches. > > Hello Ming, > > The title of this patch is misleading since this patch does not touch the calls > related to queue congestion (blk_mq_stop_hw_queue() and > blk_mq_start_stopped_hw_queues()). I assume that you meant that this patch avoids > that the xen-blkfront driver uses blk_mq_(start|stop)_hw_queues() (with queues in > plural form)? Can you please reflect that in the subject of this and related > patches? > > Additionally, it's probably a good idea that this is not just an interface change > but that this kind of patches fix a (hard to trigger?) race condition. > > > static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo) > > { > > - if (!RING_FULL(&rinfo->ring)) > > + if (!RING_FULL(&rinfo->ring)) { > > blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true); > > + blk_mq_kick_requeue_list(rinfo->dev_info->rq); > > + } > > } > > > > static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > > @@ -1225,7 +1227,8 @@ static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) > > unsigned long flags; > > > > spin_lock_irqsave(&rinfo->ring_lock, flags); > > - kick_pending_request_queues_locked(rinfo); > > + if (!RING_FULL(&rinfo->ring)) > > + blk_mq_run_hw_queues(rinfo->dev_info->rq, true); > > spin_unlock_irqrestore(&rinfo->ring_lock, flags); > > } > > Why do some kick_pending_request_queues_locked() kick the requeue list and why > has the above kick_pending_request_queues_locked() call been converted into a > blk_mq_run_hw_queues() call and thereby ignores the requeue list? Looks I forget to reply the question about requeue list. Actually blk_mq_kick_requeue_list() is only needed run where the queue is restarted, so this patch moves it after blk_mq_start_stopped_hw_queues(). In other path, we don't stop queue anymore, so needn't to kick requeue list.
On Tue, Jul 11, 2017 at 11:24:44PM +0200, Roger Pau Monné wrote: > On Wed, Jul 12, 2017 at 02:20:58AM +0800, Ming Lei wrote: > > This interfaces will be removed soon, so use quiesce and > > unquiesce instead, which should be more safe. > > > > The only one usage will be removed in the following > > congestion control patches. > > Would it be better to simply fix blk_mq_{start/stop}_stopped_hw_queues > rather than introducing a new interface? No, we do not want to expose start/stop state to drivers any more, which has caused enough trouble already, so these APIs need to be removed, and quiesce/unquiesce is preferred way for this purpose, as you can see the work done by Sagi Grimberg: http://marc.info/?t=149927415900006&r=1&w=2 Thanks, Ming
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c index c852ed3c01d5..1578befda635 100644 --- a/drivers/block/xen-blkfront.c +++ b/drivers/block/xen-blkfront.c @@ -1187,7 +1187,7 @@ static void xlvbd_release_gendisk(struct blkfront_info *info) return; /* No more blkif_request(). */ - blk_mq_stop_hw_queues(info->rq); + blk_mq_quiesce_queue(info->rq); for (i = 0; i < info->nr_rings; i++) { struct blkfront_ring_info *rinfo = &info->rinfo[i]; @@ -1216,8 +1216,10 @@ static void xlvbd_release_gendisk(struct blkfront_info *info) /* Already hold rinfo->ring_lock. */ static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo) { - if (!RING_FULL(&rinfo->ring)) + if (!RING_FULL(&rinfo->ring)) { blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true); + blk_mq_kick_requeue_list(rinfo->dev_info->rq); + } } static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) @@ -1225,7 +1227,8 @@ static void kick_pending_request_queues(struct blkfront_ring_info *rinfo) unsigned long flags; spin_lock_irqsave(&rinfo->ring_lock, flags); - kick_pending_request_queues_locked(rinfo); + if (!RING_FULL(&rinfo->ring)) + blk_mq_run_hw_queues(rinfo->dev_info->rq, true); spin_unlock_irqrestore(&rinfo->ring_lock, flags); } @@ -1346,7 +1349,7 @@ static void blkif_free(struct blkfront_info *info, int suspend) BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED; /* No more blkif_request(). */ if (info->rq) - blk_mq_stop_hw_queues(info->rq); + blk_mq_quiesce_queue(info->rq); for (i = 0; i < info->nr_rings; i++) blkif_free_ring(&info->rinfo[i]); @@ -2018,22 +2021,13 @@ static int blkif_recover(struct blkfront_info *info) /* Now safe for us to use the shared ring */ info->connected = BLKIF_STATE_CONNECTED; - for (r_index = 0; r_index < info->nr_rings; r_index++) { - struct blkfront_ring_info *rinfo; - - rinfo = &info->rinfo[r_index]; - /* Kick any other new requests queued since we resumed */ - kick_pending_request_queues(rinfo); - } - list_for_each_entry_safe(req, n, &info->requests, queuelist) { /* Requeue pending requests (flush or discard) */ list_del_init(&req->queuelist); BUG_ON(req->nr_phys_segments > segs); blk_mq_requeue_request(req, false); } - blk_mq_start_stopped_hw_queues(info->rq, true); - blk_mq_kick_requeue_list(info->rq); + blk_mq_unquiesce_queue(info->rq); while ((bio = bio_list_pop(&info->bio_list)) != NULL) { /* Traverse the list of pending bios and re-queue them */
This interfaces will be removed soon, so use quiesce and unquiesce instead, which should be more safe. The only one usage will be removed in the following congestion control patches. Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: "Roger Pau Monné" <roger.pau@citrix.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: Juergen Gross <jgross@suse.com> Cc: xen-devel@lists.xenproject.org Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/block/xen-blkfront.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)