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

[Mike Snitzer <snitzer@redhat.com>]

On Wed, May 09 2012 at  2:10pm -0400,
Moger, Babu <Babu.Moger@netapp.com> wrote:

> Mike,
> 
> > -----Original Message-----
> > From: Mike Snitzer [mailto:snitzer@redhat.com]
> > Sent: Tuesday, May 08, 2012 4:56 PM
> > To: dm-devel@redhat.com
> > Cc: agk@redhat.com; hare@suse.de; Moger, Babu; sekharan@us.ibm.com;
> > Mike Snitzer
> > Subject: [PATCH v2 4/5] dm mpath: add 'default_hw_handler' feature
> > 
> > From: Hannes Reinecke <hare@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.
  
> 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?

> > 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'.

So are you testing this patch with a device handler having already been
attached?

Mike