Re: [dm-devel] [PATCH 2/7] scsi_dh: Add 'dh_state' sysfs attribute

[Hannes Reinecke <hare@suse.de>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 4111 bytes --]

On Thu, May 22, 2008 at 07:08:20PM -0700, Chandra Seetharaman wrote:
> 
> On Tue, 2008-05-20 at 16:05 +0200, Hannes Reinecke wrote:
> > Implement a 'dh_state' sdev attribute for dynamic device handler
> > manipulation. A read on the attribute will return the name of
> > the currently attached device handler or 'detached' if no handler
> > is attached.
> > The attribute allows the following strings to be written:
> > - The name of the device handler to be attached if the state is
> >   'detached'.
> > - 'activate' to trigger path activation if a device handler
> >   is attached.
> > - 'detach' to detach the currently attached device handler.
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > ---
> >  drivers/scsi/device_handler/scsi_dh.c |  107 ++++++++++++++++++++++++++++++++-
> >  1 files changed, 105 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/scsi/device_handler/scsi_dh.c b/drivers/scsi/device_handler/scsi_dh.c
> > index 0b5c457..b80fae7 100644
> > --- a/drivers/scsi/device_handler/scsi_dh.c
> > +++ b/drivers/scsi/device_handler/scsi_dh.c
> > @@ -79,6 +79,98 @@ static void scsi_dh_handler_detach(struct scsi_device *sdev)
> >  }
> > 
> >  /*
> > + * Functions for sysfs attribute 'dh_state'
> > + */
> > +static ssize_t
> > +store_dh_state(struct device *dev, struct device_attribute *attr,
> > +	       const char *buf, size_t count)
> > +{
> > +	struct scsi_device *sdev = to_scsi_device(dev);
> > +	struct scsi_device_handler *scsi_dh;
> > +	int err = -EINVAL;
> > +
> > +	if (!sdev->scsi_dh_data) {
> > +		/*
> > +		 * Attach to a device handler
> > +		 */
> > +		if (!(scsi_dh = get_device_handler(buf)))
> > +			return err;
> > +		err = scsi_dh_handler_attach(sdev, scsi_dh);
> > +	} else {
> > +		if (!strncmp(buf, "detach", 6)) {
> > +			/*
> > +			 * Detach from a device handler
> > +			 */
> > +			scsi_dh_handler_detach(sdev);
> > +			err = 0;
> > +		} else if (!strncmp(buf, "activate", 8)) {
> > +			/*
> > +			 * Activate a device handler
> > +			 */
> > +			scsi_dh = sdev->scsi_dh_data->scsi_dh;
> > +			if (scsi_dh->activate)
> > +				err = scsi_dh->activate(sdev);
> > +			else
> > +				err = 0;
> 
> Sorry for the late comment. Why do we want to give the user to be able
> to activate a path ? (I am assuming user as we do not want to be calling
> this from multipath tools).
> 
This is quite a handy feature if you want to test failover manually (ie
if the commands are being sent correctly and the array reacts properly).
And, of course, there might be scenarios where you'd want to trigger
a failover manually.

[ .. ]
> > @@ -114,14 +206,19 @@ static int scsi_dh_notifier(struct notifier_block *nb,
> >  		goto out;
> > 
> >  	if (action == BUS_NOTIFY_ADD_DEVICE) {
> > +		int err;
> > +
> >  		err = scsi_dh_handler_attach(sdev, devinfo);
> > +		if (!err)
> > +			err = device_create_file(dev, &scsi_dh_state_attr);
> 
> we don't want to detach when we fail creating the file ? I am ok with
> it, but just wanted to make sure that is what you intended.
> 
The 'dh_state' attribute is just an add-on and not necessary for proper
functionality. So I thought we could just ignore this error.
But seeing that this indicates some general error condition it's probably
better to at least return this error.

> >  	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
> >  		if (sdev->scsi_dh_data == NULL)
> >  			goto out;
> > +		device_remove_file(dev, &scsi_dh_state_attr);
> >  		scsi_dh_handler_detach(sdev);
> >  	}
> >  out:
> > -	return err;
> > +	return 0;
> 
> why are we not returning err ?
See above. 

Fixed in the scsi_dh branch.

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