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

[Benjamin Marzinski]

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 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?
> > 
> > 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.

> >  
> > > > +       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.
> 
> I misunderstood what being "cautious" means in this context. I gather
> now that it's "if in doubt, reload". Fine with me.
> 
> > 
> > > > +       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.
> 
> Same misunderstanding on my part. Thanks for the clarification.
> 
> Regards
> Martin
> 
> > 
> > -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)
> >