[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/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);

Why can't you just use mpp->dmi here?

> +       if (!dmi) /* assume we do need to reload the device */
> +               return true;

Why that? I'd assume that if a DM_DEVICE_INFO ioctl fails on the
device, a RELOAD would almost certainly fail, too.

> +       if (dmi->read_only == ro) {
> +               free(dmi);
> +               return false;
> +       }
> +       free(dmi);
> +       if (ro == 1)
> +               return true;
> +       vector_foreach_slot (mpp->pg, pgp, i) {
> +               vector_foreach_slot (pgp->paths, pp, j) {
> +                       if (sysfs_get_ro(pp) == 1)
> +                               return false;

I think you should also return false here if sysfs_get_ro() returns
error.

Regards,
Martin

> +               }
> +       }
> +       return true;
> +}
> +
>  static int
>  uev_update_path (struct uevent *uev, struct vectors * vecs)
>  {
> @@ -1512,7 +1541,7 @@ uev_update_path (struct uevent *uev, struct
> vectors * vecs)
>                 }
>  
>                 ro = uevent_get_disk_ro(uev);
> -               if (mpp && ro >= 0) {
> +               if (needs_ro_update(mpp, ro)) {
>                         condlog(2, "%s: update path write_protect to
> '%d' (uevent)", uev->kernel, ro);
>  
>                         if (mpp->wait_for_udev)