[PATCH 01/13] dm-mpath: Split activate_path()

[PATCH 01/13] dm-mpath: Split activate_path()

[Bart Van Assche]

This patch does not change any functionality but makes the next
patch in this series easier to read.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: <stable@vger.kernel.org>
---
 drivers/md/dm-mpath.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 7f223dbed49f..909098e18643 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -111,7 +111,8 @@ typedef int (*action_fn) (struct pgpath *pgpath);
 
 static struct workqueue_struct *kmultipathd, *kmpath_handlerd;
 static void trigger_event(struct work_struct *work);
-static void activate_path(struct work_struct *work);
+static void activate_path(struct pgpath *pgpath);
+static void activate_path_work(struct work_struct *work);
 static void process_queued_bios(struct work_struct *work);
 
 /*-----------------------------------------------
@@ -136,7 +137,7 @@ static struct pgpath *alloc_pgpath(void)
 
 	if (pgpath) {
 		pgpath->is_active = true;
-		INIT_DELAYED_WORK(&pgpath->activate_path, activate_path);
+		INIT_DELAYED_WORK(&pgpath->activate_path, activate_path_work);
 	}
 
 	return pgpath;
@@ -1437,10 +1438,8 @@ static void pg_init_done(void *data, int errors)
 	spin_unlock_irqrestore(&m->lock, flags);
 }
 
-static void activate_path(struct work_struct *work)
+static void activate_path(struct pgpath *pgpath)
 {
-	struct pgpath *pgpath =
-		container_of(work, struct pgpath, activate_path.work);
 	struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);
 
 	if (pgpath->is_active && !blk_queue_dying(q))
@@ -1449,6 +1448,11 @@ static void activate_path(struct work_struct *work)
 		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
 }
 
+static void activate_path_work(struct work_struct *work)
+{
+	activate_path(container_of(work, struct pgpath, activate_path.work));
+}
+
 static int noretry_error(int error)
 {
 	switch (error) {
-- 
2.12.2

[PATCH 02/13] dm-mpath: Avoid that path removal can trigger an infinite loop

[Bart Van Assche]

If blk_get_request() fails check whether the failure is due to
a path being removed. If that is the case fail the path by
triggering a call to fail_path(). This patch avoids that the
following scenario can be encountered while removing paths:
* CPU usage of a kworker thread jumps to 100%.
* Removing the dm device becomes impossible.

Delay requeueing if blk_get_request() returns -EBUSY or
-EWOULDBLOCK because in these cases immediate requeuing is
inappropriate.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: <stable@vger.kernel.org>
---
 drivers/md/dm-mpath.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 909098e18643..6d4333fdddf5 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -490,6 +490,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	struct pgpath *pgpath;
 	struct block_device *bdev;
 	struct dm_mpath_io *mpio = get_mpio(map_context);
+	struct request_queue *q;
 	struct request *clone;
 
 	/* Do we need to select a new pgpath? */
@@ -512,13 +513,19 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 	mpio->nr_bytes = nr_bytes;
 
 	bdev = pgpath->path.dev->bdev;
-
-	clone = blk_get_request(bdev_get_queue(bdev),
-			rq->cmd_flags | REQ_NOMERGE,
-			GFP_ATOMIC);
+	q = bdev_get_queue(bdev);
+	clone = blk_get_request(q, rq->cmd_flags | REQ_NOMERGE, GFP_ATOMIC);
 	if (IS_ERR(clone)) {
 		/* EBUSY, ENODEV or EWOULDBLOCK: requeue */
-		return r;
+		pr_debug("blk_get_request() returned %ld%s - requeuing\n",
+			 PTR_ERR(clone), blk_queue_dying(q) ?
+			 " (path offline)" : "");
+		if (blk_queue_dying(q)) {
+			atomic_inc(&m->pg_init_in_progress);
+			activate_path(pgpath);
+			return DM_MAPIO_REQUEUE;
+		}
+		return DM_MAPIO_DELAY_REQUEUE;
 	}
 	clone->bio = clone->biotail = NULL;
 	clone->rq_disk = bdev->bd_disk;
-- 
2.12.2

[PATCH 03/13] dm-mpath: Delay requeuing while path initialization is in progress

[Bart Van Assche]

Requeuing a request immediately while path initialization is ongoing
causes high CPU usage, something that is undesired. Hence delay
requeuing while path initialization is in progress.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: <stable@vger.kernel.org>
---
 drivers/md/dm-mpath.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 6d4333fdddf5..e38c92178746 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -322,13 +322,16 @@ static int __pg_init_all_paths(struct multipath *m)
 	return atomic_read(&m->pg_init_in_progress);
 }
 
-static void pg_init_all_paths(struct multipath *m)
+static int pg_init_all_paths(struct multipath *m)
 {
 	unsigned long flags;
+	int ret;
 
 	spin_lock_irqsave(&m->lock, flags);
-	__pg_init_all_paths(m);
+	ret = __pg_init_all_paths(m);
 	spin_unlock_irqrestore(&m->lock, flags);
+
+	return ret;
 }
 
 static void __switch_pg(struct multipath *m, struct priority_group *pg)
@@ -485,7 +488,6 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 				   struct request **__clone)
 {
 	struct multipath *m = ti->private;
-	int r = DM_MAPIO_REQUEUE;
 	size_t nr_bytes = blk_rq_bytes(rq);
 	struct pgpath *pgpath;
 	struct block_device *bdev;
@@ -504,8 +506,8 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
 		return -EIO;	/* Failed */
 	} else if (test_bit(MPATHF_QUEUE_IO, &m->flags) ||
 		   test_bit(MPATHF_PG_INIT_REQUIRED, &m->flags)) {
-		pg_init_all_paths(m);
-		return r;
+		return pg_init_all_paths(m) == 0 ? DM_MAPIO_REQUEUE :
+			DM_MAPIO_DELAY_REQUEUE;
 	}
 
 	memset(mpio, 0, sizeof(*mpio));
-- 
2.12.2

Re: [dm-devel] [PATCH 02/13] dm-mpath: Avoid that path removal can trigger an infinite loop

[Hannes Reinecke]

On 04/26/2017 08:37 PM, Bart Van Assche wrote:
> If blk_get_request() fails check whether the failure is due to
> a path being removed. If that is the case fail the path by
> triggering a call to fail_path(). This patch avoids that the
> following scenario can be encountered while removing paths:
> * CPU usage of a kworker thread jumps to 100%.
> * Removing the dm device becomes impossible.
> 
> Delay requeueing if blk_get_request() returns -EBUSY or
> -EWOULDBLOCK because in these cases immediate requeuing is
> inappropriate.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/md/dm-mpath.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 909098e18643..6d4333fdddf5 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -490,6 +490,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
>  	struct pgpath *pgpath;
>  	struct block_device *bdev;
>  	struct dm_mpath_io *mpio = get_mpio(map_context);
> +	struct request_queue *q;
>  	struct request *clone;
>  
>  	/* Do we need to select a new pgpath? */
> @@ -512,13 +513,19 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
>  	mpio->nr_bytes = nr_bytes;
>  
>  	bdev = pgpath->path.dev->bdev;
> -
> -	clone = blk_get_request(bdev_get_queue(bdev),
> -			rq->cmd_flags | REQ_NOMERGE,
> -			GFP_ATOMIC);
> +	q = bdev_get_queue(bdev);
> +	clone = blk_get_request(q, rq->cmd_flags | REQ_NOMERGE, GFP_ATOMIC);
>  	if (IS_ERR(clone)) {
>  		/* EBUSY, ENODEV or EWOULDBLOCK: requeue */
> -		return r;
> +		pr_debug("blk_get_request() returned %ld%s - requeuing\n",
> +			 PTR_ERR(clone), blk_queue_dying(q) ?
> +			 " (path offline)" : "");
> +		if (blk_queue_dying(q)) {
> +			atomic_inc(&m->pg_init_in_progress);
> +			activate_path(pgpath);
> +			return DM_MAPIO_REQUEUE;
> +		}
> +		return DM_MAPIO_DELAY_REQUEUE;
>  	}
>  	clone->bio = clone->biotail = NULL;
>  	clone->rq_disk = bdev->bd_disk;
> 
At the very least this does warrant some inline comments.
Why do we call activate_path() here, seeing that the queue is dying?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [dm-devel] [PATCH 03/13] dm-mpath: Delay requeuing while path initialization is in progress

[Hannes Reinecke]

On 04/26/2017 08:37 PM, Bart Van Assche wrote:
> Requeuing a request immediately while path initialization is ongoing
> causes high CPU usage, something that is undesired. Hence delay
> requeuing while path initialization is in progress.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/md/dm-mpath.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 6d4333fdddf5..e38c92178746 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -322,13 +322,16 @@ static int __pg_init_all_paths(struct multipath *m)
>  	return atomic_read(&m->pg_init_in_progress);
>  }
>  
> -static void pg_init_all_paths(struct multipath *m)
> +static int pg_init_all_paths(struct multipath *m)
>  {
>  	unsigned long flags;
> +	int ret;
>  
>  	spin_lock_irqsave(&m->lock, flags);
> -	__pg_init_all_paths(m);
> +	ret = __pg_init_all_paths(m);
>  	spin_unlock_irqrestore(&m->lock, flags);
> +
> +	return ret;
>  }
>  
>  static void __switch_pg(struct multipath *m, struct priority_group *pg)
> @@ -485,7 +488,6 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
>  				   struct request **__clone)
>  {
>  	struct multipath *m = ti->private;
> -	int r = DM_MAPIO_REQUEUE;
>  	size_t nr_bytes = blk_rq_bytes(rq);
>  	struct pgpath *pgpath;
>  	struct block_device *bdev;
> @@ -504,8 +506,8 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq,
>  		return -EIO;	/* Failed */
>  	} else if (test_bit(MPATHF_QUEUE_IO, &m->flags) ||
>  		   test_bit(MPATHF_PG_INIT_REQUIRED, &m->flags)) {
> -		pg_init_all_paths(m);
> -		return r;
> +		return pg_init_all_paths(m) == 0 ? DM_MAPIO_REQUEUE :
> +			DM_MAPIO_DELAY_REQUEUE;
>  	}
>  
>  	memset(mpio, 0, sizeof(*mpio));
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [dm-devel] [PATCH 02/13] dm-mpath: Avoid that path removal can trigger an infinite loop

[Bart Van Assche]

On Thu, 2017-04-27 at 07:46 +0200, Hannes Reinecke wrote:
> On 04/26/2017 08:37 PM, Bart Van Assche wrote:
> > +	clone = blk_get_request(q, rq->cmd_flags | REQ_NOMERGE, GFP_ATOMIC);
> >  	if (IS_ERR(clone)) {
> >  		/* EBUSY, ENODEV or EWOULDBLOCK: requeue */
> > -		return r;
> > +		pr_debug("blk_get_request() returned %ld%s - requeuing\n",
> > +			 PTR_ERR(clone), blk_queue_dying(q) ?
> > +			 " (path offline)" : "");
> > +		if (blk_queue_dying(q)) {
> > +			atomic_inc(&m->pg_init_in_progress);
> > +			activate_path(pgpath);
> > +			return DM_MAPIO_REQUEUE;
> > +		}
> > +		return DM_MAPIO_DELAY_REQUEUE;
> >  	}
> >  	clone->bio = clone->biotail = NULL;
> >  	clone->rq_disk = bdev->bd_disk;
> 
> At the very least this does warrant some inline comments.
> Why do we call activate_path() here, seeing that the queue is dying?

Hello Hannes,

activate_path() is not only able to activate a path but can also change
the state of a path to offline. The body of the activate_path() function
makes that clear and that is why I had not added a comment above the
activate_path() call:

static void activate_path(struct pgpath *pgpath)
{
	struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);

	if (pgpath->is_active && !blk_queue_dying(q))
		scsi_dh_activate(q, pg_init_done, pgpath);
	else
		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
}

Bart.

Re: [dm-devel] [PATCH 02/13] dm-mpath: Avoid that path removal can trigger an infinite loop

[Hannes Reinecke]

On 04/27/2017 05:11 PM, Bart Van Assche wrote:
> On Thu, 2017-04-27 at 07:46 +0200, Hannes Reinecke wrote:
>> On 04/26/2017 08:37 PM, Bart Van Assche wrote:
>>> +	clone = blk_get_request(q, rq->cmd_flags | REQ_NOMERGE, GFP_ATOMIC);
>>>  	if (IS_ERR(clone)) {
>>>  		/* EBUSY, ENODEV or EWOULDBLOCK: requeue */
>>> -		return r;
>>> +		pr_debug("blk_get_request() returned %ld%s - requeuing\n",
>>> +			 PTR_ERR(clone), blk_queue_dying(q) ?
>>> +			 " (path offline)" : "");
>>> +		if (blk_queue_dying(q)) {
>>> +			atomic_inc(&m->pg_init_in_progress);
>>> +			activate_path(pgpath);
>>> +			return DM_MAPIO_REQUEUE;
>>> +		}
>>> +		return DM_MAPIO_DELAY_REQUEUE;
>>>  	}
>>>  	clone->bio = clone->biotail = NULL;
>>>  	clone->rq_disk = bdev->bd_disk;
>>
>> At the very least this does warrant some inline comments.
>> Why do we call activate_path() here, seeing that the queue is dying?
> 
> Hello Hannes,
> 
> activate_path() is not only able to activate a path but can also change
> the state of a path to offline. The body of the activate_path() function
> makes that clear and that is why I had not added a comment above the
> activate_path() call:
> 
> static void activate_path(struct pgpath *pgpath)
> {
> 	struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);
> 
> 	if (pgpath->is_active && !blk_queue_dying(q))
> 		scsi_dh_activate(q, pg_init_done, pgpath);
> 	else
> 		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
> }
> 
So why not call 'pg_init_done()' directly and avoid the confusion?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N�rnberg
GF: F. Imend�rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N�rnberg)

Re: [PATCH 02/13] dm-mpath: Avoid that path removal can trigger an infinite loop

[Mike Snitzer]

On Thu, Apr 27 2017 at 11:13am -0400,
Hannes Reinecke <hare@suse.de> wrote:

> On 04/27/2017 05:11 PM, Bart Van Assche wrote:
> > On Thu, 2017-04-27 at 07:46 +0200, Hannes Reinecke wrote:
> >> On 04/26/2017 08:37 PM, Bart Van Assche wrote:
> >>> +	clone = blk_get_request(q, rq->cmd_flags | REQ_NOMERGE, GFP_ATOMIC);
> >>>  	if (IS_ERR(clone)) {
> >>>  		/* EBUSY, ENODEV or EWOULDBLOCK: requeue */
> >>> -		return r;
> >>> +		pr_debug("blk_get_request() returned %ld%s - requeuing\n",
> >>> +			 PTR_ERR(clone), blk_queue_dying(q) ?
> >>> +			 " (path offline)" : "");
> >>> +		if (blk_queue_dying(q)) {
> >>> +			atomic_inc(&m->pg_init_in_progress);
> >>> +			activate_path(pgpath);
> >>> +			return DM_MAPIO_REQUEUE;
> >>> +		}
> >>> +		return DM_MAPIO_DELAY_REQUEUE;
> >>>  	}
> >>>  	clone->bio = clone->biotail = NULL;
> >>>  	clone->rq_disk = bdev->bd_disk;
> >>
> >> At the very least this does warrant some inline comments.
> >> Why do we call activate_path() here, seeing that the queue is dying?
> > 
> > Hello Hannes,
> > 
> > activate_path() is not only able to activate a path but can also change
> > the state of a path to offline. The body of the activate_path() function
> > makes that clear and that is why I had not added a comment above the
> > activate_path() call:
> > 
> > static void activate_path(struct pgpath *pgpath)
> > {
> > 	struct request_queue *q = bdev_get_queue(pgpath->path.dev->bdev);
> > 
> > 	if (pgpath->is_active && !blk_queue_dying(q))
> > 		scsi_dh_activate(q, pg_init_done, pgpath);
> > 	else
> > 		pg_init_done(pgpath, SCSI_DH_DEV_OFFLINED);
> > }
> > 
> So why not call 'pg_init_done()' directly and avoid the confusion?

Doing so is sprinkling more SCSI specific droppings in code that should
be increasingly transport agnostic.  Might be worth renaming
activate_path() to activate_or_offline_path() ?

Mike