[PATCH] RFC: have dm-mpath use already attached scsi_dh

[PATCH] RFC: have dm-mpath use already attached scsi_dh

[michaelc]

From: Mike Christie <michaelc@cs.wisc.edu>

If you have a mixed environment of clarriions, where some
support ALAU and some support PNR, what do you put in
your multipath.conf? With this patch you do not have to worry about
it. If those modules are loaded before dm-mpath, then they
will have attached to the correct devices based on inquiry, alua commands
and parsing of data buffers (for example in scsi_dh_emc's alua check).
There is no need for the user to set that info in the multipath.conf.
And in general since all scsi_dh_modules will attach to the devices
they work for, we do not need to have users specific this.

I kept the code to try and use what the user passed in for compat
reasons (in case scsi_dh_* was not loaded), but if the scsi
layer has already attached we use that.

I have not tested the patch. It is only compile tested.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/md/dm-mpath.c                 |   22 +++++++++++++++++++---
 drivers/scsi/device_handler/scsi_dh.c |   22 ++++++++++++++++++++++
 include/scsi/scsi_dh.h                |    6 ++++++
 3 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 095f77b..bae7b77 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -571,6 +571,8 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
 			       struct dm_target *ti)
 {
 	int r;
+	char *hw_handler_name;
+	struct request_queue *q;
 	struct pgpath *p;
 	struct multipath *m = ti->private;
 
@@ -591,9 +593,23 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
 		goto bad;
 	}
 
-	if (m->hw_handler_name) {
-		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
-				   m->hw_handler_name);
+
+	q = bdev_get_queue(p->path.dev->bdev);
+	hw_handler_name = scsi_dh_get_attached_name(q);
+	if (hw_handler_name) {
+		if (!m->hw_handler_name)
+			m->hw_handler_name = hw_handler_name;
+		else if (strcmp(hw_handler_name, m->hw_handler_name)) {
+			/*
+			 * if there is a mismatch, then assume
+			 * the scsi layer knew best.
+			 */
+			kfree(m->hw_handler_name);
+			m->hw_handler_name = hw_handler_name;
+		} else
+			kfree(hw_handler_name);
+	} else if (m->hw_handler_name) {
+		r = scsi_dh_attach(q, m->hw_handler_name);
 		if (r < 0) {
 			dm_put_device(ti, p->path.dev);
 			goto bad;
diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
index a518f2e..257327f 100644
--- a/drivers/scsi/device_handler/scsi_dh.c
+++ b/drivers/scsi/device_handler/scsi_dh.c
@@ -481,6 +481,28 @@ int scsi_dh_attach(struct request_queue *q, const char *name)
 }
 EXPORT_SYMBOL_GPL(scsi_dh_attach);
 
+/**
+ * scsi_dh_get_attached_name
+ * @q: request queue to test
+ *
+ * Return name of scsi_dh module if attached. Caller must
+ * free returned string.
+ */
+char *scsi_dh_get_attached_name(struct request_queue *q)
+{
+	struct scsi_device *sdev;
+	unsigned long flags;
+	char *attached = NULL;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	sdev = q->queuedata;
+	if (sdev && sdev->scsi_dh_data)
+		attached = kstrdup(sdev->scsi_dh_data->scsi_dh->name,
+				   GFP_ATOMIC);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+	return attached;
+}
+
 /*
  * scsi_dh_handler_detach - Detach device handler
  * @sdev - sdev the handler should be detached from
diff --git a/include/scsi/scsi_dh.h b/include/scsi/scsi_dh.h
index 33efce2..9373702 100644
--- a/include/scsi/scsi_dh.h
+++ b/include/scsi/scsi_dh.h
@@ -60,6 +60,7 @@ extern int scsi_dh_activate(struct request_queue *);
 extern int scsi_dh_handler_exist(const char *);
 extern int scsi_dh_attach(struct request_queue *, const char *);
 extern void scsi_dh_detach(struct request_queue *);
+extern char *scsi_dh_get_attached_name(struct request_queue *);
 #else
 static inline int scsi_dh_activate(struct request_queue *req)
 {
@@ -77,4 +78,9 @@ static inline void scsi_dh_detach(struct request_queue *q)
 {
 	return;
 }
+
+extern char *scsi_dh_get_attached_name(struct request_queue *q)
+{
+	return NULL;
+}
 #endif
-- 
1.6.0.6

Re: [PATCH] RFC: have dm-mpath use already attached scsi_dh

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

Hi Mike,

michaelc@cs.wisc.edu wrote:
> From: Mike Christie <michaelc@cs.wisc.edu>
> 
> If you have a mixed environment of clarriions, where some
> support ALAU and some support PNR, what do you put in
> your multipath.conf? With this patch you do not have to worry about
> it. If those modules are loaded before dm-mpath, then they
> will have attached to the correct devices based on inquiry, alua commands
> and parsing of data buffers (for example in scsi_dh_emc's alua check).
> There is no need for the user to set that info in the multipath.conf.
> And in general since all scsi_dh_modules will attach to the devices
> they work for, we do not need to have users specific this.
> 
No. The problem here is the hardware table from scsi_dh is compiled
in and cannot be changed from userland. The multipath.conf OTOH
is purely user-defined and, what's more, the user might have a valid
reason for modifying it.
(EG EMC Clariion can well be run in PNR mode even though ALUA is
active, or the user might want to try ALUA on any as-of-yet unknown
devices)

So _not_ allowing multipath to override the device handler setting
will just add to the confusion and makes error tracking even more
difficult.

So I would prefer the attached patch, it even save to touch
device handler code at all.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

[-- Attachment #2: scsi-dh-reattach-handler --]
[-- Type: text/plain, Size: 993 bytes --]

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 095f77b..46d01d9 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -592,12 +592,25 @@ static struct pgpath *parse_path(struct arg_set *as, struct path_selector *ps,
 	}
 
 	if (m->hw_handler_name) {
-		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
-				   m->hw_handler_name);
+		struct request_queue *q = bdev_get_queue(p->path.dev->bdev);
+
+		r = scsi_dh_attach(q, m->hw_handler_name);
+		if (r == -EBUSY) {
+			/*
+			 * Already attached to different hw_handler,
+			 * try to reattach with correct one.
+			 */
+			scsi_dh_detach(q);
+			r = scsi_dh_attach(q, m->hw_handler_name);
+		}
 		if (r < 0) {
+			ti->error = "error attaching hardware handler";
 			dm_put_device(ti, p->path.dev);
 			goto bad;
 		}
+	} else {
+		/* Play safe and detach hardware handler */
+		scsi_dh_detach(bdev_get_queue(p->path.dev->bdev));
 	}
 
 	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);

Re: [PATCH] RFC: have dm-mpath use already attached scsi_dh

[Mike Christie]

Hannes Reinecke wrote:
> Hi Mike,
> 
> michaelc@cs.wisc.edu wrote:
>> From: Mike Christie <michaelc@cs.wisc.edu>
>>
>> If you have a mixed environment of clarriions, where some
>> support ALAU and some support PNR, what do you put in
>> your multipath.conf? With this patch you do not have to worry about
>> it. If those modules are loaded before dm-mpath, then they
>> will have attached to the correct devices based on inquiry, alua commands
>> and parsing of data buffers (for example in scsi_dh_emc's alua check).
>> There is no need for the user to set that info in the multipath.conf.
>> And in general since all scsi_dh_modules will attach to the devices
>> they work for, we do not need to have users specific this.
>>
> No. The problem here is the hardware table from scsi_dh is compiled
> in and cannot be changed from userland. The multipath.conf OTOH
> is purely user-defined and, what's more, the user might have a valid
> reason for modifying it.
> (EG EMC Clariion can well be run in PNR mode even though ALUA is
> active, or the user might want to try ALUA on any as-of-yet unknown
> devices)

Ah. I misread the code and misunderstood the compat mode. I thought 
scsi_dh_emc was failing the attach when ALUA support was detected.

> 
> So _not_ allowing multipath to override the device handler setting
> will just add to the confusion and makes error tracking even more
> difficult.
> 
> So I would prefer the attached patch, it even save to touch
> device handler code at all.
> 

Thanks. I think this will work for me.

Are you going to push this for 2.6.30?

Re: [PATCH] RFC: have dm-mpath use already attached scsi_dh

[Hannes Reinecke]

Hi Mike,

Mike Christie wrote:
> Hannes Reinecke wrote:
>> Hi Mike,
>>
>> michaelc@cs.wisc.edu wrote:
>>> From: Mike Christie <michaelc@cs.wisc.edu>
>>>
>>> If you have a mixed environment of clarriions, where some
>>> support ALAU and some support PNR, what do you put in
>>> your multipath.conf? With this patch you do not have to worry about
>>> it. If those modules are loaded before dm-mpath, then they
>>> will have attached to the correct devices based on inquiry, alua
>>> commands
>>> and parsing of data buffers (for example in scsi_dh_emc's alua check).
>>> There is no need for the user to set that info in the multipath.conf.
>>> And in general since all scsi_dh_modules will attach to the devices
>>> they work for, we do not need to have users specific this.
>>>
>> No. The problem here is the hardware table from scsi_dh is compiled
>> in and cannot be changed from userland. The multipath.conf OTOH
>> is purely user-defined and, what's more, the user might have a valid
>> reason for modifying it.
>> (EG EMC Clariion can well be run in PNR mode even though ALUA is
>> active, or the user might want to try ALUA on any as-of-yet unknown
>> devices)
> 
> Ah. I misread the code and misunderstood the compat mode. I thought
> scsi_dh_emc was failing the attach when ALUA support was detected.
> 
>>
>> So _not_ allowing multipath to override the device handler setting
>> will just add to the confusion and makes error tracking even more
>> difficult.
>>
>> So I would prefer the attached patch, it even save to touch
>> device handler code at all.
>>
> 
> Thanks. I think this will work for me.
> 
> Are you going to push this for 2.6.30?
Well, yes, I could.

However, I'm still waiting for agk to push the request-based
multipath patches; I've got quite some updates here for multipathing
which are done with request-based multipath in place;
disentangling them will be quite time consuming (and pointless).

But this one, sure.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] RFC: have dm-mpath use already attached scsi_dh

[Chandra Seetharaman]

I agree with Hannes, that we should allow multipath to override the
default attachment, mainly to give control to the user.

But, I have a small issue with the attached patch. See below.

On Wed, 2009-04-22 at 11:16 +0200, Hannes Reinecke wrote:
> Hi Mike,
> 
<snip>

> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index 095f77b..46d01d9 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -592,12 +592,25 @@ static struct pgpath *parse_path(struct arg_set
> *as, struct path_selector *ps,
>  	}
> 
>  	if (m->hw_handler_name) {
> -		r = scsi_dh_attach(bdev_get_queue(p->path.dev->bdev),
> -				   m->hw_handler_name);
> +		struct request_queue *q = bdev_get_queue(p->path.dev->bdev);
> +
> +		r = scsi_dh_attach(q, m->hw_handler_name);
> +		if (r == -EBUSY) {
> +			/*
> +			 * Already attached to different hw_handler,
> +			 * try to reattach with correct one.
> +			 */
> +			scsi_dh_detach(q);
> +			r = scsi_dh_attach(q, m->hw_handler_name);
> +		}
>  		if (r < 0) {
> +			ti->error = "error attaching hardware handler";
>  			dm_put_device(ti, p->path.dev);
>  			goto bad;
>  		}
> +	} else {
> +		/* Play safe and detach hardware handler */
> +		scsi_dh_detach(bdev_get_queue(p->path.dev->bdev));
>  	}

I prefer not to detach a previously attached hardware handler, if
multipath doesn't have any handler associated with the device. Leaving
it will not do any hard to multipath as the multipath layer would not
call scsi_dh_activate for the device.

Device handler would just handle the sense code as defined in the
kernel.
 
> 
>  	r = ps->type->add_path(ps, &p->path, as->argc, as->argv,
> &ti->error);

Re: Re: [PATCH] RFC: have dm-mpath use already attached scsi_dh

[Alasdair G Kergon]

On Wed, Apr 22, 2009 at 10:32:24AM -0700, Chandra Seetharaman wrote:
> I agree with Hannes, that we should allow multipath to override the
> default attachment, mainly to give control to the user.
> But, I have a small issue with the attached patch. See below.
> I prefer not to detach a previously attached hardware handler, if
> multipath doesn't have any handler associated with the device. Leaving
> it will not do any hard to multipath as the multipath layer would not
> call scsi_dh_activate for the device.
 
On balance, I'm inclined to agree - too many layers of config overlapping
here.

If you agree, would one of you like to submit the patch to dm-devel properly,
with appropriate signoffs and a descriptive patch header?

Alasdair
-- 
agk@redhat.com