vfs-scale, d_revalidate from nfsd

vfs-scale, d_revalidate from nfsd

[J. R. Okajima]

NFSD calls filesystem's ->d_revalidate() with the parameter nd == NULL.
So every
	if (nd->flags & LOOKUP_RCU)
		return -ECHILD;
code which was added to ->d_revalidate() of FS which supports NFS
exporting will crash.

If we rewrite it as
	if (nd && (nd->flags & LOOKUP_RCU))
		return -ECHILD;
the problem may not occur.
But I am not sure whether lookup_one_len() call in NFSD support rcu-walk.


J. R. Okajima

Re: vfs-scale, d_revalidate from nfsd

[Nick Piggin]

On Fri, Jan 14, 2011 at 1:03 AM, J. R. Okajima <hooanon05@yahoo.co.jp> wrote:
>
> NFSD calls filesystem's ->d_revalidate() with the parameter nd == NULL.
> So every
>        if (nd->flags & LOOKUP_RCU)
>                return -ECHILD;
> code which was added to ->d_revalidate() of FS which supports NFS
> exporting will crash.
>
> If we rewrite it as
>        if (nd && (nd->flags & LOOKUP_RCU))
>                return -ECHILD;
> the problem may not occur.
> But I am not sure whether lookup_one_len() call in NFSD support rcu-walk.

Ah, good catch.

I'm going to change the d_revalidate API so it takes and inode and rcu-walk
flag parameter to make it easier for filesystems to implement rcu-walk.

That will take care of this NULL nd case.

Thanks,
Nick

Re: vfs-scale, d_revalidate from nfsd

[Al Viro]

On Fri, Jan 14, 2011 at 01:57:55PM +1100, Nick Piggin wrote:
> On Fri, Jan 14, 2011 at 1:03 AM, J. R. Okajima <hooanon05@yahoo.co.jp> wrote:
> >
> > NFSD calls filesystem's ->d_revalidate() with the parameter nd == NULL.
> > So every
> > ? ? ? ?if (nd->flags & LOOKUP_RCU)
> > ? ? ? ? ? ? ? ?return -ECHILD;
> > code which was added to ->d_revalidate() of FS which supports NFS
> > exporting will crash.
> >
> > If we rewrite it as
> > ? ? ? ?if (nd && (nd->flags & LOOKUP_RCU))
> > ? ? ? ? ? ? ? ?return -ECHILD;
> > the problem may not occur.
> > But I am not sure whether lookup_one_len() call in NFSD support rcu-walk.
> 
> Ah, good catch.
> 
> I'm going to change the d_revalidate API so it takes and inode and rcu-walk
> flag parameter to make it easier for filesystems to implement rcu-walk.
> 
> That will take care of this NULL nd case.

Oh, for fuck sake...  Would you at least mind posting your API change
description to fsdevel before going for it?

->d_revalidate() is one sick puppy and it's intimately involved in atomic
open rewrite.  Please, *please* don't make that shit even messier...

Re: vfs-scale, d_revalidate from nfsd

[Nick Piggin]

On Fri, Jan 14, 2011 at 2:03 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Jan 14, 2011 at 01:57:55PM +1100, Nick Piggin wrote:
>> On Fri, Jan 14, 2011 at 1:03 AM, J. R. Okajima <hooanon05@yahoo.co.jp> wrote:
>> >
>> > NFSD calls filesystem's ->d_revalidate() with the parameter nd == NULL.
>> > So every
>> > ? ? ? ?if (nd->flags & LOOKUP_RCU)
>> > ? ? ? ? ? ? ? ?return -ECHILD;
>> > code which was added to ->d_revalidate() of FS which supports NFS
>> > exporting will crash.
>> >
>> > If we rewrite it as
>> > ? ? ? ?if (nd && (nd->flags & LOOKUP_RCU))
>> > ? ? ? ? ? ? ? ?return -ECHILD;
>> > the problem may not occur.
>> > But I am not sure whether lookup_one_len() call in NFSD support rcu-walk.
>>
>> Ah, good catch.
>>
>> I'm going to change the d_revalidate API so it takes and inode and rcu-walk
>> flag parameter to make it easier for filesystems to implement rcu-walk.
>>
>> That will take care of this NULL nd case.
>
> Oh, for fuck sake...  Would you at least mind posting your API change
> description to fsdevel before going for it?

Of course. I was discussing it with Miklos yesterday too, but haven't
finished getting a proposal together.

The main idea here would be to just pass in a flags parameter rather
thank poking in nd to get the rcu-walk status. That would solve this
problem and also avoid nd for most filesystems that don't care about
it.


> ->d_revalidate() is one sick puppy and it's intimately involved in atomic
> open rewrite.  Please, *please* don't make that shit even messier...

Right, I did agree with Miklos that using nd for passing rcu-walk
status was going in the wrong direction with that API. Do you
agree?

Re: vfs-scale, d_revalidate from nfsd

[Al Viro]

On Fri, Jan 14, 2011 at 02:12:35PM +1100, Nick Piggin wrote:
> The main idea here would be to just pass in a flags parameter rather
> thank poking in nd to get the rcu-walk status. That would solve this
> problem and also avoid nd for most filesystems that don't care about
> it.

Start with nd->flags getting passed explicitly, and be ready to see
	* call on the final stage of open split away and folded with
->lookup() and ->open()/->creat()
	* the rest of callers to lose nd completely.
That's what's going to happen in the next cycle.

BTW, why on the earth do you have that:
static int xattr_hide_revalidate(struct dentry *dentry, struct nameidata *nd)
{
        if (nd->flags & LOOKUP_RCU)
                return -ECHILD;
        return -EPERM;
}
when the sole intent of that sucker is to have dentry of /.xattr (pinned
in dcache and hashed all along) rejected on lookups from root?  IOW, WTF
bother with -ECHILD here at all?

Re: vfs-scale, d_revalidate from nfsd

[Al Viro]

> BTW, why on the earth do you have that:
> static int xattr_hide_revalidate(struct dentry *dentry, struct nameidata *nd)
> {
>         if (nd->flags & LOOKUP_RCU)
>                 return -ECHILD;
>         return -EPERM;
> }
> when the sole intent of that sucker is to have dentry of /.xattr (pinned
> in dcache and hashed all along) rejected on lookups from root?  IOW, WTF
> bother with -ECHILD here at all?

PS: that's fs/reiserfs/xattr.c

Re: vfs-scale, d_revalidate from nfsd

[Nick Piggin]

On Fri, Jan 14, 2011 at 2:20 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Fri, Jan 14, 2011 at 02:12:35PM +1100, Nick Piggin wrote:
>> The main idea here would be to just pass in a flags parameter rather
>> thank poking in nd to get the rcu-walk status. That would solve this
>> problem and also avoid nd for most filesystems that don't care about
>> it.
>
> Start with nd->flags getting passed explicitly, and be ready to see
>        * call on the final stage of open split away and folded with
> ->lookup() and ->open()/->creat()
>        * the rest of callers to lose nd completely.
> That's what's going to happen in the next cycle.

OK that sounds nice. I'll see what it looks like and definitely run any
proposed API change past you and fsdevel.


> BTW, why on the earth do you have that:
> static int xattr_hide_revalidate(struct dentry *dentry, struct nameidata *nd)
> {
>        if (nd->flags & LOOKUP_RCU)
>                return -ECHILD;
>        return -EPERM;
> }
> when the sole intent of that sucker is to have dentry of /.xattr (pinned
> in dcache and hashed all along) rejected on lookups from root?  IOW, WTF
> bother with -ECHILD here at all?

That's true. I guess I always have a weakness for doing "just one
little easy optimisation/simplification" folded into patch that is supposed
to be more mechanical changes :) I did have exactly that in the patch
initially, but I decided it's better just to do everything with -ECHILD first.

That also gives the -ECHILD paths a bit more workout before
fs conversions are done, too.

Re: vfs-scale, d_revalidate from nfsd

[Al Viro]

On Fri, Jan 14, 2011 at 02:29:11PM +1100, Nick Piggin wrote:
> > ? ? ? ?if (nd->flags & LOOKUP_RCU)
> > ? ? ? ? ? ? ? ?return -ECHILD;
> > ? ? ? ?return -EPERM;
> > }
> > when the sole intent of that sucker is to have dentry of /.xattr (pinned
> > in dcache and hashed all along) rejected on lookups from root? ?IOW, WTF
> > bother with -ECHILD here at all?
> 
> That's true. I guess I always have a weakness for doing "just one
> little easy optimisation/simplification" folded into patch that is supposed
> to be more mechanical changes :) I did have exactly that in the patch
> initially, but I decided it's better just to do everything with -ECHILD first.
> 
> That also gives the -ECHILD paths a bit more workout before
> fs conversions are done, too.

Not unless your test loads include trying to access pathnames like
/.xattr/whatever on reiserfs and watching those attempts fail...

Re: vfs-scale, d_revalidate from nfsd

[J. R. Okajima]

Nick Piggin:
> Of course. I was discussing it with Miklos yesterday too, but haven't
> finished getting a proposal together.
>
> The main idea here would be to just pass in a flags parameter rather
> thank poking in nd to get the rcu-walk status. That would solve this
> problem and also avoid nd for most filesystems that don't care about
> it.

Let me make sure.
- add a flag parameter to ->d_revalidate. not remove the panameter nd.
- FS ->d_revalidate() will (probably) return -ECHILD when LOOKUP_RCU is
  set.
Right?


Then how about the callers?
Current sequence is
- NFSD calls lookup_one_len
- __lookup_hash
- do_revalidate
- d_revalidate
{
	status = dentry->d_op->d_revalidate(dentry, nd);
	if (status == -ECHILD) {
		;;;
		status = dentry->d_op->d_revalidate(dentry, nd);
	}
}

There will be no change in NFSD but VFS d_revalidate(), such like this?
VFS d_revalidate()
{
	if (nd) {
		dentry->d_op->d_revalidate(dentry, nd, nd->flags);
		if (-ECHILD) {
			dentry->d_op->d_revalidate(dentry, nd, nd->flags);
		}
	} else
		return dentry->d_op->d_revalidate(dentry, NULL, 0);
}


J. R. Okajima

Re: vfs-scale, d_revalidate from nfsd

[Nick Piggin]

On Sat, Jan 15, 2011 at 2:47 PM, J. R. Okajima <hooanon05@yahoo.co.jp> wrote:
>
> Nick Piggin:
>> Of course. I was discussing it with Miklos yesterday too, but haven't
>> finished getting a proposal together.
>>
>> The main idea here would be to just pass in a flags parameter rather
>> thank poking in nd to get the rcu-walk status. That would solve this
>> problem and also avoid nd for most filesystems that don't care about
>> it.
>
> Let me make sure.
> - add a flag parameter to ->d_revalidate. not remove the panameter nd.
> - FS ->d_revalidate() will (probably) return -ECHILD when LOOKUP_RCU is
>  set.
> Right?
>
>
> Then how about the callers?
> Current sequence is
> - NFSD calls lookup_one_len
> - __lookup_hash
> - do_revalidate
> - d_revalidate
> {
>        status = dentry->d_op->d_revalidate(dentry, nd);
>        if (status == -ECHILD) {
>                ;;;
>                status = dentry->d_op->d_revalidate(dentry, nd);
>        }
> }
>
> There will be no change in NFSD but VFS d_revalidate(), such like this?
> VFS d_revalidate()
> {
>        if (nd) {
>                dentry->d_op->d_revalidate(dentry, nd, nd->flags);
>                if (-ECHILD) {
>                        dentry->d_op->d_revalidate(dentry, nd, nd->flags);
>                }
>        } else
>                return dentry->d_op->d_revalidate(dentry, NULL, 0);
> }

Yes that's the idea. I also want to pass a struct inode * parameter, to
facilitate rcu-walk implementations that want to check the inode
(such as fuse).

Re: vfs-scale, d_revalidate from nfsd

[J. R. Okajima]

Nick Piggin:
> On Fri, Jan 14, 2011 at 1:03 AM, J. R. Okajima <hooanon05@yahoo.co.jp> wrot=
> e:
> > NFSD calls filesystem's ->d_revalidate() with the parameter nd =3D=3D NUL=
> L.
	:::
> Ah, good catch.
>
> I'm going to change the d_revalidate API so it takes and inode and rcu-walk
> flag parameter to make it easier for filesystems to implement rcu-walk.
>
> That will take care of this NULL nd case.

While you might already know there are similar parts in fs/namei.c, I'd
like to write here just to make sure.

- nfsd calls lookup_one_len()
- lookup_one_len()
  + __lookup_hash() with nd=NULL
    + do_revalidate(dentry, nd)
      or/and
    + d_alloc_and_lookup(base, name, nd)

- do_revalidate(dentry, nd)
  + status = d_revalidate(dentry, nd)
    + d_op->d_revalidate(dentry, nd)	<-- previous mail pointed out
  + if (status < 0) {
	if (!(nd->flags & LOOKUP_RCU))	<-- this also needs fixing
		dput(dentry);
    } else {
	if (nameidata_dentry_drop_rcu_maybe(nd, dentry)) <-- and here
		return ERR_PTR(-ECHILD);
    }

And if ->d_revalidate() can return ECHILD when 'nd' is NULL,
then nameidata_dentry_drop_rcu() call in d_revalidate() needs fixing
too.

I don't think ->lookup(inode, dentry, nd) call in d_alloc_and_lookup()
is a problem since every FS which supports NFS-exporting never refer
'nd' already.


J. R. Okajima