[PATCH ] multipath: Fall back to getuid_callout if ID_SERIAL attribute is missing.

[PATCH ] multipath: Fall back to getuid_callout if ID_SERIAL attribute is missing.

[Merla, ShivaKrishna]

 Sometimes whenever a path is added, scsi_id call during udev rule processing
 can fail and ID_SERIAL attribute will not be set. This causes multipathd to add
 the path as orphan. We have seen several instances of this happening during
 testing.

      Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid_attribute = ID_SERIAL (config file default)
      Jun 17 10:14:43 ictm-vader multipathd[474]: sda: no ID_SERIAL attribute
      Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid = <empty> (udev)
      Jun 17 10:14:43 ictm-vader multipathd[474]: sda: no ID_SERIAL attribute
      Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid = <empty> (udev)
      Jun 17 10:14:43 ictm-vader multipathd[474]: sda: failed to get path uid
      Jun 17 10:14:43 ictm-vader multipathd[474]: sda: orphan path, failed to add path
 
 This patch handles this case by allowing to fall back to explict getuid_callout
 incase if ID_SERIAL attribute is not set. This way support for deprecated
 getuid_callout is not lost for older versions where ID_SERIAL attribute is not
 present but also serves good purpose in the scenario mentioned above.

Signed-off-by: Shiva Krishna Merla<shivakrishna.merla@netapp.com>
 ---
 libmultipath/defaults.h  |    1 +
 libmultipath/discovery.c |   59 +++++++++++++++++++++------------------------
 libmultipath/propsel.c   |   29 ++++++++++++----------
 3 files changed, 45 insertions(+), 44 deletions(-)

diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
index b83d9fb..47bd7ba 100644
--- a/libmultipath/defaults.h
+++ b/libmultipath/defaults.h
@@ -1,4 +1,5 @@
 #define DEFAULT_UID_ATTRIBUTE	"ID_SERIAL"
+#define DEFAULT_GETUID		"/lib/udev/scsi_id --whitelisted --device=/dev/%n"
 #define DEFAULT_UDEVDIR		"/dev"
 #define DEFAULT_MULTIPATHDIR	"/" LIB_STRING "/multipath"
 #define DEFAULT_SELECTOR	"service-time 0"
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 786e1de..7cf674d 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1098,7 +1098,8 @@ static int
 get_uid (struct path * pp)
 {
 	char *c;
-	const char *origin;
+	const char *origin = "udev";
+	const char *value;
 
 	if (!pp->uid_attribute && !pp->getuid)
 		select_getuid(pp);
@@ -1109,40 +1110,36 @@ get_uid (struct path * pp)
 	}
 
 	memset(pp->wwid, 0, WWID_SIZE);
-	if (pp->getuid) {
-		char buff[CALLOUT_MAX_SIZE];
-
-		/* Use 'getuid' callout, deprecated */
-		condlog(1, "%s: using deprecated getuid callout", pp->dev);
-		if (apply_format(pp->getuid, &buff[0], pp)) {
-			condlog(0, "error formatting uid callout command");
-			memset(pp->wwid, 0, WWID_SIZE);
-		} else if (execute_program(buff, pp->wwid, WWID_SIZE)) {
-			condlog(3, "error calling out %s", buff);
-			memset(pp->wwid, 0, WWID_SIZE);
+
+	value = udev_device_get_property_value(pp->udev,
+					       pp->uid_attribute);
+	if ((!value || strlen(value) == 0) && conf->dry_run == 2)
+		value = getenv(pp->uid_attribute);
+	if (value && strlen(value)) {
+		size_t len = WWID_SIZE;
+
+		if (strlen(value) + 1 > WWID_SIZE) {
+			condlog(0, "%s: wwid overflow", pp->dev);
+		} else {
+			len = strlen(value);
 		}
-		origin = "callout";
+		strncpy(pp->wwid, value, len);
 	} else {
-		const char *value;
-
-		value = udev_device_get_property_value(pp->udev,
-						       pp->uid_attribute);
-		if ((!value || strlen(value) == 0) && conf->dry_run == 2)
-			value = getenv(pp->uid_attribute);
-		if (value && strlen(value)) {
-			size_t len = WWID_SIZE;
-
-			if (strlen(value) + 1 > WWID_SIZE) {
-				condlog(0, "%s: wwid overflow", pp->dev);
-			} else {
-				len = strlen(value);
+		condlog(3, "%s: no %s attribute", pp->dev,
+			pp->uid_attribute);
+		if (pp->getuid) {
+			char buff[CALLOUT_MAX_SIZE];
+			/* Use 'getuid' callout, deprecated */
+			 condlog(1, "%s: using deprecated getuid callout", pp->dev);
+			if (apply_format(pp->getuid, &buff[0], pp)) {
+				condlog(0, "error formatting uid callout command");
+				memset(pp->wwid, 0, WWID_SIZE);
+			} else if (execute_program(buff, pp->wwid, WWID_SIZE)) {
+				condlog(3, "error calling out %s", buff);
+				memset(pp->wwid, 0, WWID_SIZE);
 			}
-			strncpy(pp->wwid, value, len);
-		} else {
-			condlog(3, "%s: no %s attribute", pp->dev,
-				pp->uid_attribute);
+			origin = "callout";
 		}
-		origin = "udev";
 	}
 	/* Strip any trailing blanks */
 	c = strchr(pp->wwid, '\0');
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index ee6cea6..b8a155c 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -379,29 +379,32 @@ select_getuid (struct path * pp)
 		pp->uid_attribute = pp->hwe->uid_attribute;
 		condlog(3, "%s: uid_attribute = %s (controller setting)",
 			pp->dev, pp->uid_attribute);
-		return 0;
+	}
+	else if (conf->uid_attribute) {
+		pp->uid_attribute = conf->uid_attribute;
+		condlog(3, "%s: uid_attribute = %s (config file default)",
+			pp->dev, pp->uid_attribute);
+	}
+	else {
+		pp->uid_attribute = STRDUP(DEFAULT_UID_ATTRIBUTE);
+		condlog(3, "%s: uid_attribute = %s (internal default)",
+		pp->dev, pp->uid_attribute);
 	}
 	if (pp->hwe && pp->hwe->getuid) {
 		pp->getuid = pp->hwe->getuid;
 		condlog(3, "%s: getuid = %s (deprecated) (controller setting)",
 			pp->dev, pp->getuid);
-		return 0;
 	}
-	if (conf->uid_attribute) {
-		pp->uid_attribute = conf->uid_attribute;
-		condlog(3, "%s: uid_attribute = %s (config file default)",
-			pp->dev, pp->uid_attribute);
-		return 0;
-	}
-	if (conf->getuid) {
+	else if (conf->getuid) {
 		pp->getuid = conf->getuid;
 		condlog(3, "%s: getuid = %s (deprecated) (config file default)",
 			pp->dev, pp->getuid);
-		return 0;
 	}
-	pp->uid_attribute = STRDUP(DEFAULT_UID_ATTRIBUTE);
-	condlog(3, "%s: uid_attribute = %s (internal default)",
-		pp->dev, pp->uid_attribute);
+	else {
+		pp->getuid = STRDUP(DEFAULT_GETUID);
+		condlog(3, "%s: getuid = %s (deprecated) (internal default)",
+			pp->dev, pp->getuid);
+	}
 	return 0;
 }
--

Re: [PATCH ] multipath: Fall back to getuid_callout if ID_SERIAL attribute is missing.

[Hannes Reinecke]

On 06/18/2014 02:17 AM, Merla, ShivaKrishna wrote:
>   Sometimes whenever a path is added, scsi_id call during udev rule processing
>   can fail and ID_SERIAL attribute will not be set. This causes multipathd to add
>   the path as orphan. We have seen several instances of this happening during
>   testing.
>
>        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid_attribute = ID_SERIAL (config file default)
>        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: no ID_SERIAL attribute
>        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid = <empty> (udev)
>        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: no ID_SERIAL attribute
>        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid = <empty> (udev)
>        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: failed to get path uid
>        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: orphan path, failed to add path
>
>   This patch handles this case by allowing to fall back to explict getuid_callout
>   incase if ID_SERIAL attribute is not set. This way support for deprecated
>   getuid_callout is not lost for older versions where ID_SERIAL attribute is not
>   present but also serves good purpose in the scenario mentioned above.
>
> Signed-off-by: Shiva Krishna Merla<shivakrishna.merla@netapp.com>

Hmm. I have a similar patch in my tree. Will be checking.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH ] multipath: Fall back to getuid_callout if ID_SERIAL attribute is missing.

[Merla, ShivaKrishna]

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Wednesday, June 18, 2014 1:18 AM
> To: Merla, ShivaKrishna; christophe.varoqui@opensvc.com
> Cc: dm-devel@redhat.com
> Subject: Re: [PATCH ] multipath: Fall back to getuid_callout if ID_SERIAL
> attribute is missing.
> 
> On 06/18/2014 02:17 AM, Merla, ShivaKrishna wrote:
> >   Sometimes whenever a path is added, scsi_id call during udev rule
> processing
> >   can fail and ID_SERIAL attribute will not be set. This causes multipathd to
> add
> >   the path as orphan. We have seen several instances of this happening
> during
> >   testing.
> >
> >        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid_attribute =
> ID_SERIAL (config file default)
> >        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: no ID_SERIAL attribute
> >        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid = <empty> (udev)
> >        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: no ID_SERIAL attribute
> >        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: uid = <empty> (udev)
> >        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: failed to get path uid
> >        Jun 17 10:14:43 ictm-vader multipathd[474]: sda: orphan path, failed to
> add path
> >
> >   This patch handles this case by allowing to fall back to explict
> getuid_callout
> >   incase if ID_SERIAL attribute is not set. This way support for deprecated
> >   getuid_callout is not lost for older versions where ID_SERIAL attribute is
> not
> >   present but also serves good purpose in the scenario mentioned above.
> >
> > Signed-off-by: Shiva Krishna Merla<shivakrishna.merla@netapp.com>
> 
> Hmm. I have a similar patch in my tree. Will be checking.
>
Hannes, did you get a chance to verify this?. We are seeing many scenarios
where scsi_id call can fail during udev processing and path will not be added
to map until scsi_id is successful on future udev change events. Falling back to 
getuid_callout will solve this particular problem.