lkml.org 
[lkml]   [2008]   [Jan]   [30]   [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
Hi David-

On Jan 29, 2008, at 10:25 PM, David Howells wrote:
> 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.

In addition to adding a new feature, you are changing existing code.
If any one of the changes you made breaks existing behavior, having
them all in small atomic patches makes it practical to bisect and
find the problem.

In addition it makes it worlds easier to review by people who are not
so familiar with your fscache implementation. And smaller patches
means the ratio of patch descriptions to code changes can be much
higher.

It does make sense to introduce the files under fs/fsc in a single
patch. But when you are changing code that is already being used,
more care needs to be taken.

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

The very latest version (post 1.1.1) is required today for text-based
NFS mounts. (That is, the bleeding edge version you get by cloning
the nfs-utils git repo).

And it only works on kernels later than 2.6.22 -- if that particular
user is testing fscache on 2.6.22 or older, then only the legacy
binary NFS mount system call API is supported.

>> Add comments like this in a separate clean up patch.
>
> ????

+/*
+ * Notification that a PTE pointing to an NFS page is about to be made
+ * writable, implying that someone is about to modify the page
through a
+ * shared-writable mapping
+ */

What does that have to do with local disk caching?

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

Why is it necessary to add additional mtime, ctime and size fields
for NFS inodes? Similar metadata is already stored in nfsi.

All I'm asking for is some documentation of what these fields do that
the existing time stamps and size fields in nfsi don't. Explain why
the NFS fsc implementation needs this data structure.

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

We should explore whether it is typical or even possible that such a
configuration exports the same file handles on different ports, and
whether that really matters to the client.

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

I always do this: I meant 2.6.25, not 2.6.24.

By the time you return, basic IPv6 support for NFSv4 should be in
2.6.25-rc1's NFS client (not server). Not that it is bug-free, but
an implementation is now there.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com


\
 
 \ /
  Last update: 2008-01-30 23:41    [W:0.082 / U:1.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site