nfsd4_is_junction

nfsd4_is_junction

[Christoph Hellwig]

This function should probably grow a check for S_NOSEC instead of
hitting the filesystems with an xattr read for every lookup.

Re: nfsd4_is_junction

[J. Bruce Fields]

On Sat, Oct 29, 2011 at 06:16:49AM -0400, Christoph Hellwig wrote:
> This function should probably grow a check for S_NOSEC instead of
> hitting the filesystems with an xattr read for every lookup.

Note it also takes the convention that junctions should have the sticky
bit set without being executable, and it does those checks before
checking for the xattr, so it shouldn't be doing an xattr read for every
lookup.

Grepping for S_NOSEC.... "no suid or xattr security attributes".  But
"junction.type" isn't a "security attribute", is it?  I may not
understand what that means.

--b.

Re: nfsd4_is_junction

[Chuck Lever]

On Oct 29, 2011, at 6:16 AM, Christoph Hellwig wrote:

> This function should probably grow a check for S_NOSEC instead of
> hitting the filesystems with an xattr read for every lookup.

It doesn't perform the xattr read on _every_ lookup, but only when the object has a special set of permission bits.

But actually, it probably doesn't need that 'type" attribute check at all.  It should delegate that check to mountd.  I assume many of those upcalls would hit in the export cache anyway?

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: nfsd4_is_junction

[J. Bruce Fields]

On Sat, Oct 29, 2011 at 01:47:55PM -0400, Chuck Lever wrote:
> 
> On Oct 29, 2011, at 6:16 AM, Christoph Hellwig wrote:
> 
> > This function should probably grow a check for S_NOSEC instead of
> > hitting the filesystems with an xattr read for every lookup.
> 
> It doesn't perform the xattr read on _every_ lookup, but only when the object has a special set of permission bits.
> 
> But actually, it probably doesn't need that 'type" attribute check at all.  It should delegate that check to mountd.  I assume many of those upcalls would hit in the export cache anyway?

You're suggesting doing an upcall on every path that passes the mode-bit
checks?

You'd end up populating the export cache with every path that passes
the mode-bit checks.  I don't think that's a good idea.

--b.

Re: nfsd4_is_junction

[Chuck Lever]

On Oct 29, 2011, at 1:55 PM, J. Bruce Fields wrote:

> On Sat, Oct 29, 2011 at 01:47:55PM -0400, Chuck Lever wrote:
>> 
>> On Oct 29, 2011, at 6:16 AM, Christoph Hellwig wrote:
>> 
>>> This function should probably grow a check for S_NOSEC instead of
>>> hitting the filesystems with an xattr read for every lookup.
>> 
>> It doesn't perform the xattr read on _every_ lookup, but only when the object has a special set of permission bits.
>> 
>> But actually, it probably doesn't need that 'type" attribute check at all.  It should delegate that check to mountd.  I assume many of those upcalls would hit in the export cache anyway?
> 
> You're suggesting doing an upcall on every path that passes the mode-bit
> checks?
> 
> You'd end up populating the export cache with every path that passes
> the mode-bit checks.  I don't think that's a good idea.

You just finished telling us that the mode bit checks would prevent doing an attribute read too often.  Again, how often do we think there will be directories with no execute bits set and the sticky bit set?  I think that's going to be exceptionally rare.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: nfsd4_is_junction

[J. Bruce Fields]

On Sat, Oct 29, 2011 at 01:58:02PM -0400, Chuck Lever wrote:
> 
> On Oct 29, 2011, at 1:55 PM, J. Bruce Fields wrote:
> 
> > On Sat, Oct 29, 2011 at 01:47:55PM -0400, Chuck Lever wrote:
> >> 
> >> On Oct 29, 2011, at 6:16 AM, Christoph Hellwig wrote:
> >> 
> >>> This function should probably grow a check for S_NOSEC instead of
> >>> hitting the filesystems with an xattr read for every lookup.
> >> 
> >> It doesn't perform the xattr read on _every_ lookup, but only when the object has a special set of permission bits.
> >> 
> >> But actually, it probably doesn't need that 'type" attribute check at all.  It should delegate that check to mountd.  I assume many of those upcalls would hit in the export cache anyway?
> > 
> > You're suggesting doing an upcall on every path that passes the mode-bit
> > checks?
> > 
> > You'd end up populating the export cache with every path that passes
> > the mode-bit checks.  I don't think that's a good idea.
> 
> You just finished telling us that the mode bit checks would prevent doing an attribute read too often.  Again, how often do we think there will be directories with no execute bits set and the sticky bit set?  I think that's going to be exceptionally rare.

Yeah, probably so, but if there's a pathological case where we're wrong
then I think excessive xattr reads will be less annoying than excessive
upcalls and export cache entries.

And in the cached case: I doubt a lookup in the export cache is really
faster than a read of a cached xattr.  But I don't know.

--b.

Re: nfsd4_is_junction

[Chuck Lever]

On Oct 29, 2011, at 2:09 PM, J. Bruce Fields wrote:

> On Sat, Oct 29, 2011 at 01:58:02PM -0400, Chuck Lever wrote:
>> 
>> On Oct 29, 2011, at 1:55 PM, J. Bruce Fields wrote:
>> 
>>> On Sat, Oct 29, 2011 at 01:47:55PM -0400, Chuck Lever wrote:
>>>> 
>>>> On Oct 29, 2011, at 6:16 AM, Christoph Hellwig wrote:
>>>> 
>>>>> This function should probably grow a check for S_NOSEC instead of
>>>>> hitting the filesystems with an xattr read for every lookup.
>>>> 
>>>> It doesn't perform the xattr read on _every_ lookup, but only when the object has a special set of permission bits.
>>>> 
>>>> But actually, it probably doesn't need that 'type" attribute check at all.  It should delegate that check to mountd.  I assume many of those upcalls would hit in the export cache anyway?
>>> 
>>> You're suggesting doing an upcall on every path that passes the mode-bit
>>> checks?
>>> 
>>> You'd end up populating the export cache with every path that passes
>>> the mode-bit checks.  I don't think that's a good idea.
>> 
>> You just finished telling us that the mode bit checks would prevent doing an attribute read too often.  Again, how often do we think there will be directories with no execute bits set and the sticky bit set?  I think that's going to be exceptionally rare.
> 
> Yeah, probably so, but if there's a pathological case where we're wrong
> then I think excessive xattr reads will be less annoying than excessive
> upcalls and export cache entries.

If we find a pathological case, we should fix it.

Basically I'd like the kernel check to be as simple as possible not only because it should be fast, but also so that user space can implement junctions flexibly, especially in the early days when we are still unsure of how these need to work.  I think we are better off being flexible to start, and restricting only if we have to.

Or to put it another way, user space, and not the kernel, should arbitrate policy.  The semantics of junctions is a policy, at least for now.

And lastly, right now, I expect that nearly 100% of the time the mode bit test and the xattr test will either both succeed or both fail.  That suggests one of them is redundant.

> And in the cached case: I doubt a lookup in the export cache is really
> faster than a read of a cached xattr.  But I don't know.

However either case should hit in memory if the path being looked up is hot.  I suspect the mode bit test plus the kernel's export cache should shield mountd from an excessive number of upcalls.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: nfsd4_is_junction

[Trond Myklebust]

On Sat, 2011-10-29 at 14:38 -0400, Chuck Lever wrote: 
> On Oct 29, 2011, at 2:09 PM, J. Bruce Fields wrote:
> 
> > On Sat, Oct 29, 2011 at 01:58:02PM -0400, Chuck Lever wrote:
> >> 
> >> On Oct 29, 2011, at 1:55 PM, J. Bruce Fields wrote:
> >> 
> >>> On Sat, Oct 29, 2011 at 01:47:55PM -0400, Chuck Lever wrote:
> >>>> 
> >>>> On Oct 29, 2011, at 6:16 AM, Christoph Hellwig wrote:
> >>>> 
> >>>>> This function should probably grow a check for S_NOSEC instead of
> >>>>> hitting the filesystems with an xattr read for every lookup.
> >>>> 
> >>>> It doesn't perform the xattr read on _every_ lookup, but only when the object has a special set of permission bits.
> >>>> 
> >>>> But actually, it probably doesn't need that 'type" attribute check at all.  It should delegate that check to mountd.  I assume many of those upcalls would hit in the export cache anyway?
> >>> 
> >>> You're suggesting doing an upcall on every path that passes the mode-bit
> >>> checks?
> >>> 
> >>> You'd end up populating the export cache with every path that passes
> >>> the mode-bit checks.  I don't think that's a good idea.
> >> 
> >> You just finished telling us that the mode bit checks would prevent doing an attribute read too often.  Again, how often do we think there will be directories with no execute bits set and the sticky bit set?  I think that's going to be exceptionally rare.
> > 
> > Yeah, probably so, but if there's a pathological case where we're wrong
> > then I think excessive xattr reads will be less annoying than excessive
> > upcalls and export cache entries.
> 
> If we find a pathological case, we should fix it.
> 
> Basically I'd like the kernel check to be as simple as possible not only because it should be fast, but also so that user space can implement junctions flexibly, especially in the early days when we are still unsure of how these need to work.  I think we are better off being flexible to start, and restricting only if we have to.
> 
> Or to put it another way, user space, and not the kernel, should arbitrate policy.  The semantics of junctions is a policy, at least for now.
> 
> And lastly, right now, I expect that nearly 100% of the time the mode bit test and the xattr test will either both succeed or both fail.  That suggests one of them is redundant.

They are almost redundant: If you do the xattr test, then you don't need
to do the mode bit test. However if you do the mode bit test, then you
still need to do the xattr test.

Basically, the mode bit test is supposed to be the speedup that avoids
the need for another S_NOSEC-style bit...

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: nfsd4_is_junction

[Chuck Lever]

On Oct 29, 2011, at 7:36 PM, Trond Myklebust wrote:

> On Sat, 2011-10-29 at 14:38 -0400, Chuck Lever wrote: 
>> On Oct 29, 2011, at 2:09 PM, J. Bruce Fields wrote:
>> 
>>> On Sat, Oct 29, 2011 at 01:58:02PM -0400, Chuck Lever wrote:
>>>> 
>>>> On Oct 29, 2011, at 1:55 PM, J. Bruce Fields wrote:
>>>> 
>>>>> On Sat, Oct 29, 2011 at 01:47:55PM -0400, Chuck Lever wrote:
>>>>>> 
>>>>>> On Oct 29, 2011, at 6:16 AM, Christoph Hellwig wrote:
>>>>>> 
>>>>>>> This function should probably grow a check for S_NOSEC instead of
>>>>>>> hitting the filesystems with an xattr read for every lookup.
>>>>>> 
>>>>>> It doesn't perform the xattr read on _every_ lookup, but only when the object has a special set of permission bits.
>>>>>> 
>>>>>> But actually, it probably doesn't need that 'type" attribute check at all.  It should delegate that check to mountd.  I assume many of those upcalls would hit in the export cache anyway?
>>>>> 
>>>>> You're suggesting doing an upcall on every path that passes the mode-bit
>>>>> checks?
>>>>> 
>>>>> You'd end up populating the export cache with every path that passes
>>>>> the mode-bit checks.  I don't think that's a good idea.
>>>> 
>>>> You just finished telling us that the mode bit checks would prevent doing an attribute read too often.  Again, how often do we think there will be directories with no execute bits set and the sticky bit set?  I think that's going to be exceptionally rare.
>>> 
>>> Yeah, probably so, but if there's a pathological case where we're wrong
>>> then I think excessive xattr reads will be less annoying than excessive
>>> upcalls and export cache entries.
>> 
>> If we find a pathological case, we should fix it.
>> 
>> Basically I'd like the kernel check to be as simple as possible not only because it should be fast, but also so that user space can implement junctions flexibly, especially in the early days when we are still unsure of how these need to work.  I think we are better off being flexible to start, and restricting only if we have to.
>> 
>> Or to put it another way, user space, and not the kernel, should arbitrate policy.  The semantics of junctions is a policy, at least for now.
>> 
>> And lastly, right now, I expect that nearly 100% of the time the mode bit test and the xattr test will either both succeed or both fail.  That suggests one of them is redundant.
> 
> They are almost redundant: If you do the xattr test, then you don't need
> to do the mode bit test. However if you do the mode bit test, then you
> still need to do the xattr test.
> 
> Basically, the mode bit test is supposed to be the speedup that avoids
> the need for another S_NOSEC-style bit...

OK, just to close this out:  I'm hearing that folks are not interested in getting rid of the "trusted.junction.type" extended attribute.  I don't agree, but I will accede and continue to support its use for junctions that NFSD has to pay attention to.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: nfsd4_is_junction

[Christoph Hellwig]

On Sat, Oct 29, 2011 at 01:45:33PM -0400, J. Bruce Fields wrote:
> Note it also takes the convention that junctions should have the sticky
> bit set without being executable, and it does those checks before
> checking for the xattr, so it shouldn't be doing an xattr read for every
> lookup.
> 
> Grepping for S_NOSEC.... "no suid or xattr security attributes".  But
> "junction.type" isn't a "security attribute", is it?  I may not
> understand what that means.

It's basically to avoid doing getxattr calls if not needed.  So far this
was just for security xattrs, but this one falls into a pretty similar
category.