lkml.org 
[lkml]   [2008]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 24/27] NFS: Use local caching [try #2]
Date

Chuck Lever <chuck.lever@oracle.com> 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


\
 
 \ /
  Last update: 2008-01-30 04:45    [W:1.463 / U:0.292 seconds]
©2003-2011 Jasper Spaans. Advertise on this site