From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Snitzer Subject: Re: dm_ioctl: Only send a change uevent when a resume ioctl changes the device. Date: Tue, 26 Jan 2010 16:12:19 -0500 Message-ID: <20100126211219.GC17190@redhat.com> References: <1264528570-10498-1-git-send-email-dwysocha@redhat.com> <20100126180929.GA17190@redhat.com> <1264531714.2572.6.camel@f10-node1> <1264532220.2572.8.camel@f10-node1> Reply-To: device-mapper development Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1264532220.2572.8.camel@f10-node1> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: device-mapper development List-Id: dm-devel.ids On Tue, Jan 26 2010 at 1:57pm -0500, Dave Wysochanski 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 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 > > > > --- > > > > 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: so the change uevent here really means "resume was successful" yes wait - if we suspend it, but resume fails, shouldn't we still send the event? in that case the device will be left suspended; in terms of udev that's not a "change" device is wedged (suspended) telling udev about it could just stack up other processes trying to issue IO to it 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 that does make sense