[PATCH] multipathd: Don't mark a virtio_blk path offline if it has no sysfs "state" attribute

[PATCH] multipathd: Don't mark a virtio_blk path offline if it has no sysfs "state" attribute

[Martin Schwenke]

multipathd currently marks paths as down if they don't have a sysfs
"state" attribute.

Unfortunately, this makes multipathd unusable with Linux virtio_blk
devices, since they don't have this attribute.  I need to use multipath
with virtio for consistency with a real configuration when testing a
clustered NAS system - yeah, I'm that guy again...  :-)

One way of working around this might be to have path_offline() return
PATH_UP for all devices that don't have a sysfs "state" attribute,
instead of PATH_WILD.  However, I'm guessing the current behaviour might
exist for a reason.

The following patch makes path_offline() always return PATH_UP instead
of PATH_WILD for virtio_blk devices.  I've implemented the nested if
statements as below to change the code flow as little as possible
when sysfs_get_state() actually succeeds, which I assume is usually the
case.   If nobody is feeling paranoid then the check for virtio_blk
could obviously be done before the call to sysfs_get_state().

If people think this patch is too specific then at least it can be
used to start a discussion...  ;-)


commit fc2eb79ba3bca70702568cce73be9daffaf3f149
Author: Martin Schwenke <martin@meltin.net>
Date:   Thu May 5 14:39:29 2011 +1000

    Don't mark a virtio_blk path offline if it has no sysfs "state" attribute.
    
    Signed-off-by: Martin Schwenke <martin@meltin.net>

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index aa25cc4..1155f68 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -722,8 +722,15 @@ path_offline (struct path * pp)
 		return PATH_WILD;
 	}
 
-	if (sysfs_get_state(parent, buff, SCSI_STATE_SIZE))
-		return PATH_WILD;
+	if (sysfs_get_state(parent, buff, SCSI_STATE_SIZE)) {
+		if (!strncmp(parent->driver, "virtio_blk", 10)) {
+			condlog(3, "%s: virtio_blk is always up", pp->dev);
+			return PATH_UP;
+		} else {
+			condlog(3, "%s: failed to get \"state\"", pp->dev);
+			return PATH_WILD;
+		}
+	}
 
 	condlog(3, "%s: state = %s", pp->dev, buff);
 

peace & happiness,
martin

Re: [PATCH] multipathd: Don't mark a virtio_blk path offline if it has no sysfs "state" attribute

[Hannes Reinecke]

On 05/05/2011 08:32 AM, Martin Schwenke wrote:
> multipathd currently marks paths as down if they don't have a sysfs
> "state" attribute.
>
> Unfortunately, this makes multipathd unusable with Linux virtio_blk
> devices, since they don't have this attribute.  I need to use multipath
> with virtio for consistency with a real configuration when testing a
> clustered NAS system - yeah, I'm that guy again...  :-)
>
> One way of working around this might be to have path_offline() return
> PATH_UP for all devices that don't have a sysfs "state" attribute,
> instead of PATH_WILD.  However, I'm guessing the current behaviour might
> exist for a reason.
>
> The following patch makes path_offline() always return PATH_UP instead
> of PATH_WILD for virtio_blk devices.  I've implemented the nested if
> statements as below to change the code flow as little as possible
> when sysfs_get_state() actually succeeds, which I assume is usually the
> case.   If nobody is feeling paranoid then the check for virtio_blk
> could obviously be done before the call to sysfs_get_state().
>
> If people think this patch is too specific then at least it can be
> used to start a discussion...  ;-)
>
Argl. They messed it up _again_.
(Speaking as someone who is in the process of cleaning up his patch 
queue for multipath. Should have done that _far_ earlier :-( )

path_offline() _absolutely_ requires a check if the device is 
actually a SCSI device. We do have several others to contend with 
(cciss, dasd), and none of those do have the 'state' attribute.

So we need this patch

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 6cd2ec9..4af0cd3 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -645,6 +645,10 @@ path_offline (struct path * pp)
         struct sysfs_device * parent;
         char buff[SCSI_STATE_SIZE];

+       if (pp->bus != SYSFS_BUS_SCSI)
+               /* No information for non-SCSI devices, return UP */
+               return PATH_UP;
+
         pp->sysdev = sysfs_device_from_path(pp);
         if (!pp->sysdev) {
                 condlog(1, "%s: failed to get sysfs information", 
pp->dev);

And you need to implement virtio_blk as it's own bus type.
(Say SYSFS_BUS_VIRTIO). And teach discovery.c to detect this one 
properly.
Look at 'cciss' and 'dasd' on how that's done.

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] multipathd: Don't mark a virtio_blk path offline if it has no sysfs "state" attribute

[Martin Schwenke]

On Thu, 05 May 2011 09:02:07 +0200, Hannes Reinecke <hare@suse.de>
wrote:

> path_offline() _absolutely_ requires a check if the device is 
> actually a SCSI device. We do have several others to contend with 
> (cciss, dasd), and none of those do have the 'state' attribute.
> 
> So we need this patch
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 6cd2ec9..4af0cd3 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -645,6 +645,10 @@ path_offline (struct path * pp)
>          struct sysfs_device * parent;
>          char buff[SCSI_STATE_SIZE];
> 
> +       if (pp->bus != SYSFS_BUS_SCSI)
> +               /* No information for non-SCSI devices, return UP */
> +               return PATH_UP;
> +
>          pp->sysdev = sysfs_device_from_path(pp);
>          if (!pp->sysdev) {
>                  condlog(1, "%s: failed to get sysfs information", 
> pp->dev);


That sure works for me...  ;-)

> And you need to implement virtio_blk as it's own bus type.
> (Say SYSFS_BUS_VIRTIO). And teach discovery.c to detect this one 
> properly.
> Look at 'cciss' and 'dasd' on how that's done.

Have to?  Currently it all works!  :-)

Devices with bus SYSFS_BUS_UNDEF seem to work just fine - someone
seems to have made a conscious decision to make it so in the past. I
don't get a vendor/model or anything else very useful listed against
my paths, but multipath works fine...

Do you think that's an acceptable way of operating or should we
definitely put some code in to give virtio_blk a bus type?  My personal
opinion is that it is OK as it is.

Thanks...

peace & happiness,
martin

Re: [PATCH] multipathd: Don't mark a virtio_blk path offline if it has no sysfs "state" attribute

[Christophe Varoqui]

On jeu., 2011-05-05 at 16:32 +1000, Martin Schwenke wrote:
> multipathd currently marks paths as down if they don't have a sysfs
> "state" attribute.
> 
> Unfortunately, this makes multipathd unusable with Linux virtio_blk
> devices, since they don't have this attribute.  I need to use multipath
> with virtio for consistency with a real configuration when testing a
> clustered NAS system - yeah, I'm that guy again...  :-)
> 
> One way of working around this might be to have path_offline() return
> PATH_UP for all devices that don't have a sysfs "state" attribute,
> instead of PATH_WILD.  However, I'm guessing the current behaviour might
> exist for a reason.
> 
> The following patch makes path_offline() always return PATH_UP instead
> of PATH_WILD for virtio_blk devices.  I've implemented the nested if
> statements as below to change the code flow as little as possible
> when sysfs_get_state() actually succeeds, which I assume is usually the
> case.   If nobody is feeling paranoid then the check for virtio_blk
> could obviously be done before the call to sysfs_get_state().
> 
> If people think this patch is too specific then at least it can be
> used to start a discussion...  ;-)
> 
I'd personnaly prefer the un-nested version, and keep apply this
passthrough only for drivers who don't present the state file. Is there
other such drivers by the way ?

Best regards,
-- 
Christophe Varoqui
OpenSVC - Tools to scale
http://www.opensvc.com/