[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:
> > +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.

Actually this is just the way sysfs_attr_get_value() works. You have to
provide a larger buffer than you will read (in this case, ro will only
ever have two bytes), otherwise it will think that it overflowed.
sysfs_attr_get_value() also does the NULL termination itself.

-Ben

> > +               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().
> 
> 
> >  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.
> 
> 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