[PATCH 2/7] scsi_dh: Add 'dh_state' sysfs attribute

[PATCH 2/7] scsi_dh: Add 'dh_state' sysfs attribute

[Hannes Reinecke]

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;
+		}
+	}
+
+	return err<0?err:count;
+}
+
+static ssize_t
+show_dh_state(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+
+	if (!sdev->scsi_dh_data)
+		return snprintf(buf, 20, "detached\n");
+
+	return snprintf(buf, 20, "%s\n", sdev->scsi_dh_data->scsi_dh->name);
+}
+
+static struct device_attribute scsi_dh_state_attr =
+	__ATTR(dh_state, S_IRUGO | S_IWUSR, show_dh_state,
+	       store_dh_state);
+
+/*
+ * scsi_dh_sysfs_attr_add - Callback for scsi_init_dh
+ */
+static int scsi_dh_sysfs_attr_add(struct device *dev, void *data)
+{
+	struct scsi_device *sdev;
+	int err;
+
+	if (!scsi_is_sdev_device(dev))
+		return 0;
+
+	sdev = to_scsi_device(dev);
+
+	err = device_create_file(&sdev->sdev_gendev,
+				 &scsi_dh_state_attr);
+
+	return 0;
+}
+
+/*
+ * scsi_dh_sysfs_attr_remove - Callback for scsi_exit_dh
+ */
+static int scsi_dh_sysfs_attr_remove(struct device *dev, void *data)
+{
+	struct scsi_device *sdev;
+
+	if (!scsi_is_sdev_device(dev))
+		return 0;
+
+	sdev = to_scsi_device(dev);
+
+	device_remove_file(&sdev->sdev_gendev,
+			   &scsi_dh_state_attr);
+
+	return 0;
+}
+
+/*
  * scsi_dh_notifier - notifier chain callback
  */
 static int scsi_dh_notifier(struct notifier_block *nb,
@@ -86,7 +178,7 @@ static int scsi_dh_notifier(struct notifier_block *nb,
 {
 	struct device *dev = data;
 	struct scsi_device *sdev;
-	int i, err = 0;
+	int i;
 	struct scsi_device_handler *tmp, *devinfo = NULL;
 
 	if (!scsi_is_sdev_device(dev))
@@ -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);
 	} 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;
 }
 
 /*
@@ -275,11 +372,17 @@ static int __init scsi_dh_init(void)
 
 	r = bus_register_notifier(&scsi_bus_type, &scsi_dh_nb);
 
+	if (!r)
+		bus_for_each_dev(&scsi_bus_type, NULL, NULL,
+				 scsi_dh_sysfs_attr_add);
+
 	return r;
 }
 
 static void __exit scsi_dh_exit(void)
 {
+	bus_for_each_dev(&scsi_bus_type, NULL, NULL,
+			 scsi_dh_sysfs_attr_remove);
 	bus_unregister_notifier(&scsi_bus_type, &scsi_dh_nb);
 }
 
-- 
1.5.2.4

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

[Chandra Seetharaman]

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

> +		}
> +	}
> +
> +	return err<0?err:count;
> +}
> +
> +static ssize_t
> +show_dh_state(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct scsi_device *sdev = to_scsi_device(dev);
> +
> +	if (!sdev->scsi_dh_data)
> +		return snprintf(buf, 20, "detached\n");
> +
> +	return snprintf(buf, 20, "%s\n", sdev->scsi_dh_data->scsi_dh->name);
> +}
> +
> +static struct device_attribute scsi_dh_state_attr =
> +	__ATTR(dh_state, S_IRUGO | S_IWUSR, show_dh_state,
> +	       store_dh_state);
> +
> +/*
> + * scsi_dh_sysfs_attr_add - Callback for scsi_init_dh
> + */
> +static int scsi_dh_sysfs_attr_add(struct device *dev, void *data)
> +{
> +	struct scsi_device *sdev;
> +	int err;
> +
> +	if (!scsi_is_sdev_device(dev))
> +		return 0;
> +
> +	sdev = to_scsi_device(dev);
> +
> +	err = device_create_file(&sdev->sdev_gendev,
> +				 &scsi_dh_state_attr);
> +
> +	return 0;
> +}
> +
> +/*
> + * scsi_dh_sysfs_attr_remove - Callback for scsi_exit_dh
> + */
> +static int scsi_dh_sysfs_attr_remove(struct device *dev, void *data)
> +{
> +	struct scsi_device *sdev;
> +
> +	if (!scsi_is_sdev_device(dev))
> +		return 0;
> +
> +	sdev = to_scsi_device(dev);
> +
> +	device_remove_file(&sdev->sdev_gendev,
> +			   &scsi_dh_state_attr);
> +
> +	return 0;
> +}
> +
> +/*
>   * scsi_dh_notifier - notifier chain callback
>   */
>  static int scsi_dh_notifier(struct notifier_block *nb,
> @@ -86,7 +178,7 @@ static int scsi_dh_notifier(struct notifier_block *nb,
>  {
>  	struct device *dev = data;
>  	struct scsi_device *sdev;
> -	int i, err = 0;
> +	int i;
>  	struct scsi_device_handler *tmp, *devinfo = NULL;
> 
>  	if (!scsi_is_sdev_device(dev))
> @@ -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.

>  	} 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 ?
>  }
> 
>  /*
> @@ -275,11 +372,17 @@ static int __init scsi_dh_init(void)
> 
>  	r = bus_register_notifier(&scsi_bus_type, &scsi_dh_nb);
> 
> +	if (!r)
> +		bus_for_each_dev(&scsi_bus_type, NULL, NULL,
> +				 scsi_dh_sysfs_attr_add);
> +
>  	return r;
>  }
> 
>  static void __exit scsi_dh_exit(void)
>  {
> +	bus_for_each_dev(&scsi_bus_type, NULL, NULL,
> +			 scsi_dh_sysfs_attr_remove);
>  	bus_unregister_notifier(&scsi_bus_type, &scsi_dh_nb);
>  }
> 

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

[Hannes Reinecke]

[-- 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