| Message ID | 20190526143411.11244-4-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sort out fsnotify_nameremove() mess | expand |
On Sun 26-05-19 17:34:04, Amir Goldstein wrote: > This will allow generating fsnotify delete events after the > fsnotify_nameremove() hook is removed from d_delete(). > > Cc: Trond Myklebust <trond.myklebust@hammerspace.com> > Cc: Anna Schumaker <anna.schumaker@netapp.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > net/sunrpc/rpc_pipe.c | 4 ++++ > 1 file changed, 4 insertions(+) Hum, I don't think all __rpc_depopulate() calls are guarded by i_rwsem (e.g. those in rpc_gssd_dummy_populate()). Why aren't we using rpc_depopulate() in those cases? Trond, Anna? Honza > > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c > index 979d23646e33..917c85f15a0b 100644 > --- a/net/sunrpc/rpc_pipe.c > +++ b/net/sunrpc/rpc_pipe.c > @@ -597,6 +597,8 @@ static int __rpc_rmdir(struct inode *dir, struct dentry *dentry) > > dget(dentry); > ret = simple_rmdir(dir, dentry); > + if (!ret) > + fsnotify_rmdir(dir, dentry); > d_delete(dentry); > dput(dentry); > return ret; > @@ -608,6 +610,8 @@ static int __rpc_unlink(struct inode *dir, struct dentry *dentry) > > dget(dentry); > ret = simple_unlink(dir, dentry); > + if (!ret) > + fsnotify_unlink(dir, dentry); > d_delete(dentry); > dput(dentry); > return ret; > -- > 2.17.1 >
On Mon, May 27, 2019 at 1:54 PM Jan Kara <jack@suse.cz> wrote: > > On Sun 26-05-19 17:34:04, Amir Goldstein wrote: > > This will allow generating fsnotify delete events after the > > fsnotify_nameremove() hook is removed from d_delete(). > > > > Cc: Trond Myklebust <trond.myklebust@hammerspace.com> > > Cc: Anna Schumaker <anna.schumaker@netapp.com> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > net/sunrpc/rpc_pipe.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > Hum, I don't think all __rpc_depopulate() calls are guarded by i_rwsem > (e.g. those in rpc_gssd_dummy_populate()). Why aren't we using > rpc_depopulate() in those cases? Trond, Anna? > Do we care? For fsnotify hook, we should only care that d_parent/d_name are stable. They are stable because rpc_pipefs has no rename. Thanks, Amir.
On Mon 27-05-19 16:26:03, Amir Goldstein wrote: > On Mon, May 27, 2019 at 1:54 PM Jan Kara <jack@suse.cz> wrote: > > > > On Sun 26-05-19 17:34:04, Amir Goldstein wrote: > > > This will allow generating fsnotify delete events after the > > > fsnotify_nameremove() hook is removed from d_delete(). > > > > > > Cc: Trond Myklebust <trond.myklebust@hammerspace.com> > > > Cc: Anna Schumaker <anna.schumaker@netapp.com> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > net/sunrpc/rpc_pipe.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > Hum, I don't think all __rpc_depopulate() calls are guarded by i_rwsem > > (e.g. those in rpc_gssd_dummy_populate()). Why aren't we using > > rpc_depopulate() in those cases? Trond, Anna? > > > > Do we care? For fsnotify hook, we should only care that > d_parent/d_name are stable. > They are stable because rpc_pipefs has no rename. Yeah, good point. Probably we don't. I was just wondering... Honza
Hi Trond,Anna,Bruce, Seems that rpc_pipefs is co-maintained by client and server trees, so not sure who is the best to ask for an ack. We need to add those explicit fsnotify hooks to match the existing fsnotify_create/mkdir hooks, because the hook embedded inside d_delete() is going away [1]. Thanks, Amir. [1] https://lore.kernel.org/linux-fsdevel/20190526143411.11244-1-amir73il@gmail.com/ On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote: > > This will allow generating fsnotify delete events after the > fsnotify_nameremove() hook is removed from d_delete(). > > Cc: Trond Myklebust <trond.myklebust@hammerspace.com> > Cc: Anna Schumaker <anna.schumaker@netapp.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > net/sunrpc/rpc_pipe.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c > index 979d23646e33..917c85f15a0b 100644 > --- a/net/sunrpc/rpc_pipe.c > +++ b/net/sunrpc/rpc_pipe.c > @@ -597,6 +597,8 @@ static int __rpc_rmdir(struct inode *dir, struct dentry *dentry) > > dget(dentry); > ret = simple_rmdir(dir, dentry); > + if (!ret) > + fsnotify_rmdir(dir, dentry); > d_delete(dentry); > dput(dentry); > return ret; > @@ -608,6 +610,8 @@ static int __rpc_unlink(struct inode *dir, struct dentry *dentry) > > dget(dentry); > ret = simple_unlink(dir, dentry); > + if (!ret) > + fsnotify_unlink(dir, dentry); > d_delete(dentry); > dput(dentry); > return ret; > -- > 2.17.1 >
On Thu, 2019-05-30 at 08:43 +0300, Amir Goldstein wrote: > Hi Trond,Anna,Bruce, > > Seems that rpc_pipefs is co-maintained by client and server trees, so > not sure who is the best to ask for an ack. > We need to add those explicit fsnotify hooks to match the existing > fsnotify_create/mkdir hooks, because > the hook embedded inside d_delete() is going away [1]. > > Thanks, > Amir. > > [1] > https://lore.kernel.org/linux-fsdevel/20190526143411.11244-1-amir73il@gmail.com/ > > On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> > wrote: > > This will allow generating fsnotify delete events after the > > fsnotify_nameremove() hook is removed from d_delete(). > > > > Cc: Trond Myklebust <trond.myklebust@hammerspace.com> > > Cc: Anna Schumaker <anna.schumaker@netapp.com> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > net/sunrpc/rpc_pipe.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c > > index 979d23646e33..917c85f15a0b 100644 > > --- a/net/sunrpc/rpc_pipe.c > > +++ b/net/sunrpc/rpc_pipe.c > > @@ -597,6 +597,8 @@ static int __rpc_rmdir(struct inode *dir, > > struct dentry *dentry) > > > > dget(dentry); > > ret = simple_rmdir(dir, dentry); > > + if (!ret) > > + fsnotify_rmdir(dir, dentry); > > d_delete(dentry); > > dput(dentry); > > return ret; > > @@ -608,6 +610,8 @@ static int __rpc_unlink(struct inode *dir, > > struct dentry *dentry) > > > > dget(dentry); > > ret = simple_unlink(dir, dentry); > > + if (!ret) > > + fsnotify_unlink(dir, dentry); > > d_delete(dentry); > > dput(dentry); > > return ret; > > -- > > 2.17.1 > > This looks just fine to me. Acked-by: Trond Myklebust <trond.myklebust@hammerspace.com>
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c index 979d23646e33..917c85f15a0b 100644 --- a/net/sunrpc/rpc_pipe.c +++ b/net/sunrpc/rpc_pipe.c @@ -597,6 +597,8 @@ static int __rpc_rmdir(struct inode *dir, struct dentry *dentry) dget(dentry); ret = simple_rmdir(dir, dentry); + if (!ret) + fsnotify_rmdir(dir, dentry); d_delete(dentry); dput(dentry); return ret; @@ -608,6 +610,8 @@ static int __rpc_unlink(struct inode *dir, struct dentry *dentry) dget(dentry); ret = simple_unlink(dir, dentry); + if (!ret) + fsnotify_unlink(dir, dentry); d_delete(dentry); dput(dentry); return ret;
This will allow generating fsnotify delete events after the fsnotify_nameremove() hook is removed from d_delete(). Cc: Trond Myklebust <trond.myklebust@hammerspace.com> Cc: Anna Schumaker <anna.schumaker@netapp.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- net/sunrpc/rpc_pipe.c | 4 ++++ 1 file changed, 4 insertions(+)