From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [PATCH] fuse: make fuse_permission() RCU aware Date: Thu, 13 Jan 2011 12:49:53 +1100 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-fsdevel@vger.kernel.org, akpm@linux-foundation.org, linux-kernel@vger.kernel.org To: Miklos Szeredi Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Thu, Jan 13, 2011 at 1:26 AM, Miklos Szeredi wro= te: > On Wed, 12 Jan 2011, Nick Piggin wrote: >> On Tue, Jan 11, 2011 at 11:14 PM, Miklos Szeredi = wrote: >> > From: Miklos Szeredi >> > >> > Only bail out of fuse_permission() on IPERM_FLAG_RCU when it is >> > actually necessary. >> >> Great, thanks for taking a look... How about d_revalidate? > > Yeah, here's the patch > > Do you want to collect these patches from fs maintainers, or should I > submit to Linus directly? I would like to have a look over them and ack them before they go to Li= nus, but I think the most logical and merge friendly approach is just going = via filesystems trees. > From: Miklos Szeredi > Subject: fuse: make fuse_dentry_revalidate() RCU aware > > Only bail out of fuse_dentry_revalidate() on LOOKUP_RCU when blocking > is actually necessary. > > Signed-off-by: Miklos Szeredi > --- > =A0fs/fuse/dir.c | =A0 =A06 +++--- > =A01 file changed, 3 insertions(+), 3 deletions(-) > > Index: linux-2.6/fs/fuse/dir.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- linux-2.6.orig/fs/fuse/dir.c =A0 =A0 =A0 =A02011-01-12 13:06:04.0= 00000000 +0100 > +++ linux-2.6/fs/fuse/dir.c =A0 =A0 2011-01-12 13:07:30.000000000 +01= 00 > @@ -158,9 +158,6 @@ static int fuse_dentry_revalidate(struct > =A0{ > =A0 =A0 =A0 =A0struct inode *inode; > > - =A0 =A0 =A0 if (nd->flags & LOOKUP_RCU) > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ECHILD; > - > =A0 =A0 =A0 =A0inode =3D entry->d_inode; > =A0 =A0 =A0 =A0if (inode && is_bad_inode(inode)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0; Now it can be the case that entry->d_inode is not stable -- it can go away or even flip between inodes in the case of concurrent unlink/creat activity. And you may be using a different inode than the namei path walk is using! This isn't as scary as it sounds actually, because any such changes do get detected and the path walk restarted in that case. You might be OK becuse you do test for NULL (although it really wants an ACCESS_ONCE() so it doesn't load a NULL or different inodes, but that's quite theoretical). But this is a little unfriendly for filesystems, and I do want to impre= ss the rule to not touch ->d_parent or ->d_inode in rcu walk mode (just to avoid any surprises). So what I have done in such cases is to update the API to provide what the callers want. In this case, we could consider adding an inode parameter to .d_revalidate, which callers can be sure matches the inode used by vfs, and will not change. Aside from that detail, the changes seem good. Thanks for looking at it. > @@ -177,6 +174,9 @@ static int fuse_dentry_revalidate(struct > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (!inode) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return 0; > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (nd->flags & LOOKUP_RCU) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ECHILD; > + > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fc =3D get_fuse_conn(inode); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0req =3D fuse_get_req(fc); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (IS_ERR(req)) >