[PATCH 1/2] dm-mpath: Enable hw_handler_params to take effect if hw_handler is the same between new and old

[PATCH 1/2] dm-mpath: Enable hw_handler_params to take effect if hw_handler is the same between new and old

[tang.junhui]

From: "tang.junhui" <tang.junhui@zte.com.cn>

If the hardware handle which a device has already attached is the same
with m->hw_handler_name, m->hw_handler_params should not be ignored, but
be enabled to take effect.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
 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 0caab4b..b1aba63 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -849,19 +849,23 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
 retain:
 		attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
 		if (attached_handler_name) {
+			/* clear any hw_handler_params associated with
+			 * m->hw_handler_params if a different handler has
+			 * already been attached.
+			 */
+			if(!m->hw_handler_name || strcmp(attached_handler_name, m->hw_handler_name))
+			{
+				kfree(m->hw_handler_params);
+				m->hw_handler_params = NULL;
+			}
 			/*
 			 * Reset hw_handler_name to match the attached handler
-			 * and clear any hw_handler_params associated with the
-			 * ignored handler.
 			 *
 			 * NB. This modifies the table line to show the actual
 			 * handler instead of the original table passed in.
 			 */
 			kfree(m->hw_handler_name);
 			m->hw_handler_name = attached_handler_name;
-
-			kfree(m->hw_handler_params);
-			m->hw_handler_params = NULL;
 		}
 	}
 
-- 
2.8.1.windows.1

[PATCH 2/2] dm-mpath: Remove useless retain_attached_hw_handler parameter

[tang.junhui]

From: "tang.junhui" <tang.junhui@zte.com.cn>

Hardware handle would be retained no matter parameter
retain_attached_hw_handler is set or not in the logic
of current code. So remove this useless parameter.

Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>
---
 drivers/md/dm-mpath.c | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index b1aba63..fe6a529 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -129,10 +129,9 @@ static void process_queued_bios(struct work_struct *work);
 #define MPATHF_QUEUE_IO 0			/* Must we queue all I/O? */
 #define MPATHF_QUEUE_IF_NO_PATH 1		/* Queue I/O if last path fails? */
 #define MPATHF_SAVED_QUEUE_IF_NO_PATH 2		/* Saved state during suspension */
-#define MPATHF_RETAIN_ATTACHED_HW_HANDLER 3	/* If there's already a hw_handler present, don't change it. */
-#define MPATHF_PG_INIT_DISABLED 4		/* pg_init is not currently allowed */
-#define MPATHF_PG_INIT_REQUIRED 5		/* pg_init needs calling? */
-#define MPATHF_PG_INIT_DELAY_RETRY 6		/* Delay pg_init retry? */
+#define MPATHF_PG_INIT_DISABLED 3		/* pg_init is not currently allowed */
+#define MPATHF_PG_INIT_REQUIRED 4		/* pg_init needs calling? */
+#define MPATHF_PG_INIT_DELAY_RETRY 5		/* Delay pg_init retry? */
 
 /*-----------------------------------------------
  * Allocation routines
@@ -240,11 +239,6 @@ static int alloc_multipath_stage2(struct dm_target *ti, struct multipath *m)
 	}
 	else if (m->queue_mode == DM_TYPE_BIO_BASED) {
 		INIT_WORK(&m->process_queued_bios, process_queued_bios);
-		/*
-		 * bio-based doesn't support any direct scsi_dh management;
-		 * it just discovers if a scsi_dh is attached.
-		 */
-		set_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags);
 	}
 
 	dm_table_set_type(ti->table, m->queue_mode);
@@ -842,11 +836,8 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
 		goto bad;
 	}
 
-	if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags) || m->hw_handler_name)
+	if (m->hw_handler_name) {
 		q = bdev_get_queue(p->path.dev->bdev);
-
-	if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags)) {
-retain:
 		attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
 		if (attached_handler_name) {
 			/* clear any hw_handler_params associated with
@@ -871,13 +862,6 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
 
 	if (m->hw_handler_name) {
 		r = scsi_dh_attach(q, m->hw_handler_name);
-		if (r == -EBUSY) {
-			char b[BDEVNAME_SIZE];
-
-			printk(KERN_INFO "dm-mpath: retaining handler on device %s\n",
-				bdevname(p->path.dev->bdev, b));
-			goto retain;
-		}
 		if (r < 0) {
 			ti->error = "error attaching hardware handler";
 			dm_put_device(ti, p->path.dev);
@@ -1040,7 +1024,7 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m)
 	const char *arg_name;
 
 	static struct dm_arg _args[] = {
-		{0, 8, "invalid number of feature args"},
+		{0, 7, "invalid number of feature args"},
 		{1, 50, "pg_init_retries must be between 1 and 50"},
 		{0, 60000, "pg_init_delay_msecs must be between 0 and 60000"},
 	};
@@ -1061,11 +1045,6 @@ static int parse_features(struct dm_arg_set *as, struct multipath *m)
 			continue;
 		}
 
-		if (!strcasecmp(arg_name, "retain_attached_hw_handler")) {
-			set_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags);
-			continue;
-		}
-
 		if (!strcasecmp(arg_name, "pg_init_retries") &&
 		    (argc >= 1)) {
 			r = dm_read_arg(_args + 1, as, &m->pg_init_retries, &ti->error);
@@ -1745,7 +1724,6 @@ static void multipath_status(struct dm_target *ti, status_type_t type,
 		DMEMIT("%u ", test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags) +
 			      (m->pg_init_retries > 0) * 2 +
 			      (m->pg_init_delay_msecs != DM_PG_INIT_DELAY_DEFAULT) * 2 +
-			      test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags) +
 			      (m->queue_mode != DM_TYPE_REQUEST_BASED) * 2);
 
 		if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags))
@@ -1754,8 +1732,6 @@ static void multipath_status(struct dm_target *ti, status_type_t type,
 			DMEMIT("pg_init_retries %u ", m->pg_init_retries);
 		if (m->pg_init_delay_msecs != DM_PG_INIT_DELAY_DEFAULT)
 			DMEMIT("pg_init_delay_msecs %u ", m->pg_init_delay_msecs);
-		if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags))
-			DMEMIT("retain_attached_hw_handler ");
 		if (m->queue_mode != DM_TYPE_REQUEST_BASED) {
 			switch(m->queue_mode) {
 			case DM_TYPE_BIO_BASED:
-- 
2.8.1.windows.1

Re: [PATCH 2/2] dm-mpath: Remove useless retain_attached_hw_handler parameter

[Mike Snitzer]

On Thu, Nov 24 2016 at  2:11am -0500,
tang.junhui@zte.com.cn <tang.junhui@zte.com.cn> wrote:

> From: "tang.junhui" <tang.junhui@zte.com.cn>
> 
> Hardware handle would be retained no matter parameter
> retain_attached_hw_handler is set or not in the logic
> of current code. So remove this useless parameter.

Right, that wasn't always the case.  Previously (before commit )
dm-mpath would first detach the attached handler.

I'm not completely opposed to removing the code that checks
MPATHF_RETAIN_ATTACHED_HW_HANDLER in parse_path() but your proposed
patch is broken in 2 ways:
1) in parse_path() you need to always initialize q
2) setting m->hw_handler_name to attached_handler_name needs to still
   happen regardless of whether m->hw_handler_name was previously set
3) the "retain_attached_hw_handler" feature should still be allowed on
   the command line, no sense to break multipath-tools

But honestly, I'm not seeing any reason to not just leave the existing
code.

Re: [PATCH 2/2] dm-mpath: Remove useless retain_attached_hw_handler parameter

[Mike Snitzer]

On Mon, Nov 28 2016 at  4:39pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Nov 24 2016 at  2:11am -0500,
> tang.junhui@zte.com.cn <tang.junhui@zte.com.cn> wrote:
> 
> > From: "tang.junhui" <tang.junhui@zte.com.cn>
> > 
> > Hardware handle would be retained no matter parameter
> > retain_attached_hw_handler is set or not in the logic
> > of current code. So remove this useless parameter.
> 
> Right, that wasn't always the case.  Previously (before commit )
> dm-mpath would first detach the attached handler.
> 
> I'm not completely opposed to removing the code that checks
> MPATHF_RETAIN_ATTACHED_HW_HANDLER in parse_path() but your proposed
> patch is broken in 2 ways:
> 1) in parse_path() you need to always initialize q
> 2) setting m->hw_handler_name to attached_handler_name needs to still
>    happen regardless of whether m->hw_handler_name was previously set
> 3) the "retain_attached_hw_handler" feature should still be allowed on
>    the command line, no sense to break multipath-tools
> 
> But honestly, I'm not seeing any reason to not just leave the existing
> code.

In addition, your patch actually breaks the ability to cope with -EBUSY
from the call to scsi_dh_attach().  Meaning we wouldn't properly fall
back to retaining the attached hw handler.

So NACK on this patch.

Re: [PATCH 2/2] dm-mpath: Remove useless retain_attached_hw_handler parameter

[Mike Snitzer]

On Mon, Nov 28 2016 at  4:39pm -0500,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Thu, Nov 24 2016 at  2:11am -0500,
> tang.junhui@zte.com.cn <tang.junhui@zte.com.cn> wrote:
> 
> > From: "tang.junhui" <tang.junhui@zte.com.cn>
> > 
> > Hardware handle would be retained no matter parameter
> > retain_attached_hw_handler is set or not in the logic
> > of current code. So remove this useless parameter.
> 
> Right, that wasn't always the case.  Previously (before commit )
> dm-mpath would first detach the attached handler.

meant to reference commit 1bab0de02 ("dm-mpath, scsi_dh: don't let dm
detach device handlers")

Re: [PATCH 2/2] dm-mpath: Remove useless retain_attached_hw_handler parameter

[tang.junhui]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1: Type: text/plain; charset="GB2312", Size: 2805 bytes --]

Hello Mike:

Thanks for your respond.

> 1) in parse_path() you need to always initialize q
the condition of q initialization is 
if (m->retain_attached_hw_handler)¡±
comparing with previous condition
if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags) || 
m->hw_handler_name)
it does not increase the possibility of q initialization.

> 2) setting m->hw_handler_name to attached_handler_name needs to still
   > happen regardless of whether m->hw_handler_name was previously set
The patch does not change the logic above, does it? 

> 3) the "retain_attached_hw_handler" feature should still be allowed on
   > the command line, no sense to break multipath-tools
Yes, actually I am still preparing two other patches now, 
one patch modifies parse_features() to ignore unsupported 
parameter and do not return failure. Another patch modifies 
multipath to remove this parameter also.

> In addition, your patch actually breaks the ability to cope with ¨CEBUSY
> from the call to scsi_dh_attach().  Meaning we wouldn't properly fall
> back to retaining the attached hw handler.
scsi_dh_attach() return ¨CEBUSY only if that device has attached 
a different handle, after previous processing, the handle would 
be same, so it is impossible to return ¨CEBUSY in scsi_dh_attach(),
we do not need to process it.

Regards,
Tang





·¢¼þÈË:         Mike Snitzer <snitzer@redhat.com>
ÊÕ¼þÈË:         tang.junhui@zte.com.cn, 
³­ËÍ:   zhang.kai16@zte.com.cn, dm-devel@redhat.com, agk@redhat.com
ÈÕÆÚ:   2016/11/29 05:52
Ö÷Ìâ:   Re: [dm-devel] [PATCH 2/2] dm-mpath: Remove useless 
retain_attached_hw_handler parameter
·¢¼þÈË: dm-devel-bounces@redhat.com



On Thu, Nov 24 2016 at  2:11am -0500,
tang.junhui@zte.com.cn <tang.junhui@zte.com.cn> wrote:

> From: "tang.junhui" <tang.junhui@zte.com.cn>
> 
> Hardware handle would be retained no matter parameter
> retain_attached_hw_handler is set or not in the logic
> of current code. So remove this useless parameter.

Right, that wasn't always the case.  Previously (before commit )
dm-mpath would first detach the attached handler.

I'm not completely opposed to removing the code that checks
MPATHF_RETAIN_ATTACHED_HW_HANDLER in parse_path() but your proposed
patch is broken in 2 ways:
1) in parse_path() you need to always initialize q
2) setting m->hw_handler_name to attached_handler_name needs to still
   happen regardless of whether m->hw_handler_name was previously set
3) the "retain_attached_hw_handler" feature should still be allowed on
   the command line, no sense to break multipath-tools

But honestly, I'm not seeing any reason to not just leave the existing
code.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel



[-- Attachment #1.2: Type: text/html, Size: 4694 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] dm-mpath: Enable hw_handler_params to take effect if hw_handler is the same between new and old

[Mike Snitzer]

On Thu, Nov 24 2016 at  2:11am -0500,
tang.junhui@zte.com.cn <tang.junhui@zte.com.cn> wrote:

> From: "tang.junhui" <tang.junhui@zte.com.cn>
> 
> If the hardware handle which a device has already attached is the same
> with m->hw_handler_name, m->hw_handler_params should not be ignored, but
> be enabled to take effect.
> 
> Signed-off-by: tang.junhui <tang.junhui@zte.com.cn>

I tweaked this a little bit and staged it for 4.10, see:
https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.10&id=1422b33eda2ffbc70eb11163bf7ac1e59ebba222