nfsd4_is_junction

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.

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

Quoted 2 lines · Christoph Hellwig

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?

Quoted 8 lines · Chuck Lever

Quoted 2 lines · Christoph Hellwig

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.

Quoted 1 line

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.

Quoted 11 lines · J. Bruce Fields

Quoted 9 lines · Chuck Lever

Quoted 2 lines · Christoph Hellwig

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?

Quoted 5 lines

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.

Quoted 17 lines · Chuck Lever

Quoted 14 lines · J. Bruce Fields

Quoted 9 lines · Chuck Lever

Quoted 2 lines · Christoph Hellwig

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?

Quoted 4 lines

Quoted 2 lines

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.

Quoted 18 lines · J. Bruce Fields

Quoted 17 lines · Chuck Lever

Quoted 14 lines · J. Bruce Fields

Quoted 9 lines · Chuck Lever

Quoted 2 lines · Christoph Hellwig

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?

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 35 lines · Chuck Lever

Quoted 18 lines · J. Bruce Fields

Quoted 17 lines · Chuck Lever

Quoted 14 lines · J. Bruce Fields

Quoted 9 lines · Chuck Lever

Quoted 2 lines · Christoph Hellwig

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?

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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...