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