* [PATCH] dm_ioctl: Only send a change uevent when a resume ioctl changes the device. @ 2010-01-26 17:56 Dave Wysochanski 2010-01-26 18:09 ` Mike Snitzer 0 siblings, 1 reply; 6+ messages in thread From: Dave Wysochanski @ 2010-01-26 17:56 UTC (permalink / raw) To: dm-devel Resume ioctls may result in no changes to the device and thus they should not generate change uevents. For example, if a device is not suspended when a resume ioctl occurs, we should not send a uevent. Signed-off-by: Dave Wysochanski <dwysocha@redhat.com> --- drivers/md/dm-ioctl.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 1d66932..0c1cf53 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -851,6 +851,7 @@ static int do_suspend(struct dm_ioctl *param) static int do_resume(struct dm_ioctl *param) { + int send_uevent = 0; int r = 0; unsigned suspend_flags = DM_SUSPEND_LOCKFS_FLAG; struct hash_cell *hc; @@ -895,18 +896,21 @@ static int do_resume(struct dm_ioctl *param) set_disk_ro(dm_disk(md), 0); else set_disk_ro(dm_disk(md), 1); + send_uevent = 1; } - - if (dm_suspended_md(md)) + if (dm_suspended_md(md)) { r = dm_resume(md); + send_uevent = 1; + } if (old_map) dm_table_destroy(old_map); - if (!r) { + if (send_uevent) dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); + + if (!r) r = __dev_status(md, param); - } dm_put(md); return r; -- 1.6.5.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: dm_ioctl: Only send a change uevent when a resume ioctl changes the device. 2010-01-26 17:56 [PATCH] dm_ioctl: Only send a change uevent when a resume ioctl changes the device Dave Wysochanski @ 2010-01-26 18:09 ` Mike Snitzer 2010-01-26 18:48 ` Dave Wysochanski 0 siblings, 1 reply; 6+ messages in thread From: Mike Snitzer @ 2010-01-26 18:09 UTC (permalink / raw) To: device-mapper development On Tue, Jan 26 2010 at 12:56pm -0500, Dave Wysochanski <dwysocha@redhat.com> wrote: > Resume ioctls may result in no changes to the device and thus they > should not generate change uevents. For example, if a device is > not suspended when a resume ioctl occurs, we should not send a > uevent. > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com> > --- > drivers/md/dm-ioctl.c | 12 ++++++++---- > 1 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > index 1d66932..0c1cf53 100644 > --- a/drivers/md/dm-ioctl.c > +++ b/drivers/md/dm-ioctl.c > @@ -851,6 +851,7 @@ static int do_suspend(struct dm_ioctl *param) > > static int do_resume(struct dm_ioctl *param) > { > + int send_uevent = 0; > int r = 0; > unsigned suspend_flags = DM_SUSPEND_LOCKFS_FLAG; > struct hash_cell *hc; > @@ -895,18 +896,21 @@ static int do_resume(struct dm_ioctl *param) > set_disk_ro(dm_disk(md), 0); > else > set_disk_ro(dm_disk(md), 1); > + send_uevent = 1; > } > - > - if (dm_suspended_md(md)) > + if (dm_suspended_md(md)) { > r = dm_resume(md); > + send_uevent = 1; > + } > > if (old_map) > dm_table_destroy(old_map); > > - if (!r) { > + if (send_uevent) > dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > + > + if (!r) > r = __dev_status(md, param); > - } > > dm_put(md); > return r; Seems you're sending the uevent even if resume failed. Is that intended? Also, can dm_kobject_uevent() come before dm_table_destroy()? If so this would be a bit simpler: diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c index 1d66932..e3cf568 100644 --- a/drivers/md/dm-ioctl.c +++ b/drivers/md/dm-ioctl.c @@ -897,16 +897,17 @@ static int do_resume(struct dm_ioctl *param) set_disk_ro(dm_disk(md), 1); } - if (dm_suspended_md(md)) + if (dm_suspended_md(md)) { r = dm_resume(md); + if (!r) + dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); + } if (old_map) dm_table_destroy(old_map); - if (!r) { - dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); + if (!r) r = __dev_status(md, param); - } dm_put(md); return r; ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: dm_ioctl: Only send a change uevent when a resume ioctl changes the device. 2010-01-26 18:09 ` Mike Snitzer @ 2010-01-26 18:48 ` Dave Wysochanski 2010-01-26 18:57 ` Dave Wysochanski 0 siblings, 1 reply; 6+ messages in thread From: Dave Wysochanski @ 2010-01-26 18:48 UTC (permalink / raw) To: device-mapper development On Tue, 2010-01-26 at 13:09 -0500, Mike Snitzer wrote: > On Tue, Jan 26 2010 at 12:56pm -0500, > Dave Wysochanski <dwysocha@redhat.com> wrote: > > > Resume ioctls may result in no changes to the device and thus they > > should not generate change uevents. For example, if a device is > > not suspended when a resume ioctl occurs, we should not send a > > uevent. > > > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com> > > --- > > drivers/md/dm-ioctl.c | 12 ++++++++---- > > 1 files changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > > index 1d66932..0c1cf53 100644 > > --- a/drivers/md/dm-ioctl.c > > +++ b/drivers/md/dm-ioctl.c > > @@ -851,6 +851,7 @@ static int do_suspend(struct dm_ioctl *param) > > > > static int do_resume(struct dm_ioctl *param) > > { > > + int send_uevent = 0; > > int r = 0; > > unsigned suspend_flags = DM_SUSPEND_LOCKFS_FLAG; > > struct hash_cell *hc; > > @@ -895,18 +896,21 @@ static int do_resume(struct dm_ioctl *param) > > set_disk_ro(dm_disk(md), 0); > > else > > set_disk_ro(dm_disk(md), 1); > > + send_uevent = 1; > > } > > - > > - if (dm_suspended_md(md)) > > + if (dm_suspended_md(md)) { > > r = dm_resume(md); > > + send_uevent = 1; > > + } > > > > if (old_map) > > dm_table_destroy(old_map); > > > > - if (!r) { > > + if (send_uevent) > > dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > > + > > + if (!r) > > r = __dev_status(md, param); > > - } > > > > dm_put(md); > > return r; > > Seems you're sending the uevent even if resume failed. Is that intended? > > Also, can dm_kobject_uevent() come before dm_table_destroy()? > > If so this would be a bit simpler: > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > index 1d66932..e3cf568 100644 > --- a/drivers/md/dm-ioctl.c > +++ b/drivers/md/dm-ioctl.c > @@ -897,16 +897,17 @@ static int do_resume(struct dm_ioctl *param) > set_disk_ro(dm_disk(md), 1); > } > > - if (dm_suspended_md(md)) > + if (dm_suspended_md(md)) { > r = dm_resume(md); > + if (!r) > + dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > + } > > if (old_map) > dm_table_destroy(old_map); > > - if (!r) { > - dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > + if (!r) > r = __dev_status(md, param); > - } > > dm_put(md); > return r; > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel Nice simplification Mike, as discussed on IRC. Ack. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dm_ioctl: Only send a change uevent when a resume ioctl changes the device. 2010-01-26 18:48 ` Dave Wysochanski @ 2010-01-26 18:57 ` Dave Wysochanski 2010-01-26 21:12 ` Mike Snitzer 0 siblings, 1 reply; 6+ messages in thread From: Dave Wysochanski @ 2010-01-26 18:57 UTC (permalink / raw) To: device-mapper development On Tue, 2010-01-26 at 13:48 -0500, Dave Wysochanski wrote: > On Tue, 2010-01-26 at 13:09 -0500, Mike Snitzer wrote: > > On Tue, Jan 26 2010 at 12:56pm -0500, > > Dave Wysochanski <dwysocha@redhat.com> wrote: > > > > > Resume ioctls may result in no changes to the device and thus they > > > should not generate change uevents. For example, if a device is > > > not suspended when a resume ioctl occurs, we should not send a > > > uevent. > > > > > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com> > > > --- > > > drivers/md/dm-ioctl.c | 12 ++++++++---- > > > 1 files changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > > > index 1d66932..0c1cf53 100644 > > > --- a/drivers/md/dm-ioctl.c > > > +++ b/drivers/md/dm-ioctl.c > > > @@ -851,6 +851,7 @@ static int do_suspend(struct dm_ioctl *param) > > > > > > static int do_resume(struct dm_ioctl *param) > > > { > > > + int send_uevent = 0; > > > int r = 0; > > > unsigned suspend_flags = DM_SUSPEND_LOCKFS_FLAG; > > > struct hash_cell *hc; > > > @@ -895,18 +896,21 @@ static int do_resume(struct dm_ioctl *param) > > > set_disk_ro(dm_disk(md), 0); > > > else > > > set_disk_ro(dm_disk(md), 1); > > > + send_uevent = 1; > > > } > > > - > > > - if (dm_suspended_md(md)) > > > + if (dm_suspended_md(md)) { > > > r = dm_resume(md); > > > + send_uevent = 1; > > > + } > > > > > > if (old_map) > > > dm_table_destroy(old_map); > > > > > > - if (!r) { > > > + if (send_uevent) > > > dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > > > + > > > + if (!r) > > > r = __dev_status(md, param); > > > - } > > > > > > dm_put(md); > > > return r; > > > > Seems you're sending the uevent even if resume failed. Is that intended? > > > > Also, can dm_kobject_uevent() come before dm_table_destroy()? > > > > If so this would be a bit simpler: > > > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > > index 1d66932..e3cf568 100644 > > --- a/drivers/md/dm-ioctl.c > > +++ b/drivers/md/dm-ioctl.c > > @@ -897,16 +897,17 @@ static int do_resume(struct dm_ioctl *param) > > set_disk_ro(dm_disk(md), 1); > > } > > > > - if (dm_suspended_md(md)) > > + if (dm_suspended_md(md)) { > > r = dm_resume(md); > > + if (!r) > > + dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > > + } > > > > if (old_map) > > dm_table_destroy(old_map); > > > > - if (!r) { > > - dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > > + if (!r) > > r = __dev_status(md, param); > > - } > > > > dm_put(md); > > return r; > > > > -- > > dm-devel mailing list > > dm-devel@redhat.com > > https://www.redhat.com/mailman/listinfo/dm-devel > > Nice simplification Mike, as discussed on IRC. > > Ack. > Actually, the more I think about it I am not sure about sending the uevent in the case where dm_resume() fails. Seems like we should send it since the map could have changed - depends on whether we went through that 'if (new_map)' block. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dm_ioctl: Only send a change uevent when a resume ioctl changes the device. 2010-01-26 18:57 ` Dave Wysochanski @ 2010-01-26 21:12 ` Mike Snitzer 2010-01-26 22:55 ` Dave Wysochanski 0 siblings, 1 reply; 6+ messages in thread From: Mike Snitzer @ 2010-01-26 21:12 UTC (permalink / raw) To: device-mapper development On Tue, Jan 26 2010 at 1:57pm -0500, Dave Wysochanski <dwysocha@redhat.com> wrote: > On Tue, 2010-01-26 at 13:48 -0500, Dave Wysochanski wrote: > > On Tue, 2010-01-26 at 13:09 -0500, Mike Snitzer wrote: > > > On Tue, Jan 26 2010 at 12:56pm -0500, > > > Dave Wysochanski <dwysocha@redhat.com> wrote: > > > > > > > Resume ioctls may result in no changes to the device and thus they > > > > should not generate change uevents. For example, if a device is > > > > not suspended when a resume ioctl occurs, we should not send a > > > > uevent. > > > > > > > > Signed-off-by: Dave Wysochanski <dwysocha@redhat.com> > > > > --- > > > > drivers/md/dm-ioctl.c | 12 ++++++++---- > > > > 1 files changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > > > > index 1d66932..0c1cf53 100644 > > > > --- a/drivers/md/dm-ioctl.c > > > > +++ b/drivers/md/dm-ioctl.c > > > > @@ -851,6 +851,7 @@ static int do_suspend(struct dm_ioctl *param) > > > > > > > > static int do_resume(struct dm_ioctl *param) > > > > { > > > > + int send_uevent = 0; > > > > int r = 0; > > > > unsigned suspend_flags = DM_SUSPEND_LOCKFS_FLAG; > > > > struct hash_cell *hc; > > > > @@ -895,18 +896,21 @@ static int do_resume(struct dm_ioctl *param) > > > > set_disk_ro(dm_disk(md), 0); > > > > else > > > > set_disk_ro(dm_disk(md), 1); > > > > + send_uevent = 1; > > > > } > > > > - > > > > - if (dm_suspended_md(md)) > > > > + if (dm_suspended_md(md)) { > > > > r = dm_resume(md); > > > > + send_uevent = 1; > > > > + } > > > > > > > > if (old_map) > > > > dm_table_destroy(old_map); > > > > > > > > - if (!r) { > > > > + if (send_uevent) > > > > dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > > > > + > > > > + if (!r) > > > > r = __dev_status(md, param); > > > > - } > > > > > > > > dm_put(md); > > > > return r; > > > > > > Seems you're sending the uevent even if resume failed. Is that intended? > > > > > > Also, can dm_kobject_uevent() come before dm_table_destroy()? > > > > > > If so this would be a bit simpler: > > > > > > diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c > > > index 1d66932..e3cf568 100644 > > > --- a/drivers/md/dm-ioctl.c > > > +++ b/drivers/md/dm-ioctl.c > > > @@ -897,16 +897,17 @@ static int do_resume(struct dm_ioctl *param) > > > set_disk_ro(dm_disk(md), 1); > > > } > > > > > > - if (dm_suspended_md(md)) > > > + if (dm_suspended_md(md)) { > > > r = dm_resume(md); > > > + if (!r) > > > + dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > > > + } > > > > > > if (old_map) > > > dm_table_destroy(old_map); > > > > > > - if (!r) { > > > - dm_kobject_uevent(md, KOBJ_CHANGE, param->event_nr); > > > + if (!r) > > > r = __dev_status(md, param); > > > - } > > > > > > dm_put(md); > > > return r; > > > > > > > Nice simplification Mike, as discussed on IRC. > > > > Ack. > > > > Actually, the more I think about it I am not sure about sending the > uevent in the case where dm_resume() fails. Seems like we should send > it since the map could have changed - depends on whether we went through > that 'if (new_map)' block. Pretty sure we resolved this. As we discussed (just after you sent this last reply); for the benefit of others: <dave> so the change uevent here really means "resume was successful" <mike> yes <dave> wait - if we suspend it, but resume fails, shouldn't we still send the event? <mike> in that case the device will be left suspended; in terms of udev that's not a "change" <mike> device is wedged (suspended) <mike> telling udev about it could just stack up other processes trying to issue IO to it <mike> but to be honest I don't know what the purist definition of what a "change" uevent is; I'm just treating it as telling some blackbox that DM device has changed and is functional <dave> that does make sense ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: dm_ioctl: Only send a change uevent when a resume ioctl changes the device. 2010-01-26 21:12 ` Mike Snitzer @ 2010-01-26 22:55 ` Dave Wysochanski 0 siblings, 0 replies; 6+ messages in thread From: Dave Wysochanski @ 2010-01-26 22:55 UTC (permalink / raw) To: device-mapper development On Tue, 2010-01-26 at 16:12 -0500, Mike Snitzer wrote: > > > > > > > > > > Nice simplification Mike, as discussed on IRC. > > > > > > Ack. > > > > > > > Actually, the more I think about it I am not sure about sending the > > uevent in the case where dm_resume() fails. Seems like we should send > > it since the map could have changed - depends on whether we went through > > that 'if (new_map)' block. > > Pretty sure we resolved this. As we discussed (just after you sent this > last reply); for the benefit of others: > > <dave> so the change uevent here really means "resume was successful" > <mike> yes > <dave> wait - if we suspend it, but resume fails, shouldn't we still > send the event? > <mike> in that case the device will be left suspended; in terms of udev > that's not a "change" > <mike> device is wedged (suspended) > <mike> telling udev about it could just stack up other processes trying > to issue IO to it > <mike> but to be honest I don't know what the purist definition of what > a "change" uevent is; I'm just treating it as telling some blackbox that > DM device has changed and is functional > <dave> that does make sense > Yes, I should have stopped with my original Ack! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-01-26 22:55 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-26 17:56 [PATCH] dm_ioctl: Only send a change uevent when a resume ioctl changes the device Dave Wysochanski 2010-01-26 18:09 ` Mike Snitzer 2010-01-26 18:48 ` Dave Wysochanski 2010-01-26 18:57 ` Dave Wysochanski 2010-01-26 21:12 ` Mike Snitzer 2010-01-26 22:55 ` Dave Wysochanski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.