Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> This patch really ought to be broken into more manageable atomic
> changes to make it easier to review, and to provide more fine-grained
> explanation and rationalization for each specific change via
> individual patch descriptions.
Hmmm.... I broke the patch up as Trond stipulated - at least, I thought I
had.
In many ways this request doesn't make sense. You can't do NFS caching
without all the appropriate bits, so logically they should be one patch.
Breaking it up won't help git-bisect since the option to enable all this is
the last (or nearly last) patch.
However, I can do it (when I get back from LCA next week).
> This should no longer be necessary. The latest mount.nfs subcommand
> from nfs-utils supports text-based mounts when running on kernels
> 2.6.23 and later.
Okay. I'll update my patches to reflect this. Note, however, I've got
someone reporting a bug that seems to show otherwise. I'll have to
investigate this more next week.
> I hope you intend to provide updates to nfs(5) that describe the new
> mount options you introduce in this and later patches. You don't
> mention it, but I assume that "nofsc" is the default behavior.
I should make SteveD do that, the options was his idea:-) But I'll deal with
it.
> Add comments like this in a separate clean up patch.
????
> A suggestion: fs/nfs/fsc-index.c might be a better name.
If you wish, though I'd prefer to use a name that isn't like to clash with a
name that's going to appear in fs/fscache/ (or include/linux/ - I'd really
like to rename fs/nfs/fscache.h as dealing with two fscache.h's is annoying.
> > +struct nfs_fh_auxdata {
> > + struct timespec i_mtime;
> > + struct timespec i_ctime;
> > + loff_t i_size;
> > +};
>
> It might be useful to explain here why you need to supplement the
> mtime, ctime, and size fields that already exist in an NFS inode.
Supplement? I don't understand.
> > + key->port = clp->cl_addr.sin_port;
>
> Not sure why you are using the server's port here. In almost every
> case the server side port number will be 2049, so it really doesn't
> add any uniquification.
The reason lies is "in almost every case". It's possible to configure it
such that a server is running two separate NFS servers on different ports.
> If you're going for the client side port number, that changes after
> every connection, so it would be useless to identify a cache after a
> reboot (or even after the connection idles out!).
I'm going for the server side port number. Using the client side port number
would be silly.
> I strongly recommend you use the existing IPv6 address conversion
> macros for this instead of open-coding yet another way of mapping an
> IPv4 address to an IPv6 address.
>
> However, since AF_INET6 support is being introduced in the NFS client
> in 2.6.24, I recommend you take a look at these source files after
> Trond has pushed his NFS_ALL for 2.6.24.
I'll look at them.
> See below: the NFS cache-related stats should be added to nfs_iostats.
I believe I asked Trond, but I'll check.
I've got to move, so I'll deal with the rest of your email later.
David
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
