[dm-devel] [PATCH] multipathd: avoid unnecessary path read-only reloads

[Benjamin Marzinski]

On Fri, Dec 03, 2021 at 08:25:00AM +0000, Martin Wilck wrote:
> On Thu, 2021-11-18 at 16:47 -0600, Benjamin Marzinski wrote:
> > A mulitpath device can only be reloaded read/write when all paths are
> > read/write. Also, whenever a read-only device is rescanned, the scsi
> > subsystem will first unconditionally issue a uevent with DISK_RO=0
> > before checking the read-only status, and if it the device is still
> > read-only, issuing another uevent with DISK_RO=1. These uevents cause
> > pointless reloads when read-only paths are rescanned. To avoid this,
> > check to see if all paths are read/write before changing a multipath
> > device from read-only to read/write.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > ---
> >  libmultipath/libmultipath.version |  5 +++++
> >  libmultipath/sysfs.c              | 22 ++++++++++++++++++++++
> >  libmultipath/sysfs.h              |  1 +
> >  multipathd/main.c                 | 31
> > ++++++++++++++++++++++++++++++-
> >  4 files changed, 58 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libmultipath/libmultipath.version
> > b/libmultipath/libmultipath.version
> > index 58a7d1be..ab4c7e30 100644
> > --- a/libmultipath/libmultipath.version
> > +++ b/libmultipath/libmultipath.version
> > @@ -296,3 +296,8 @@ global:
> >  local:
> >         *;
> >  };
> > +
> > +LIBMULTIPATH_11.1.0 {
> > +global:
> > +       sysfs_get_ro;
> > +} LIBMULTIPATH_11.0.0;
> > diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
> > index 9ff145f2..24c12b6a 100644
> > --- a/libmultipath/sysfs.c
> > +++ b/libmultipath/sysfs.c
> > @@ -236,6 +236,28 @@ sysfs_get_size (struct path *pp, unsigned long
> > long * size)
> >         return 0;
> >  }
> >  
> > +int
> > +sysfs_get_ro (struct path *pp)
> > +{
> > +       int ro;
> > +       char buff[3]; /* Either "0\n\0" or "1\n\0" */
> > +
> > +       if (!pp->udev)
> > +               return -1;
> > +
> > +       if (sysfs_attr_get_value(pp->udev, "ro", buff, sizeof(buff))
> > <= 0) {
> > +               condlog(3, "%s: Cannot read ro attribute in sysfs",
> > pp->dev);
> > +               return -1;
> > +       }
> > +
> > +       if (sscanf(buff, "%d\n", &ro) != 1 || ro < 0 || ro > 1) {
> 
> This is ok, but for self-evidently correct code in multipath-tools,
> it'd be better to read just 2 bytes and set buff[2] = '\0' explicitly.
> I haven't checked, but coverity might stumble on this.

Sure.
 
> > +               condlog(3, "%s: Cannot parse ro attribute", pp->dev);
> > +               return -1;
> > +       }
> > +
> > +       return ro;
> > +}
> > +
> 
> Does this function need to be in libmultipath? We could avoid extending
> the ABI  by adding it as a static function to multipathd. After all,
> it's just sysfs_attr_get_value() + sscanf().
>

Sure.

> 
> >  int sysfs_check_holders(char * check_devt, char * new_devt)
> >  {
> >         unsigned int major, new_minor, table_minor;
> > diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
> > index 72b39ab2..c948c467 100644
> > --- a/libmultipath/sysfs.h
> > +++ b/libmultipath/sysfs.h
> > @@ -13,6 +13,7 @@ ssize_t sysfs_attr_get_value(struct udev_device
> > *dev, const char *attr_name,
> >  ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char
> > *attr_name,
> >                                  unsigned char * value, size_t
> > value_len);
> >  int sysfs_get_size (struct path *pp, unsigned long long * size);
> > +int sysfs_get_ro(struct path *pp);
> >  int sysfs_check_holders(char * check_devt, char * new_devt);
> >  bool sysfs_is_multipathed(struct path *pp, bool set_wwid);
> >  #endif
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 08a8a592..a1665494 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -1440,6 +1440,35 @@ finish_path_init(struct path *pp, struct
> > vectors * vecs)
> >         return -1;
> >  }
> >  
> > +static bool
> > +needs_ro_update(struct multipath *mpp, int ro)
> > +{
> > +       struct pathgroup * pgp;
> > +       struct path * pp;
> > +       unsigned int i, j;
> > +       struct dm_info *dmi = NULL;
> > +
> > +       if (!mpp || ro < 0)
> > +               return false;
> > +       dm_get_info(mpp->alias, &dmi);
> 
> I'm still not convinced that we need this call, in particular as you
> have added a call to dm_get_info() in the uev_update_map() call, and it
> was called in the dmevent handler, too.

You are reviewing my original patch again here, not the V2 version,
which has this removed.

-Ben


> In general, I think that multipath-tools could rely more on the kernel
> to send events for status changes than it currently does. 
> This is particularly true for dm properties, where we have not only one
> event mechanism but two (uevent + dmevent).
> 
> Regards
> Martin