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

Commit Message

Hannes Reinecke May 5, 2011, 7:02 a.m. UTC

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

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

Comments

Martin Schwenke May 5, 2011, 7:37 a.m. UTC | #1

On Thu, 05 May 2011 09:02:07 +0200, Hannes Reinecke <[e-mail]>
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
--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

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",