[v3,03/10] rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks
diff mbox series

Message ID 20190526143411.11244-4-amir73il@gmail.com
State New
Headers show
Series
  • Sort out fsnotify_nameremove() mess
Related show

Commit Message

Amir Goldstein May 26, 2019, 2:34 p.m. UTC
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(+)

Comments

Jan Kara May 27, 2019, 10:53 a.m. UTC | #1
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
>
Amir Goldstein May 27, 2019, 1:26 p.m. UTC | #2
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.
Jan Kara May 27, 2019, 2 p.m. UTC | #3
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
Amir Goldstein May 30, 2019, 5:43 a.m. UTC | #4
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
>
Trond Myklebust May 30, 2019, 12:16 p.m. UTC | #5
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>

Patch
diff mbox series

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;