On Mon, 2021-11-22 at 11:43 -0600, Benjamin Marzinski wrote:
> On Mon, Nov 22, 2021 at 04:48:06PM +0000, Martin Wilck wrote:
> > On Mon, 2021-11-22 at 09:35 -0600, Benjamin Marzinski wrote:
> > > 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.
> >
> > I had expected that this property wouldn't silently change under
> > us.
> > Actually, I do think that we should get an uevent if this happens.
> > Not sure if we process it properly, though.
>
> I think we will update the dmi, but I'm not sure that these uevents
> won't race. The worry was that the device would switch to read-only
> and
> then back too quickly, and we would get this event and still see the
> device in read/write because we haven't processed the event which
> would
> update the multipath dmi.
OK. I'm fine with the patch, perhaps explain these subtleties some more
in the commit message for future reference.
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
