[PATCH] Fixes to EMC scsi device handler

[PATCH] Fixes to EMC scsi device handler

[Hannes Reinecke]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

Hi Chandra,

based on your latest patchset there are some issues which
should be fixed:

- When SYSFS_DEPRECATED is not set also scsi_target and
  scsi_host devices will be attached to the scsi bus.
  So we have to check with 'scsi_is_sdev_device' before
  processing the notifier call
- Newer EMC flarecode also provide for a 'VRAID' device
  model; this should be added to the device map
- The name should be 'emc', not 'emc_clariion', for
  integration with existing multipath-tools
- It's spelled 'detached' :-)

Patch attached.

Some issues which I'd like to see eventually:

- We should have a way of adding/modifying the internal
  device map. Currently we have to recompile the module
  every time a new device definition is found.
- The ->activate function should be able to be triggered
  manually via sysfs.

James, seeing that the scsi_dh pointer is already present
in the sdev structure, would you accept a patch which adds
scsi_dh attributes to drivers/scsi/scsi_sysfs.c?
Having them encapsulated into drivers/scsi/device_handler
somewhere is really painful due to the asynchronous nature
of this whole thing ...

Apart from this: Great work!
I'll check the other device handlers, too; already noticed
that the device map for HP is wrong :-)

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)

[-- Attachment #2: scsi-dh-emc-fixes --]
[-- Type: text/plain, Size: 1725 bytes --]

Fixes to EMC scsi device handler

Several fixes to the EMC scsi device handler:
- The name should be 'emc' for seamless integration with multipathing
- Newer EMC flarecode also has a 'VRAID' type
- Check if this is really a sdev before processing the notifier call

Signed-off-by: Hannes Reinecke <hare@suse.de>

diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index 1b18989..0aded96 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -25,7 +25,7 @@
 #include <scsi/scsi_dh.h>
 #include <scsi/scsi_device.h>
 
-#define CLARIION_NAME			"emc_clariion"
+#define CLARIION_NAME			"emc"
 
 #define CLARIION_TRESPASS_PAGE		0x22
 #define CLARIION_BUFFER_SIZE		0x80
@@ -390,12 +390,13 @@ static int clariion_check_sense(struct scsi_device *sdev,
 	return SUCCESS;
 }
 
-static const struct {
+static const struct clariion_dev_list_t {
 	char *vendor;
 	char *model;
 } clariion_dev_list[] = {
 	{"DGC", "RAID"},
 	{"DGC", "DISK"},
+	{"DGC", "VRAID"},
 	{NULL, NULL},
 };
 
@@ -422,6 +423,9 @@ static int clariion_bus_notify(struct notifier_block *nb,
 	int i, found = 0;
 	unsigned long flags;
 
+	if (!scsi_is_sdev_device(dev))
+		return 0;
+
 	if (action == BUS_NOTIFY_ADD_DEVICE) {
 		for (i = 0; clariion_dev_list[i].vendor; i++) {
 			if (!strncmp(sdev->vendor, clariion_dev_list[i].vendor,
@@ -465,7 +469,7 @@ static int clariion_bus_notify(struct notifier_block *nb,
 		sdev->scsi_dh_data = NULL;
 		spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
 
-		sdev_printk(KERN_NOTICE, sdev, "Dettached %s.\n",
+		sdev_printk(KERN_NOTICE, sdev, "Detached %s.\n",
 			    CLARIION_NAME);
 
 		kfree(scsi_dh_data);

Re: [PATCH] Fixes to EMC scsi device handler

[Chandra Seetharaman]

On Mon, 2008-05-05 at 13:55 +0200, Hannes Reinecke wrote:
> Hi Chandra,
> 
> based on your latest patchset there are some issues which
> should be fixed:
> 
> - When SYSFS_DEPRECATED is not set also scsi_target and
>   scsi_host devices will be attached to the scsi bus.
>   So we have to check with 'scsi_is_sdev_device' before
>   processing the notifier call

This need to be done for all the handlers, right ?
>  
> - Newer EMC flarecode also provide for a 'VRAID' device
>   model; this should be added to the device map
> - The name should be 'emc', not 'emc_clariion', for
>   integration with existing multipath-tools
> - It's spelled 'detached' :-)
> 
> Patch attached.

Looks good to me.
> 
> Some issues which I'd like to see eventually:
> 
> - We should have a way of adding/modifying the internal
>   device map. Currently we have to recompile the module
>   every time a new device definition is found.

Yeah, very true. it will become an annoyance. Is this the reason why
you were asking for the sysfs interface (below) ?

> - The ->activate function should be able to be triggered
>   manually via sysfs.

Why would you need this ? 
> 
> James, seeing that the scsi_dh pointer is already present
> in the sdev structure, would you accept a patch which adds
> scsi_dh attributes to drivers/scsi/scsi_sysfs.c?
> Having them encapsulated into drivers/scsi/device_handler
> somewhere is really painful due to the asynchronous nature
> of this whole thing ...
> 
> Apart from this: Great work!
> I'll check the other device handlers, too; already noticed
> that the device map for HP is wrong :-)

Can you please help fix it. 
> 
> Cheers,
> 
> hannes
> plain text document attachment (scsi-dh-emc-fixes)
> Fixes to EMC scsi device handler
> 
> Several fixes to the EMC scsi device handler:
> - The name should be 'emc' for seamless integration with multipathing
> - Newer EMC flarecode also has a 'VRAID' type
> - Check if this is really a sdev before processing the notifier call
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
> index 1b18989..0aded96 100644
> --- a/drivers/scsi/device_handler/scsi_dh_emc.c
> +++ b/drivers/scsi/device_handler/scsi_dh_emc.c
> @@ -25,7 +25,7 @@
>  #include <scsi/scsi_dh.h>
>  #include <scsi/scsi_device.h>
> 
> -#define CLARIION_NAME			"emc_clariion"
> +#define CLARIION_NAME			"emc"
> 
>  #define CLARIION_TRESPASS_PAGE		0x22
>  #define CLARIION_BUFFER_SIZE		0x80
> @@ -390,12 +390,13 @@ static int clariion_check_sense(struct scsi_device *sdev,
>  	return SUCCESS;
>  }
> 
> -static const struct {
> +static const struct clariion_dev_list_t {
>  	char *vendor;
>  	char *model;
>  } clariion_dev_list[] = {
>  	{"DGC", "RAID"},
>  	{"DGC", "DISK"},
> +	{"DGC", "VRAID"},
>  	{NULL, NULL},
>  };
> 
> @@ -422,6 +423,9 @@ static int clariion_bus_notify(struct notifier_block *nb,
>  	int i, found = 0;
>  	unsigned long flags;
> 
> +	if (!scsi_is_sdev_device(dev))
> +		return 0;
> +
>  	if (action == BUS_NOTIFY_ADD_DEVICE) {
>  		for (i = 0; clariion_dev_list[i].vendor; i++) {
>  			if (!strncmp(sdev->vendor, clariion_dev_list[i].vendor,
> @@ -465,7 +469,7 @@ static int clariion_bus_notify(struct notifier_block *nb,
>  		sdev->scsi_dh_data = NULL;
>  		spin_unlock_irqrestore(sdev->request_queue->queue_lock, flags);
> 
> -		sdev_printk(KERN_NOTICE, sdev, "Dettached %s.\n",
> +		sdev_printk(KERN_NOTICE, sdev, "Detached %s.\n",
>  			    CLARIION_NAME);
> 
>  		kfree(scsi_dh_data);

RE: Re: [PATCH] Fixes to EMC scsi device handler

[berthiaume_wayne]

FYI: The VRAID type is for the CLARiiON implementation of RAID6. 

Regards,
Wayne.

-----Original Message-----
From: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com]
On Behalf Of Chandra Seetharaman
Sent: Monday, May 05, 2008 9:34 PM
To: Hannes Reinecke
Cc: James Bottomley; dm-devel; SCSI Mailing List
Subject: [dm-devel] Re: [PATCH] Fixes to EMC scsi device handler


On Mon, 2008-05-05 at 13:55 +0200, Hannes Reinecke wrote:
> Hi Chandra,
> 
> based on your latest patchset there are some issues which
> should be fixed:
> 
> - When SYSFS_DEPRECATED is not set also scsi_target and
>   scsi_host devices will be attached to the scsi bus.
>   So we have to check with 'scsi_is_sdev_device' before
>   processing the notifier call

This need to be done for all the handlers, right ?
>  
> - Newer EMC flarecode also provide for a 'VRAID' device
>   model; this should be added to the device map
> - The name should be 'emc', not 'emc_clariion', for
>   integration with existing multipath-tools
> - It's spelled 'detached' :-)
> 
> Patch attached.

Looks good to me.
> 
> Some issues which I'd like to see eventually:
> 
> - We should have a way of adding/modifying the internal
>   device map. Currently we have to recompile the module
>   every time a new device definition is found.

Yeah, very true. it will become an annoyance. Is this the reason why
you were asking for the sysfs interface (below) ?

> - The ->activate function should be able to be triggered
>   manually via sysfs.

Why would you need this ? 
> 
> James, seeing that the scsi_dh pointer is already present
> in the sdev structure, would you accept a patch which adds
> scsi_dh attributes to drivers/scsi/scsi_sysfs.c?
> Having them encapsulated into drivers/scsi/device_handler
> somewhere is really painful due to the asynchronous nature
> of this whole thing ...
> 
> Apart from this: Great work!
> I'll check the other device handlers, too; already noticed
> that the device map for HP is wrong :-)

Can you please help fix it. 
> 
> Cheers,
> 
> hannes
> plain text document attachment (scsi-dh-emc-fixes)
> Fixes to EMC scsi device handler
> 
> Several fixes to the EMC scsi device handler:
> - The name should be 'emc' for seamless integration with multipathing
> - Newer EMC flarecode also has a 'VRAID' type
> - Check if this is really a sdev before processing the notifier call
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> 
> diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c
b/drivers/scsi/device_handler/scsi_dh_emc.c
> index 1b18989..0aded96 100644
> --- a/drivers/scsi/device_handler/scsi_dh_emc.c
> +++ b/drivers/scsi/device_handler/scsi_dh_emc.c
> @@ -25,7 +25,7 @@
>  #include <scsi/scsi_dh.h>
>  #include <scsi/scsi_device.h>
> 
> -#define CLARIION_NAME			"emc_clariion"
> +#define CLARIION_NAME			"emc"
> 
>  #define CLARIION_TRESPASS_PAGE		0x22
>  #define CLARIION_BUFFER_SIZE		0x80
> @@ -390,12 +390,13 @@ static int clariion_check_sense(struct
scsi_device *sdev,
>  	return SUCCESS;
>  }
> 
> -static const struct {
> +static const struct clariion_dev_list_t {
>  	char *vendor;
>  	char *model;
>  } clariion_dev_list[] = {
>  	{"DGC", "RAID"},
>  	{"DGC", "DISK"},
> +	{"DGC", "VRAID"},
>  	{NULL, NULL},
>  };
> 
> @@ -422,6 +423,9 @@ static int clariion_bus_notify(struct
notifier_block *nb,
>  	int i, found = 0;
>  	unsigned long flags;
> 
> +	if (!scsi_is_sdev_device(dev))
> +		return 0;
> +
>  	if (action == BUS_NOTIFY_ADD_DEVICE) {
>  		for (i = 0; clariion_dev_list[i].vendor; i++) {
>  			if (!strncmp(sdev->vendor,
clariion_dev_list[i].vendor,
> @@ -465,7 +469,7 @@ static int clariion_bus_notify(struct
notifier_block *nb,
>  		sdev->scsi_dh_data = NULL;
>  		spin_unlock_irqrestore(sdev->request_queue->queue_lock,
flags);
> 
> -		sdev_printk(KERN_NOTICE, sdev, "Dettached %s.\n",
> +		sdev_printk(KERN_NOTICE, sdev, "Detached %s.\n",
>  			    CLARIION_NAME);
> 
>  		kfree(scsi_dh_data);

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel