Re: [PATCH 3/3] dm-multipath: 'default_hw_handler' feature

[Hannes Reinecke <hare@suse.de>]

On 05/08/2012 04:46 PM, Mike Snitzer wrote:
> On Tue, May 08 2012 at 10:18am -0400,
> Hannes Reinecke<hare@suse.de>  wrote:
>
>> This patch introduces a 'default_hw_handler' feature for
>> dm-mpath. When specifying the feature 'default_hw_handler'
>> dm-multipath will be using the currently attached hardware
>> handler instead of trying to re-attach with the one
>> specified during table creation.
>> If no hardware handler is attached the specified hardware
>> handler will be used.
>>
>> Signed-off-by: Hannes Reinecke<hare@suse.de>
>> ---
>>   drivers/md/dm-mpath.c |   25 ++++++++++++++++++++++---
>>   1 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>> index 6d3f2a8..c1ef41d 100644
>> --- a/drivers/md/dm-mpath.c
>> +++ b/drivers/md/dm-mpath.c
>> @@ -57,6 +57,7 @@ struct priority_group {
>>   };
>>
>>   #define FEATURE_NO_PARTITIONS 1
>> +#define FEATURE_DEFAULT_HW_HANDLER 2
>>
>>   /* Multipath context */
>>   struct multipath {
>> @@ -589,14 +590,24 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
>>
>>   	if (m->hw_handler_name) {
>>   		struct request_queue *q = bdev_get_queue(p->path.dev->bdev);
>> +		const char *hw_handler_name = m->hw_handler_name;
>>
>> -		r = scsi_dh_attach(q, m->hw_handler_name);
>> +		if (m->features&  FEATURE_DEFAULT_HW_HANDLER)
>> +			hw_handler_name = NULL;
>> +
>> +		r = scsi_dh_attach(q, 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, hw_handler_name);
>> +		} else if (r == -EINVAL) {
>> +			/*
>> +			 * No hardware handler attached, use
>> +			 * the specified one.
>> +			 */
>>   			r = scsi_dh_attach(q, m->hw_handler_name);
>>   		}
>
> I like what you've done with the 'default_hw_handler' feature.  But
> you're not establishing m->hw_handler_name.  As such the rest of the
> dm-mpath.c code that keys off of m->hw_handler_name (e.g. reinstate_path,
> pg_init_done, free_pgpaths) will not work.
>
Ah. True.

> Would you be OK with a hybrid of both our approaches?  Use your
> 'default_hw_handler' feature and, rather than pass a NULL name to
> scsi_dh_attach, use scsi_dh_attached_handler_name like I provided?
>
Yes, sure. That sounds like the best approach here.

I'm not _that_ keen on my 'NULL' hw handler name approach, so your idea
of having a function for that looks like a better idea.

> (I don't see a problem with altering m->hw_handler_name to reflect the
> scsi_dh that is used by the path.. Babu agreed with this too).
>
Yep.

> I'd be happy to merge our dm-mpath patches and attribute authorship to
> you.
>
Oh, cool.
Please do.

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)