[dm-devel] dm_ioctl: Only send a change uevent when a resume ioctl changes the device.

[Mike Snitzer]

On Tue, Jan 26 2010 at  1:57pm -0500,
Dave Wysochanski <dwysocha at 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 at 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 at 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