| Message ID | 1637275667-13436-1-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipathd: avoid unnecessary path read-only reloads | expand |
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@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) -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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@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. > > + 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@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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@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. > > > > + 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) > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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@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) > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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@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. 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@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Ben, some more thoughts about the ro handling. On Mon, 2021-11-22 at 20:39 +0100, Martin Wilck wrote: > 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: > > > > > > > > > > > > +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, This would need to be done on a change uevent for the dm device in ev_add_map(), but AFAICS we don't. ev_add_map() is basically a noop if the map is already known, unless wait_for_udev is 2. > > 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. I've never looked into the ro attribute processing closely. I just did. I'm unsure how a race would come to pass, in particular with your patch applied: 1. path change uevent arrives 2. ro attribute of path device has changed 3. map reload occurs if a) map was rw before (thus all paths, too) and the path changed to ro b) map was ro before and all paths have changed to rw 4. kernel will call set_disk_ro() depending on the DM_READONLY_FLAG; set_disk_ro() triggers an uevent for the block device if and only if the ro flag changed 5. we set mpp->dmi in __setup_multipath(). We hold the vecs lock between 3 and 5, so even if the uevent arrived before setup_multipath() was called, I don't see how it could race. mpp->dmi as derived in 5 should reflect the state correctly. What we could do is remember the ro-state of the map in dm_addmap(), e.g. in a mpp->ro field. If map creation with ro=0 succeeded, we can be pretty certain that the map is in read-write state. Otherwise we'd fallback to ro=1, and remember that state, too. We could verify that state once more against the dmi info in setup_multipath(). By doing that we'd cover the time span between the dm ioctl and retrieving the dmi in setup_multipath(). That would IMHO be more consistent than the current use of the temporary force_readonly flag. I've been wondering whether we should use your logic during map creation, too, and not even try to setup the map with ro=0 if we have paths in read-only state. Regards Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, Nov 23, 2021 at 11:05:20AM +0000, Martin Wilck wrote: > Hi Ben, > > some more thoughts about the ro handling. > > On Mon, 2021-11-22 at 20:39 +0100, Martin Wilck wrote: > > 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: > > > > > > > > > > > > > > +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, > > This would need to be done on a change uevent for the dm device in > ev_add_map(), but AFAICS we don't. ev_add_map() is basically a noop if > the map is already known, unless wait_for_udev is 2. > > > > 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. > > I've never looked into the ro attribute processing closely. I just did. > I'm unsure how a race would come to pass, in particular with your patch > applied: > > 1. path change uevent arrives > 2. ro attribute of path device has changed > 3. map reload occurs if > a) map was rw before (thus all paths, too) and the path changed to > ro > b) map was ro before and all paths have changed to rw > 4. kernel will call set_disk_ro() depending on the DM_READONLY_FLAG; > set_disk_ro() triggers an uevent for the block device if and only > if the ro flag changed > 5. we set mpp->dmi in __setup_multipath(). > > We hold the vecs lock between 3 and 5, so even if the uevent arrived > before setup_multipath() was called, I don't see how it could race. > mpp->dmi as derived in 5 should reflect the state correctly. I admit, I didn't find a definitive race. I was just worried about the possibility of the dmi being outdated. While there's always the possibility of the multipath device's RO state getting changed outside of multipathd (by a multipath call for example), this is not the place to deal with that. ev_add_map() would be. After looking at this, I'm o.k. with trusting the existing dmi, especially if we are updating it in ev_add_map(). > What we could do is remember the ro-state of the map in dm_addmap(), > e.g. in a mpp->ro field. If map creation with ro=0 succeeded, we can be > pretty certain that the map is in read-write state. Otherwise we'd > fallback to ro=1, and remember that state, too. We could verify that > state once more against the dmi info in setup_multipath(). By doing > that we'd cover the time span between the dm ioctl and retrieving the > dmi in setup_multipath(). That would IMHO be more consistent than the > current use of the temporary force_readonly flag. So the idea would be to never try reloading read-write when the map is marked as RO, until we get a path event updating the RO state? I do worry about cases where well fail to reload the map correctly then. Imagine that we have a map with mpp->ro=1 with one read-only path. The read-only path gets removed. If we just assume that the mpp->ro state is correct until with get a path_event changing the read-only state, we will won't reload read/write here. The other option would be to check the path's RO state every time we reload, or at least whenever we're reloading to remove a path. That has the advantage that it doesn't produce a dm error message like a failed reload does, but I'm not sure if it's any less work. Or am I misunderstanding what you are suggesting here? > I've been wondering whether we should use your logic during map > creation, too, and not even try to setup the map with ro=0 if we have > paths in read-only state. If sysfs says one of the paths is read-only, it seems reasonable to skip the read/write reload. -Ben > Regards > Martin > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, 2021-11-23 at 09:57 -0600, Benjamin Marzinski wrote: > On Tue, Nov 23, 2021 at 11:05:20AM +0000, Martin Wilck wrote: > > > > I've never looked into the ro attribute processing closely. I just > > did. > > I'm unsure how a race would come to pass, in particular with your > > patch > > applied: > > > > 1. path change uevent arrives > > 2. ro attribute of path device has changed > > 3. map reload occurs if > > a) map was rw before (thus all paths, too) and the path changed > > to > > ro > > b) map was ro before and all paths have changed to rw > > 4. kernel will call set_disk_ro() depending on the > > DM_READONLY_FLAG; > > set_disk_ro() triggers an uevent for the block device if and > > only > > if the ro flag changed > > 5. we set mpp->dmi in __setup_multipath(). > > > > We hold the vecs lock between 3 and 5, so even if the uevent > > arrived > > before setup_multipath() was called, I don't see how it could race. > > mpp->dmi as derived in 5 should reflect the state correctly. > > I admit, I didn't find a definitive race. I was just worried about > the > possibility of the dmi being outdated. While there's always the > possibility of the multipath device's RO state getting changed > outside > of multipathd (by a multipath call for example), this is not the > place > to deal with that. ev_add_map() would be. After looking at this, I'm > o.k. with trusting the existing dmi, especially if we are updating it > in > ev_add_map(). > > > What we could do is remember the ro-state of the map in > > dm_addmap(), > > e.g. in a mpp->ro field. If map creation with ro=0 succeeded, we > > can be > > pretty certain that the map is in read-write state. Otherwise we'd > > fallback to ro=1, and remember that state, too. We could verify > > that > > state once more against the dmi info in setup_multipath(). By doing > > that we'd cover the time span between the dm ioctl and retrieving > > the > > dmi in setup_multipath(). That would IMHO be more consistent than > > the > > current use of the temporary force_readonly flag. > > So the idea would be to never try reloading read-write when the map > is > marked as RO, until we get a path event updating the RO state? I do > worry about cases where well fail to reload the map correctly then. > Imagine that we have a map with mpp->ro=1 with one read-only path. > The > read-only path gets removed. If we just assume that the mpp->ro state > is > correct until with get a path_event changing the read-only state, we > will won't reload read/write here. I didn't think about this possibility. TBH, it sounds pretty pathological to me. > The other option would be to check > the path's RO state every time we reload, or at least whenever we're > reloading to remove a path. That has the advantage that it doesn't > produce a dm error message like a failed reload does, but I'm not > sure > if it's any less work. Or am I misunderstanding what you are > suggesting > here? Hm, I guess not. I'm worried that that may slow down reload operations quite a bit. Under normal circumstances, all paths will be r/w most of the time (in my experience, read-only multipath storage is pretty rare). The idea to re-check this only on path removal sounds ok-ish. We could also track the ro attribute of paths, so that we don't have to check sysfs every time.... But before things get overly complicated, trying to load r/w first and checking -EROFS like we do now, and doing the sysfs check when we receive uevents, is just as good, if not better. > > I've been wondering whether we should use your logic during map > > creation, too, and not even try to setup the map with ro=0 if we > > have > > paths in read-only state. > > If sysfs says one of the paths is read-only, it seems reasonable to > skip > the read/write reload. The only problem being that we'll have to check all paths in almost every case. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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@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/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. > + 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(). > 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. 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@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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@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/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@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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: > > +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. Actually this is just the way sysfs_attr_get_value() works. You have to provide a larger buffer than you will read (in this case, ro will only ever have two bytes), otherwise it will think that it overflowed. sysfs_attr_get_value() also does the NULL termination itself. -Ben > > + 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(). > > > > 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. > > 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@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Tue, 2021-12-14 at 18:49 -0600, Benjamin Marzinski wrote: > 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: > > > +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. > > Actually this is just the way sysfs_attr_get_value() works. You have > to > provide a larger buffer than you will read (in this case, ro will > only > ever have two bytes), otherwise it will think that it overflowed. > sysfs_attr_get_value() also does the NULL termination itself. > Well, fine then. It's a nit anyway. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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) { + condlog(3, "%s: Cannot parse ro attribute", pp->dev); + return -1; + } + + return ro; +} + 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); + if (!dmi) /* assume we do need to reload the device */ + return true; + 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; + } + } + 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)
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@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(-)