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:0.035 / U:3.808 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site