[dm-devel] [PATCH v2 4/5] dm mpath: add 'default_hw_handler' feature

[Moger, Babu]

> -----Original Message-----
> From: Mike Snitzer [mailto:snitzer at redhat.com]
> Sent: Wednesday, May 09, 2012 2:17 PM
> To: Moger, Babu
> Cc: dm-devel at redhat.com; agk at redhat.com; hare at suse.de;
> sekharan at us.ibm.com
> Subject: Re: [PATCH v2 4/5] dm mpath: add 'default_hw_handler' feature
> 
> On Wed, May 09 2012 at  2:10pm -0400,
> Moger, Babu <Babu.Moger at netapp.com> wrote:
> 
> > Mike,
> >
> > > -----Original Message-----
> > > From: Mike Snitzer [mailto:snitzer at redhat.com]
> > > Sent: Tuesday, May 08, 2012 4:56 PM
> > > To: dm-devel at redhat.com
> > > Cc: agk at redhat.com; hare at suse.de; Moger, Babu;
> sekharan at us.ibm.com;
> > > Mike Snitzer
> > > Subject: [PATCH v2 4/5] dm mpath: add 'default_hw_handler' feature
> > >
> > > From: Hannes Reinecke <hare at suse.de>
> > >
> > > When specifying the feature 'default_hw_handler' multipath will be use
> > > the currently attached hardware handler instead of trying to attach the
> > > one specified during table load.  If no hardware handler is attached the
> > > specified hardware handler will be used.
> >
> > I am trying to test these patches right now.  What is the expectation from
> > Multipath tools for this to work correctly.  Do I have to pass following
> > Parameters?
> >
> > hardware_handler "1 alua"
> > features "1 default_hw_handler"
> 
> Yes.
Ok. thanks

> 
> > It appears to me that the first line will forcibly load the alua handler even if
> > the default handler is different.
> 
> Are you saying that based on testing or based on code review?

I was only reviewing the code.  Looking at the code again, It should work fine.
I am going to test it anyway.

> 
> > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> > > index c351607..0fc6849 100644
> > > --- a/drivers/md/dm-mpath.c
> > > +++ b/drivers/md/dm-mpath.c
> > > @@ -585,9 +587,18 @@ static struct pgpath *parse_path(struct
> dm_arg_set
> > > *as, struct path_selector *ps
> > >  		goto bad;
> > >  	}
> > >
> > > -	if (m->hw_handler_name) {
> > > -		struct request_queue *q = bdev_get_queue(p->path.dev-
> > > >bdev);
> > > +	if (m->use_default_hw_handler || m->hw_handler_name)
> > > +		q = bdev_get_queue(p->path.dev->bdev);
> > > +
> > > +	if (m->use_default_hw_handler) {
> > > +		const char *attached_handler_name =
> scsi_dh_attached_handler_name(q);
> > > +		if (attached_handler_name) {
> > > +			kfree(m->hw_handler_name);
> > > +			m->hw_handler_name =
> kstrdup(attached_handler_name, GFP_KERNEL);
> > > +		}
> > > +	}
> > >
> > > +	if (m->hw_handler_name) {
> > >  		r = scsi_dh_attach(q, m->hw_handler_name);
> > >  		if (r == -EBUSY) {
> > >  			/*
> 
> The above hunk is what will cause the currently attached device handler
> to be used.  But if a device handler is _not_ attached then we fallback
> to using the provided 'hardware_handler'.

Got it.
> 
> So are you testing this patch with a device handler having already been
> attached?

Yes. I will provide the feedback only tomorrow.