* [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.