On Fri, Nov 19, 2021 at 09:33:39PM +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 rereads 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/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?
Since that value is set when the dmi is originally created, I didn't
want to not reload a map, if we simply haven't updated it yet to reflect
a change in the read-only value, like with do with dm_is_suspended()
or dm_get_deferred_remove(), etc. I could make a dm_get_read_only()
function and put it libmultipath/devmapper.c like the others, if you'd
rather.
> > + 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.
>
Since reloading when it's not necessary doesn't do any harm (it's what
we currently do) while not switching to read/write when we should is a
problem, I thought that I'd error on the side of caution, but I agree
that the reload is unlikey to succeed, so I can change this if you'd
like.
> > + 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.
Same thing here. I was erroring on the side of caution, but it should be
fine to change.
-Ben
> 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)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
