bug in nfs in 2.6.18-rc5?

bug in nfs in 2.6.18-rc5?

[Shaya Potter]

so I'm trying to use unionfs, cachefs and nfs, as cachefs is 2.6.18-rc5 
right now, thats what I'm testing, but I hit an oops.

basically unionfs's lookup does a "lookup_one_len()" on the underlying fs.

lookup_one_len() calls __lookup_hash()

__lookup_hash() is called as "__lookup_hash(&this, base, NULL)"

now that NULL is important.  that's the nameidata entry of __lookup_hash()

__lookup_hash() ends up calling the underlying fs's lookup op, i.e. 
nfs_lookup()

nfs_lookup() calls nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);

see the bug? :)

This doesn't seem like a unionfs bug, as one should be able to call 
lookup_one_len() on an NFS fs.

Re: bug in nfs in 2.6.18-rc5?

[Shaya Potter]

Shaya Potter wrote:
> so I'm trying to use unionfs, cachefs and nfs, as cachefs is 2.6.18-rc5 
> right now, thats what I'm testing, but I hit an oops.
> 
> basically unionfs's lookup does a "lookup_one_len()" on the underlying fs.
> 
> lookup_one_len() calls __lookup_hash()
> 
> __lookup_hash() is called as "__lookup_hash(&this, base, NULL)"
> 
> now that NULL is important.  that's the nameidata entry of __lookup_hash()
> 
> __lookup_hash() ends up calling the underlying fs's lookup op, i.e. 
> nfs_lookup()
> 
> nfs_lookup() calls nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);
> 
> see the bug? :)
> 
> This doesn't seem like a unionfs bug, as one should be able to call 
> lookup_one_len() on an NFS fs.

ok my "fix" is basically that nfs_reval_fsid() uses the nd->mnt to get 
to mnt->mnt_root.

I basically switched it to take a dentry and I call it as

nfs_reval_fsid(dentry->d_sb->s_root, dir.....)

have no clue if this is correct, but it doesn't oops anymore and seems 
to "work".

Re: bug in nfs in 2.6.18-rc5?

[Trond Myklebust]

On Thu, 2006-08-31 at 10:54 -0400, Shaya Potter wrote:

> __lookup_hash() ends up calling the underlying fs's lookup op, i.e. 
> nfs_lookup()
> 
> nfs_lookup() calls nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);
> 
> see the bug? :)
> 
> This doesn't seem like a unionfs bug, as one should be able to call 
> lookup_one_len() on an NFS fs.

Did someone start handing out these promises when I wasn't looking?

AFAICS, lookup_one_len() should only be used by the filesystem itself,
or by services like nfsd that have intimate knowledge of the
filesystem's inner workings.

The reason why NFS would like to insist on that nameidata is that we
need to be able to create mountpoints on the fly when we cross from one
filesystem on the server to another. Otherwise, we cannot offer the type
of guarantees that POSIX applications expect, such as the ability to
provide unique permanent inode numbers.
If we're to provide the ability for unionfs to use lookup_one_len() on
NFS, then we will have to error out whenever we hit a case where we
should be creating a new mountpoint. Is that acceptable?

Cheers,
  Trond

Re: bug in nfs in 2.6.18-rc5?

[Shaya Potter]

Trond Myklebust wrote:
> On Thu, 2006-08-31 at 10:54 -0400, Shaya Potter wrote:
> 
>> __lookup_hash() ends up calling the underlying fs's lookup op, i.e. 
>> nfs_lookup()
>>
>> nfs_lookup() calls nfs_reval_fsid(nd->mnt, dir, &fhandle, &fattr);
>>
>> see the bug? :)
>>
>> This doesn't seem like a unionfs bug, as one should be able to call 
>> lookup_one_len() on an NFS fs.
> 
> Did someone start handing out these promises when I wasn't looking?
> 
> AFAICS, lookup_one_len() should only be used by the filesystem itself,
> or by services like nfsd that have intimate knowledge of the
> filesystem's inner workings.
> 
> The reason why NFS would like to insist on that nameidata is that we
> need to be able to create mountpoints on the fly when we cross from one
> filesystem on the server to another. Otherwise, we cannot offer the type
> of guarantees that POSIX applications expect, such as the ability to
> provide unique permanent inode numbers.
> If we're to provide the ability for unionfs to use lookup_one_len() on
> NFS, then we will have to error out whenever we hit a case where we
> should be creating a new mountpoint. Is that acceptable?

why does the client care about server mounted file systems?  The 
server's nfsd has to tell them apart, otherwise shouldn't give them to 
the client.  Otherwise it seems like the nfsd and the nfs client have to 
have innate knowledge of each other.

Re: bug in nfs in 2.6.18-rc5?

[Trond Myklebust]

On Thu, 2006-08-31 at 12:24 -0400, Shaya Potter wrote:
> why does the client care about server mounted file systems?

It wants to allow POSIX applications to work correctly even in the case
where the nfsd administrator is using 'nohide'. It wants those same
applications to work correctly in the case where the nfsd administrator
is exporting more than one filesystem over NFSv4.

>   The 
> server's nfsd has to tell them apart, otherwise shouldn't give them to 
> the client.  Otherwise it seems like the nfsd and the nfs client have to 
> have innate knowledge of each other.

Of course the server knows that it is crossing a mountpoint. The client
figures it out by looking at the 'fsid' attribute (which uniquely labels
the filesystem on that server) in order to figure out which filesystem
that the file/directory it just looked up belongs to. Whenever the
fileid changes between parent directory and child, that means that a
mountpoint was crossed on the server.

  Trond

Re: bug in nfs in 2.6.18-rc5?

[Christoph Hellwig]

On Thu, Aug 31, 2006 at 12:03:50PM -0400, Trond Myklebust wrote:
> If we're to provide the ability for unionfs to use lookup_one_len() on
> NFS, then we will have to error out whenever we hit a case where we
> should be creating a new mountpoint. Is that acceptable?

Not at all.  unionfs will have to pass down proper lookup intents.
The ecryptfs developers have started looking into making that work
with stackable filesystems, and the unionfs developers should follow
that effort.

Re: bug in nfs in 2.6.18-rc5?

[Trond Myklebust]

On Thu, 2006-08-31 at 17:26 +0100, Christoph Hellwig wrote:
> On Thu, Aug 31, 2006 at 12:03:50PM -0400, Trond Myklebust wrote:
> > If we're to provide the ability for unionfs to use lookup_one_len() on
> > NFS, then we will have to error out whenever we hit a case where we
> > should be creating a new mountpoint. Is that acceptable?
> 
> Not at all.  unionfs will have to pass down proper lookup intents.
> The ecryptfs developers have started looking into making that work
> with stackable filesystems, and the unionfs developers should follow
> that effort.

Good. I fully agree that this is preferable.

Cheers,
  Trond

Re: bug in nfs in 2.6.18-rc5?

[Christoph Hellwig]

On Thu, Aug 31, 2006 at 10:54:07AM -0400, Shaya Potter wrote:
> so I'm trying to use unionfs, cachefs and nfs, as cachefs is 2.6.18-rc5 
> right now, thats what I'm testing, but I hit an oops.
> 
> basically unionfs's lookup does a "lookup_one_len()" on the underlying fs.

And there you have the bug already. unionfs must not do this, and given
the past discussion on this the unionfs developers should know that
very well :)