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

[Martin Wilck]

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.

> +               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