Re: [PATCH] nfs: emit a fsnotify_nameremove call in sillyrename codepath

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 13 Mar 2014 17:36:42 -0400
Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:

> 
> On Mar 13, 2014, at 17:26, Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
> 
> > On Thu, 2014-03-13 at 17:21 -0400, Jeff Layton wrote:
> >> On Thu, 13 Mar 2014 16:47:49 -0400
> >> Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
> >> 
> >>> 
> >>> On Mar 13, 2014, at 16:22, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >>> 
> >>>> On Thu, 13 Mar 2014 16:08:01 -0400
> >>>> Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
> >>>> 
> >>>>> 
> >>>>> On Mar 13, 2014, at 15:24, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >>>>> 
> >>>>>> If a file is sillyrenamed, then the generic vfs_unlink code will skip
> >>>>>> emitting fsnotify events for it.
> >>>>>> 
> >>>>>> This patch has the sillyrename code do that instead.
> >>>>>> 
> >>>>>> In truth this is a little bit odd since we aren't actually removing the
> >>>>>> dentry per-se, but renaming it. Still, this is probably the right thing
> >>>>>> to do since it's what userland apps expect to see when an unlink()
> >>>>>> occurs or some file is renamed on top of the dentry.
> >>>>>> 
> >>>>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> fs/nfs/dir.c    | 1 +
> >>>>>> fs/nfs/unlink.c | 2 ++
> >>>>>> 2 files changed, 3 insertions(+)
> >>>>>> 
> >>>>>> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> >>>>>> index 4a48fe4b84b6..591aec9b1719 100644
> >>>>>> --- a/fs/nfs/dir.c
> >>>>>> +++ b/fs/nfs/dir.c
> >>>>>> @@ -37,6 +37,7 @@
> >>>>>> #include <linux/sched.h>
> >>>>>> #include <linux/kmemleak.h>
> >>>>>> #include <linux/xattr.h>
> >>>>>> +#include <linux/fsnotify.h>
> >>>>>> 
> >>>>>> #include "delegation.h"
> >>>>>> #include "iostat.h"
> >>>>>> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
> >>>>>> index 11d78944de79..547ed0cd59db 100644
> >>>>>> --- a/fs/nfs/unlink.c
> >>>>>> +++ b/fs/nfs/unlink.c
> >>>>>> @@ -355,6 +355,8 @@ static void nfs_async_rename_done(struct rpc_task *task, void *calldata)
> >>>>>> 
> >>>>>> 	if (task->tk_status != 0)
> >>>>>> 		nfs_cancel_async_unlink(old_dentry);
> >>>>>> +	else if (old_dentry->d_flags & DCACHE_NFSFS_RENAMED)
> >>>>>> +		fsnotify_nameremove(old_dentry, S_ISDIR(old_dentry->d_inode->i_mode));
> >>>>>> }
> >>>>> 
> >>>>> Any reason why we shouldn’t just do this in nfs_sillyrename() itself?
> >>>>> 
> >>>> 
> >>>> We certainly could, but then you'd probably never get the event if the
> >>>> process waiting on the async sillyrename got killed while waiting on
> >>>> the reply.
> >>> 
> >>> Just send it anyway. The dentry will have been scheduled to be unlinked no matter whether or not the process is killed.
> >>> 
> >> 
> >> Hrm, I dunno...
> >> 
> >> If we do that then we may end up sending the event before it has
> >> actually occurred. I'm not sure if that's a problem or not, but it
> >> seems iffy.
> >> 
> >> I don't get it though -- why not do this in the rpc_call_done handler?
> >> If we do it there then we know we'll only send the event if the rename
> >> succeeded.
> > 
> > While nfs_async_rename() may currently reside in fs/nfs/unlink.c, the
> > function itself is completely independent of sillyrename. It doesn't
> > even share any common structures. Why should we start adding stuff which
> > has absolutely nothing to do with rename to it when we don't have to?
> 
> Put differently: if anyone out there is looking for something to do, then reuniting nfs_async_rename() with its long lost synchronous cousins would be a nice little cleanup.

Yeah, that's long overdue. I had originally meant to do that when I
added the async rename stuff, but got sidetracked...

In that case, we also have a bit of a "layering violation" with
nfs_cancel_async_unlink. It's probably not to hard to fix that with
YAFP (yet another function pointer).

Let's just drop this patch for now, and I'll have a look at this in the
near future and see if I can unify the rename code. Then we
can add the fsnotify stuff on top of that.

Thanks,
-- 
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux