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@xxxxxxxxxx>
> > ---
> > 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
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
