Re: [Xen-devel] [PATCH 1/6] xen-blkfront: avoid to use start/stop queue

2017-07-11 Thread Ming Lei 
On Tue, Jul 11, 2017 at 06:41:29PM +, 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(>ring))
> > +   if (!RING_FULL(>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(>ring_lock, flags);
> > -   kick_pending_request_queues_locked(rinfo);
> > +   if (!RING_FULL(>ring))
> > +   blk_mq_run_hw_queues(rinfo->dev_info->rq, true);
> > spin_unlock_irqrestore(>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.

-- 
Ming

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH 1/6] xen-blkfront: avoid to use start/stop queue

2017-07-11 Thread Ming Lei 
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 
Cc: "Roger Pau Monné" 
Cc: Boris Ostrovsky 
Cc: Juergen Gross 
Cc: xen-de...@lists.xenproject.org
Signed-off-by: Ming Lei 
---
 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 = >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(>ring))
+   if (!RING_FULL(>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(>ring_lock, flags);
-   kick_pending_request_queues_locked(rinfo);
+   if (!RING_FULL(>ring))
+   blk_mq_run_hw_queues(rinfo->dev_info->rq, true);
spin_unlock_irqrestore(>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(>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 = >rinfo[r_index];
-   /* Kick any other new requests queued since we resumed */
-   kick_pending_request_queues(rinfo);
-   }
-
list_for_each_entry_safe(req, n, >requests, queuelist) {
/* Requeue pending requests (flush or discard) */
list_del_init(>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(>bio_list)) != NULL) {
/* Traverse the list of pending bios and re-queue them */
-- 
2.9.4


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/6] xen-blkfront: avoid to use start/stop queue

2017-07-11 Thread Bart Van Assche 
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(>ring))
> + if (!RING_FULL(>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(>ring_lock, flags);
> - kick_pending_request_queues_locked(rinfo);
> + if (!RING_FULL(>ring))
> + blk_mq_run_hw_queues(rinfo->dev_info->rq, true);
>   spin_unlock_irqrestore(>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.
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/6] xen-blkfront: avoid to use start/stop queue

2017-07-11 Thread Ming Lei 
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=14992741596=1=2

Thanks,
Ming

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/6] xen-blkfront: avoid to use start/stop queue

2017-07-11 Thread Ming Lei 
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=14992741596=1=2


Thanks,
Ming

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/6] xen-blkfront: avoid to use start/stop queue

2017-07-11 Thread Ming Lei 
On Tue, Jul 11, 2017 at 06:41:29PM +, 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(>ring))
> > +   if (!RING_FULL(>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(>ring_lock, flags);
> > -   kick_pending_request_queues_locked(rinfo);
> > +   if (!RING_FULL(>ring))
> > +   blk_mq_run_hw_queues(rinfo->dev_info->rq, true);
> > spin_unlock_irqrestore(>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

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/6] xen-blkfront: avoid to use start/stop queue

2017-07-11 Thread Roger Pau Monné 
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.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 1/6] xen-blkfront: avoid to use start/stop queue

2017-07-11 Thread Konrad Rzeszutek Wilk 
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 
> Cc: "Roger Pau Monné" 
> Cc: Boris Ostrovsky 
> Cc: Juergen Gross 
> Cc: xen-de...@lists.xenproject.org
> Signed-off-by: Ming Lei 
> ---
>  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 = >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(>ring))
> + if (!RING_FULL(>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(>ring_lock, flags);
> - kick_pending_request_queues_locked(rinfo);
> + if (!RING_FULL(>ring))
> + blk_mq_run_hw_queues(rinfo->dev_info->rq, true);
>   spin_unlock_irqrestore(>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(>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 = >rinfo[r_index];
> - /* Kick any other new requests queued since we resumed */
> - kick_pending_request_queues(rinfo);
> - }
> -
>   list_for_each_entry_safe(req, n, >requests, queuelist) {
>   /* Requeue pending requests (flush or discard) */
>   list_del_init(>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(>bio_list)) != NULL) {
>   /* Traverse the list of pending bios and re-queue them */
> -- 
> 2.9.4
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel