[PATCH 0/3] Intro: Better handling coarse-grained timestamps

[PATCH 0/3] Intro: Better handling coarse-grained timestamps

[Bob Bell]

While developing a new product that uses NFS here at EMC, we've run into
issues with cache coherency.  Our server uses ext3, and therefore can
only provide timestamps with a granularity of 1 second.  We have seen
cases where a directory is modified on the server, then a non-existent
filename within that directory is accessed from the client (returning
an error), and then the directory entry (either subdirectory or file)
gets added on the server, all within the same second.  This leads to
a cached negative dentry on the client, which results in erroneously
returning "file not found" to all future lstat()s and open()s, as the
timestamp on the parent directory has not been modified.

Ideally, all NFS servers would provide finer-grained timestamps, so 
that all changes to a directory also modify the timestamp.  However, it
is a reality that not every one will.  Therefore, we are attempting to
tackle this problem with the following three patches.

First, if the nlinks count changes, invalidate the cached data for the 
directory.  This is of limited use, as it only catches the cases where 
the number of subdirectories has changed.  However, because there is no 
really downside to checking the nlinks count, this check can be 
universally enabled.

Secondly, a modification to the kernel has been made to disable the 
caching of negative dentries when the NFS_MOUNT_NONEGDE flag is set.  
Note that this does nothing to address outdated information being 
returned by readdir().  However, it does allow a file to be open()ed or 
lstat()ed if the filename is known.  Because this has a potential 
performance impact, it is left as an option for the user to select.

Lastly, changes have been made to userspace to accept the "nonegde" 
mount option and set the NFS_MOUNT_NONEGDE flag.

Your review and comments are greatly appreciated.  I believe this is 
a fairly basic patch, and it has worked well in testing so far on SLES
10 (with slight modifications to match the software versions in SLES
10).  Most of this patch is fairly trivial; my only open question is
whether "nonegde" is a good name (I myself sometimes read it as
"non-edge", but have yet to come up with anything better).

-- 
Bob Bell

[PATCH 1/3] NFS: Check nlinks count

[Bob Bell]

From: Philippe Armangau <armangau_philippe@emc.com>

Check the nlink count when updating an NFS inode, and mark the 
attributes invalid if it has changed.  If the inode is a directory, also 
invalidate the cached data so that the directory's contents will be 
refreshed.

Signed-off-by: Philippe Armangau <armangau_philippe@emc.com>
Acked-by: Bob Bell <bell_robert@emc.com>
---
 fs/nfs/inode.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 5747d49..3e85a70 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1063,6 +1063,12 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 	memcpy(&inode->i_atime, &fattr->atime, sizeof(inode->i_atime));
 	nfsi->change_attr = fattr->change_attr;
 
+	if (inode->i_nlink != fattr->nlink) {
+		invalid |= NFS_INO_INVALID_ATTR;
+		if (S_ISDIR(inode->i_mode))
+			invalid |= NFS_INO_INVALID_DATA;
+	}
+
 	if ((inode->i_mode & S_IALLUGO) != (fattr->mode & S_IALLUGO) ||
 	    inode->i_uid != fattr->uid ||
 	    inode->i_gid != fattr->gid)

[PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Bob Bell]

From: Bob Bell <bell_robert@emc.com>

Add NFS_MOUNT_NONEGDE mount flag.  When this flag is set on a mounted
NFS filesystem, negative dentries for that filesystem will not be 
cached.  This can help address cache coherency when the NFS server does 
not provide sufficiently fine-grained timestamps to consistently 
distinguish when a directory has been modified, though performance when 
repeatedly accessing filenames for files that do not exist may be 
impacted.

Signed-off-by: Bob Bell <bell_robert@emc.com>
---
 fs/nfs/dir.c              |    5 +++++
 fs/nfs/nfsroot.c          |   10 +++++++++-
 fs/nfs/super.c            |    9 +++++++++
 include/linux/nfs_mount.h |   35 ++++++++++++++++++-----------------
 4 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 410449d..9419f55 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -851,6 +851,11 @@ static int nfs_dentry_delete(struct dentry *dentry)
 		 * files will be cleaned up during umount */
 		return 1;
 	}
+	if (dentry->d_inode == NULL &&
+	    (NFS_SB(dentry->d_sb)->flags & NFS_MOUNT_NONEGDE)) {
+		/* Unhash it, so that negative dentries won't be cached */
+		return 1;
+	}
 	return 0;
 
 }
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c
index 4b03345..1c8e7ee 100644
--- a/fs/nfs/nfsroot.c
+++ b/fs/nfs/nfsroot.c
@@ -124,7 +124,7 @@ enum {
 	Opt_soft, Opt_hard, Opt_intr,
 	Opt_nointr, Opt_posix, Opt_noposix, Opt_cto, Opt_nocto, Opt_ac, 
 	Opt_noac, Opt_lock, Opt_nolock, Opt_v2, Opt_v3, Opt_udp, Opt_tcp,
-	Opt_acl, Opt_noacl,
+	Opt_acl, Opt_noacl, Opt_negde, Opt_nonegde,
 	/* Error token */
 	Opt_err
 };
@@ -161,6 +161,8 @@ static match_table_t __initdata tokens = {
 	{Opt_tcp, "tcp"},
 	{Opt_acl, "acl"},
 	{Opt_noacl, "noacl"},
+	{Opt_negde, "negde"},
+	{Opt_nonegde, "nonegde"},
 	{Opt_err, NULL}
 	
 };
@@ -275,6 +277,12 @@ static int __init root_nfs_parse(char *name, char *buf)
 			case Opt_noacl:
 				nfs_data.flags |= NFS_MOUNT_NOACL;
 				break;
+			case Opt_negde:
+				nfs_data.flags &= ~NFS_MOUNT_NONEGDE;
+				break;
+			case Opt_nonegde:
+				nfs_data.flags |= NFS_MOUNT_NONEGDE;
+				break;
 			default:
 				printk(KERN_WARNING "Root-NFS: unknown "
 					"option: %s\n", p);
diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 0d1bc61..7043a07 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -75,6 +75,7 @@ enum {
 	Opt_acl, Opt_noacl,
 	Opt_rdirplus, Opt_nordirplus,
 	Opt_sharecache, Opt_nosharecache,
+	Opt_negde, Opt_nonegde,
 
 	/* Mount options that take integer arguments */
 	Opt_port,
@@ -124,6 +125,8 @@ static match_table_t nfs_mount_option_tokens = {
 	{ Opt_nordirplus, "nordirplus" },
 	{ Opt_sharecache, "sharecache" },
 	{ Opt_nosharecache, "nosharecache" },
+	{ Opt_negde, "negde" },
+	{ Opt_nonegde, "nonegde" },
 
 	{ Opt_port, "port=%u" },
 	{ Opt_rsize, "rsize=%u" },
@@ -455,6 +458,7 @@ static void nfs_show_mount_options(struct seq_file *m, struct nfs_server *nfss,
 		{ NFS_MOUNT_NOACL, ",noacl", "" },
 		{ NFS_MOUNT_NORDIRPLUS, ",nordirplus", "" },
 		{ NFS_MOUNT_UNSHARED, ",nosharecache", ""},
+		{ NFS_MOUNT_NONEGDE, ",nonegde", ""},
 		{ 0, NULL, NULL }
 	};
 	const struct proc_nfs_info *nfs_infop;
@@ -779,6 +783,11 @@ static int nfs_parse_mount_options(char *raw,
 		case Opt_nosharecache:
 			mnt->flags |= NFS_MOUNT_UNSHARED;
 			break;
+		case Opt_negde:
+			mnt->flags &= ~NFS_MOUNT_NONEGDE;
+			break;
+		case Opt_nonegde:
+			mnt->flags |= NFS_MOUNT_NONEGDE;
 
 		case Opt_port:
 			if (match_int(args, &option))
diff --git a/include/linux/nfs_mount.h b/include/linux/nfs_mount.h
index a3ade89..44ffb60 100644
--- a/include/linux/nfs_mount.h
+++ b/include/linux/nfs_mount.h
@@ -47,22 +47,23 @@ struct nfs_mount_data {
 
 /* bits in the flags field */
 
-#define NFS_MOUNT_SOFT		0x0001	/* 1 */
-#define NFS_MOUNT_INTR		0x0002	/* 1 */
-#define NFS_MOUNT_SECURE	0x0004	/* 1 */
-#define NFS_MOUNT_POSIX		0x0008	/* 1 */
-#define NFS_MOUNT_NOCTO		0x0010	/* 1 */
-#define NFS_MOUNT_NOAC		0x0020	/* 1 */
-#define NFS_MOUNT_TCP		0x0040	/* 2 */
-#define NFS_MOUNT_VER3		0x0080	/* 3 */
-#define NFS_MOUNT_KERBEROS	0x0100	/* 3 */
-#define NFS_MOUNT_NONLM		0x0200	/* 3 */
-#define NFS_MOUNT_BROKEN_SUID	0x0400	/* 4 */
-#define NFS_MOUNT_NOACL		0x0800	/* 4 */
-#define NFS_MOUNT_STRICTLOCK	0x1000	/* reserved for NFSv4 */
-#define NFS_MOUNT_SECFLAVOUR	0x2000	/* 5 */
-#define NFS_MOUNT_NORDIRPLUS	0x4000	/* 5 */
-#define NFS_MOUNT_UNSHARED	0x8000	/* 5 */
-#define NFS_MOUNT_FLAGMASK	0xFFFF
+#define NFS_MOUNT_SOFT		0x00001	/* 1 */
+#define NFS_MOUNT_INTR		0x00002	/* 1 */
+#define NFS_MOUNT_SECURE	0x00004	/* 1 */
+#define NFS_MOUNT_POSIX		0x00008	/* 1 */
+#define NFS_MOUNT_NOCTO		0x00010	/* 1 */
+#define NFS_MOUNT_NOAC		0x00020	/* 1 */
+#define NFS_MOUNT_TCP		0x00040	/* 2 */
+#define NFS_MOUNT_VER3		0x00080	/* 3 */
+#define NFS_MOUNT_KERBEROS	0x00100	/* 3 */
+#define NFS_MOUNT_NONLM		0x00200	/* 3 */
+#define NFS_MOUNT_BROKEN_SUID	0x00400	/* 4 */
+#define NFS_MOUNT_NOACL		0x00800	/* 4 */
+#define NFS_MOUNT_STRICTLOCK	0x01000	/* reserved for NFSv4 */
+#define NFS_MOUNT_SECFLAVOUR	0x02000	/* 5 */
+#define NFS_MOUNT_NORDIRPLUS	0x04000	/* 5 */
+#define NFS_MOUNT_UNSHARED	0x08000	/* 5 */
+#define NFS_MOUNT_NONEGDE	0x10000	/* 5 */
+#define NFS_MOUNT_FLAGMASK	0x1FFFF
 
 #endif

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Trond Myklebust]

On Tue, 2008-01-15 at 11:30 -0500, Bob Bell wrote:
> From: Bob Bell <bell_robert@emc.com>
> 
> Add NFS_MOUNT_NONEGDE mount flag.  When this flag is set on a mounted
> NFS filesystem, negative dentries for that filesystem will not be 
> cached.  This can help address cache coherency when the NFS server does 
> not provide sufficiently fine-grained timestamps to consistently 
> distinguish when a directory has been modified, though performance when 
> repeatedly accessing filenames for files that do not exist may be 
> impacted.
> 
> Signed-off-by: Bob Bell <bell_robert@emc.com>
> ---

The patch itself looks OK. My only gripe is the two names 'negde' and
'nonegde'. Can we find something that rolls a bit more smoothly off the
tongue?

How about something like 'cacheneglookup' 'nocacheneglookup'? Better
suggestions, anyone?

Cheers
  Trond

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Chuck Lever]

On Jan 15, 2008, at 11:52 AM, Trond Myklebust wrote:
> On Tue, 2008-01-15 at 11:30 -0500, Bob Bell wrote:
>> From: Bob Bell <bell_robert@emc.com>
>>
>> Add NFS_MOUNT_NONEGDE mount flag.  When this flag is set on a mounted
>> NFS filesystem, negative dentries for that filesystem will not be
>> cached.  This can help address cache coherency when the NFS server  
>> does
>> not provide sufficiently fine-grained timestamps to consistently
>> distinguish when a directory has been modified, though performance  
>> when
>> repeatedly accessing filenames for files that do not exist may be
>> impacted.
>>
>> Signed-off-by: Bob Bell <bell_robert@emc.com>
>> ---
>
> The patch itself looks OK. My only gripe is the two names 'negde' and
> 'nonegde'. Can we find something that rolls a bit more smoothly off  
> the
> tongue?

Yeah, actually I thought it was spelled "non edge" until I looked  
closer.

> How about something like 'cacheneglookup' 'nocacheneglookup'? Better
> suggestions, anyone?


I suspect system administrators have no idea what a "negative dentry"  
is, and it would probably be beyond the scope of nfs(5) to explain  
it.  So I vote for something that describes the behavior as observed  
by a "lay person" rather than something that names the kernel  
mechanisms involved.

You might consider tying this behavior to "noac" as well.  In other  
words, make "noac" equivalent to "sync,actimeo=0,nonegde" .

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

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Trond Myklebust]

On Tue, 2008-01-15 at 12:04 -0500, Chuck Lever wrote:
> 
> I suspect system administrators have no idea what a "negative dentry"  
> is, and it would probably be beyond the scope of nfs(5) to explain  
> it.  So I vote for something that describes the behavior as observed  
> by a "lay person" rather than something that names the kernel  
> mechanisms involved.

The concept of a 'dentry' is in any case very Linux-specific. The more
generic term would be a 'negative lookup'.

That said, if someone can find a better name, I'd be all for it.

> You might consider tying this behavior to "noac" as well.  In other  
> words, make "noac" equivalent to "sync,actimeo=0,nonegde" .

I'd be open for that idea, however that should definitely be a separate
patch.

Cheers
  Trond

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Bob Bell]

On Tue, Jan 15, 2008 at 12:04:50PM -0500, Chuck Lever wrote:
>On Jan 15, 2008, at 11:52 AM, Trond Myklebust wrote:
>>The patch itself looks OK. My only gripe is the two names 'negde' and
>>'nonegde'. Can we find something that rolls a bit more smoothly off  
>>the
>>tongue?
>
>Yeah, actually I thought it was spelled "non edge" until I looked  
>closer.

Heh -- I mentioned the same thing at the bottom of my (admittedly 
verbose) intro the patch.  In fact, I've mistyped it "nonedge" myself 
a few times, which I figured was a strong hint it needed renaming.

I was going for the terseness I saw in the rest of the options, and 
"nonegde" was the best my limited creativity could come up with.

>>How about something like 'cacheneglookup' 'nocacheneglookup'? Better
>>suggestions, anyone?

As long as no one objects to the length, I'll go with "nocacheneglookup" 
(and NFS_MOUNT_NOCACHENEGLOOKUP, I suppose) for round 2.

>You might consider tying this behavior to "noac" as well.  In other  
>words, make "noac" equivalent to "sync,actimeo=0,nonegde" .

I'll provide this as a separate patch in the patch set, as Trond 
indicated he would prefer.  This might make sense, as this is probably 
the set of options we'll used.

-- 
Bob Bell

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Trond Myklebust]

On Tue, 2008-01-15 at 20:22 -0500, Bob Bell wrote:
> >>How about something like 'cacheneglookup' 'nocacheneglookup'? Better
> >>suggestions, anyone?
> 
> As long as no one objects to the length, I'll go with "nocacheneglookup" 
> (and NFS_MOUNT_NOCACHENEGLOOKUP, I suppose) for round 2.

I'm still not happy with that name. What do people think about
introducing a generic 'cache' option that can be used for fine control
of caching options?

I'm thinking somewhat along the lines of

	-ocache=lookup_all		/* Cache all dentries */
	-ocache=lookup_positive		/* Cache positive dentries */
	-ocache=lookup_none		/* Force all lookups */

...and then possibly adding other 'cache=' options later if we find out
that we need them?

Cheers
  Trond

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Steve Dickson]

Trond Myklebust wrote:
> On Tue, 2008-01-15 at 20:22 -0500, Bob Bell wrote:
>>>> How about something like 'cacheneglookup' 'nocacheneglookup'? Better
>>>> suggestions, anyone?
>> As long as no one objects to the length, I'll go with "nocacheneglookup" 
>> (and NFS_MOUNT_NOCACHENEGLOOKUP, I suppose) for round 2.
> 
> I'm still not happy with that name. What do people think about
> introducing a generic 'cache' option that can be used for fine control
> of caching options?
> 
> I'm thinking somewhat along the lines of
> 
> 	-ocache=lookup_all		/* Cache all dentries */
> 	-ocache=lookup_positive		/* Cache positive dentries */
> 	-ocache=lookup_none		/* Force all lookups */
Maybe we should define the type of cache were are controlling, in case
we want to add controls for other caches. So how about:

        -olpcache=all        /* Cache all directory entries */
        -olpcache=found      /* Cache only directory entires that are found */
        -olpcache=off        /* Turn off the caching of directory entries */ 


steved.

RE: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Muntz, Daniel]

My $.02

 -odircache=on
 -odircache=hitsonly (or positive, or hits, or ?)
 -odircache=off

-----Original Message-----
From: Steve Dickson [mailto:SteveD@redhat.com] 
Sent: Wednesday, January 16, 2008 4:50 AM
To: Trond Myklebust
Cc: Bob Bell; Chuck Lever; NFS list
Subject: Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid
caching negative dentries

Trond Myklebust wrote:
> On Tue, 2008-01-15 at 20:22 -0500, Bob Bell wrote:
>>>> How about something like 'cacheneglookup' 'nocacheneglookup'? 
>>>> Better suggestions, anyone?
>> As long as no one objects to the length, I'll go with
"nocacheneglookup" 
>> (and NFS_MOUNT_NOCACHENEGLOOKUP, I suppose) for round 2.
> 
> I'm still not happy with that name. What do people think about 
> introducing a generic 'cache' option that can be used for fine control

> of caching options?
> 
> I'm thinking somewhat along the lines of
> 
> 	-ocache=lookup_all		/* Cache all dentries */
> 	-ocache=lookup_positive		/* Cache positive dentries */
> 	-ocache=lookup_none		/* Force all lookups */
Maybe we should define the type of cache were are controlling, in case
we want to add controls for other caches. So how about:

        -olpcache=all        /* Cache all directory entries */
        -olpcache=found      /* Cache only directory entires that are
found */
        -olpcache=off        /* Turn off the caching of directory
entries */ 


steved.
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org More majordomo info
at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Trond Myklebust]

On Wed, 2008-01-16 at 11:43 -0800, Muntz, Daniel wrote:
> My $.02
> 
>  -odircache=on
>  -odircache=hitsonly (or positive, or hits, or ?)
>  -odircache=off


I like the 'dircache' bit, but to me 'on/off' suggests a binary switch:
they don't mix well with a third state.

How about

        -odircache=aggr[essive]	/* Full caching */
        -odircache=noneg[ative]	/* Positive lookups only */
        -odircache=off		/* strict lookup revalidation */

?


> -----Original Message-----
> From: Steve Dickson [mailto:SteveD@redhat.com] 
> Sent: Wednesday, January 16, 2008 4:50 AM
> To: Trond Myklebust
> Cc: Bob Bell; Chuck Lever; NFS list
> Subject: Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid
> caching negative dentries
> 
> Trond Myklebust wrote:
> > On Tue, 2008-01-15 at 20:22 -0500, Bob Bell wrote:
> >>>> How about something like 'cacheneglookup' 'nocacheneglookup'? 
> >>>> Better suggestions, anyone?
> >> As long as no one objects to the length, I'll go with
> "nocacheneglookup" 
> >> (and NFS_MOUNT_NOCACHENEGLOOKUP, I suppose) for round 2.
> > 
> > I'm still not happy with that name. What do people think about 
> > introducing a generic 'cache' option that can be used for fine control
> 
> > of caching options?
> > 
> > I'm thinking somewhat along the lines of
> > 
> > 	-ocache=lookup_all		/* Cache all dentries */
> > 	-ocache=lookup_positive		/* Cache positive dentries */
> > 	-ocache=lookup_none		/* Force all lookups */
> Maybe we should define the type of cache were are controlling, in case
> we want to add controls for other caches. So how about:
> 
>         -olpcache=all        /* Cache all directory entries */
>         -olpcache=found      /* Cache only directory entires that are
> found */
>         -olpcache=off        /* Turn off the caching of directory
> entries */ 
> 
> 
> steved.
> -
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org More majordomo info
> at  http://vger.kernel.org/majordomo-info.html
> -
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Chuck Lever]

On Jan 16, 2008, at 3:43 PM, Trond Myklebust wrote:
> On Wed, 2008-01-16 at 11:43 -0800, Muntz, Daniel wrote:
>> My $.02
>>
>>  -odircache=on
>>  -odircache=hitsonly (or positive, or hits, or ?)
>>  -odircache=off
>
>
> I like the 'dircache' bit, but to me 'on/off' suggests a binary  
> switch:
> they don't mix well with a third state.
>
> How about
>
>         -odircache=aggr[essive]	/* Full caching */
>         -odircache=noneg[ative]	/* Positive lookups only */
>         -odircache=off		/* strict lookup revalidation */

  "-olookupcache=" would be even more specific, if not more verbose.   
dircache=off implies that not even readdir results are cached.

	-olookupcache=full
	-olookupcache=pos[itive]
	-olookupcache=strict

>> -----Original Message-----
>> From: Steve Dickson [mailto:SteveD@redhat.com]
>> Sent: Wednesday, January 16, 2008 4:50 AM
>> To: Trond Myklebust
>> Cc: Bob Bell; Chuck Lever; NFS list
>> Subject: Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid
>> caching negative dentries
>>
>> Trond Myklebust wrote:
>>> On Tue, 2008-01-15 at 20:22 -0500, Bob Bell wrote:
>>>>>> How about something like 'cacheneglookup' 'nocacheneglookup'?
>>>>>> Better suggestions, anyone?
>>>> As long as no one objects to the length, I'll go with
>> "nocacheneglookup"
>>>> (and NFS_MOUNT_NOCACHENEGLOOKUP, I suppose) for round 2.
>>>
>>> I'm still not happy with that name. What do people think about
>>> introducing a generic 'cache' option that can be used for fine  
>>> control
>>
>>> of caching options?
>>>
>>> I'm thinking somewhat along the lines of
>>>
>>> 	-ocache=lookup_all		/* Cache all dentries */
>>> 	-ocache=lookup_positive		/* Cache positive dentries */
>>> 	-ocache=lookup_none		/* Force all lookups */
>> Maybe we should define the type of cache were are controlling, in  
>> case
>> we want to add controls for other caches. So how about:
>>
>>         -olpcache=all        /* Cache all directory entries */
>>         -olpcache=found      /* Cache only directory entires that are
>> found */
>>         -olpcache=off        /* Turn off the caching of directory
>> entries */
>>
>>
>> steved.
>> -
>> To unsubscribe from this list: send the line "unsubscribe linux- 
>> nfs" in
>> the body of a message to majordomo@vger.kernel.org More majordomo  
>> info
>> at  http://vger.kernel.org/majordomo-info.html
>> -
>> To unsubscribe from this list: send the line "unsubscribe linux- 
>> nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Risto Bell]

Already decided not to go the distance, with separate controls of
time-to-live of positive and negative lookup cache entries (or is it
not considered good form to overload negttl=0 as means to disable)?

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Trond Myklebust]

On Wed, 2008-01-16 at 13:12 -0800, Risto Bell wrote:
> Already decided not to go the distance, with separate controls of
> time-to-live of positive and negative lookup cache entries (or is it
> not considered good form to overload negttl=0 as means to disable)?

We don't have a time-to-live scheme for dentries. When deciding whether
we need to revalidate the lookup, we check the parent directory mtime,
and see if the directory contents have changed since the last
revalidation.

Trond

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Chuck Lever]

On Jan 16, 2008, at 4:56 PM, Trond Myklebust wrote:
> On Wed, 2008-01-16 at 13:12 -0800, Risto Bell wrote:
>> Already decided not to go the distance, with separate controls of
>> time-to-live of positive and negative lookup cache entries (or is it
>> not considered good form to overload negttl=0 as means to disable)?
>
> We don't have a time-to-live scheme for dentries.

Adding a TTL heuristic to dentry revalidation might help our  
directory mtime problem.  :-)

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

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Trond Myklebust]

On Wed, 2008-01-16 at 17:09 -0500, Chuck Lever wrote:
> On Jan 16, 2008, at 4:56 PM, Trond Myklebust wrote:
> > On Wed, 2008-01-16 at 13:12 -0800, Risto Bell wrote:
> >> Already decided not to go the distance, with separate controls of
> >> time-to-live of positive and negative lookup cache entries (or is it
> >> not considered good form to overload negttl=0 as means to disable)?
> >
> > We don't have a time-to-live scheme for dentries.
> 
> Adding a TTL heuristic to dentry revalidation might help our  
> directory mtime problem.  :-)

I have a serious problem seeing how it might 'solve' anything at all.

If you're worried about correctness, then ttl is a completely useless
scheme: you can defeat it just as easily as you can defeat the mtime
resolution. The only difference is that you will eventually catch onto
the mistake, but you may have been operating on the wrong file for quite
some time before the ttl expires.
IOW: The only useful ttl values are ttl=infinity and ttl=0.

Trond

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Bob Bell]

On Wed, Jan 16, 2008 at 03:49:20PM -0500, Chuck Lever wrote:
>On Jan 16, 2008, at 3:43 PM, Trond Myklebust wrote:
>>How about
>>         -odircache=aggr[essive]	/* Full caching */
>>         -odircache=noneg[ative]	/* Positive lookups only */
>>         -odircache=off		/* strict lookup revalidation */
>
>  "-olookupcache=" would be even more specific, if not more verbose.   
>dircache=off implies that not even readdir results are cached.
>
>	-olookupcache=full
>	-olookupcache=pos[itive]
>	-olookupcache=strict

I think that "lookupcache" is a little more accurate, and perhaps worth 
the verbosity.  "dircache" could be misinterpreted to imply that 
directory listings are being cached -- which, incidentally, I'm start to 
receive complaints is a problem for us, too...

Converstation on this seems to have died down, and I'm ready to revisit 
the patch.  I'm inclined to go with:
    -o lookupcache=full
    -o lookupcache=pos[itive]
    -o lookupcache=none

If you have a strong (enough) opinion otherwise, please speak up now and 
save me the trouble of an extra pass at the patch...

-- 
Bob Bell

RE: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Muntz, Daniel]

How about "all" instead of "full" ("all" being the opposite of "none")?
A cache being "full", although not making sense in this context, does
have another meaning. 

-----Original Message-----
From: Bob Bell [mailto:b_linuxnfs-Y/+76LoPTq9wBoktGHYdvgC/G2K4zDHf@public.gmane.org] 
Sent: Friday, January 18, 2008 7:29 AM
To: Chuck Lever
Cc: Trond Myklebust; Muntz, Daniel; Steve Dickson; NFS list
Subject: Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid
caching negative dentries

On Wed, Jan 16, 2008 at 03:49:20PM -0500, Chuck Lever wrote:
>On Jan 16, 2008, at 3:43 PM, Trond Myklebust wrote:
>>How about
>>         -odircache=aggr[essive]	/* Full caching */
>>         -odircache=noneg[ative]	/* Positive lookups only */
>>         -odircache=off		/* strict lookup revalidation */
>
>  "-olookupcache=" would be even more specific, if not more verbose.   
>dircache=off implies that not even readdir results are cached.
>
>	-olookupcache=full
>	-olookupcache=pos[itive]
>	-olookupcache=strict

I think that "lookupcache" is a little more accurate, and perhaps worth
the verbosity.  "dircache" could be misinterpreted to imply that
directory listings are being cached -- which, incidentally, I'm start to
receive complaints is a problem for us, too...

Converstation on this seems to have died down, and I'm ready to revisit
the patch.  I'm inclined to go with:
    -o lookupcache=full
    -o lookupcache=pos[itive]
    -o lookupcache=none

If you have a strong (enough) opinion otherwise, please speak up now and
save me the trouble of an extra pass at the patch...

--
Bob Bell

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Bob Bell]

On Fri, Jan 18, 2008 at 12:00:38PM -0800, Muntz, Daniel wrote:
> How about "all" instead of "full" ("all" being the opposite of "none")?
> A cache being "full", although not making sense in this context, does
> have another meaning. 

That makes sense to me.

-- 
Bob Bell

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Chuck Lever]

Hi Bob-

On Jan 18, 2008, at 10:29 AM, Bob Bell wrote:
> On Wed, Jan 16, 2008 at 03:49:20PM -0500, Chuck Lever wrote:
>> On Jan 16, 2008, at 3:43 PM, Trond Myklebust wrote:
>>> How about
>>>         -odircache=aggr[essive]	/* Full caching */
>>>         -odircache=noneg[ative]	/* Positive lookups only */
>>>         -odircache=off		/* strict lookup revalidation */
>>
>>  "-olookupcache=" would be even more specific, if not more  
>> verbose.   dircache=off implies that not even readdir results are  
>> cached.
>>
>> 	-olookupcache=full
>> 	-olookupcache=pos[itive]
>> 	-olookupcache=strict
>
> I think that "lookupcache" is a little more accurate, and perhaps  
> worth the verbosity.  "dircache" could be misinterpreted to imply  
> that directory listings are being cached -- which, incidentally,  
> I'm start to receive complaints is a problem for us, too...
>
> Converstation on this seems to have died down, and I'm ready to  
> revisit the patch.  I'm inclined to go with:
>    -o lookupcache=full
>    -o lookupcache=pos[itive]
>    -o lookupcache=none
>
> If you have a strong (enough) opinion otherwise, please speak up  
> now and save me the trouble of an extra pass at the patch...


I understand that lookupcache=full would be current behavior, and  
=positive would prevent caching negative dentries.

When would anyone want to use lookupcache=none -- what exactly would  
it do?  How is it different than actimeo=0?

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

Re: [PATCH 2/3] NFS: Add NFS_MOUNT_NONEGDE flag to avoid caching negative dentries

[Trond Myklebust]

On Fri, 2008-01-18 at 16:12 -0500, Chuck Lever wrote:

> When would anyone want to use lookupcache=none -- what exactly would  
> it do?  How is it different than actimeo=0?

It would be very different. The idea of lookupcache=none would be to
turn off the directory mtime-based revalidation algorithm in
nfs_lookup_revalidate().
IOW: you would force an on-the-wire LOOKUP call for each dentry
revalidation.

Cheers
  Trond

[PATCH 3/3] nfs-utils: Add nonegde mount option

[Bob Bell]

From: Bob Bell <bell_robert@emc.com>

Support a "nonegde" mount option to match the new NFS_MOUNT_NONEGDE 
flag.

Signed-off-by: Bob Bell <bell_robert@emc.com>
---
 utils/mount/nfs.man     |   18 ++++++++++++++++++
 utils/mount/nfs_mount.h |   31 ++++++++++++++++---------------
 utils/mount/nfsmount.c  |    9 +++++++--
 3 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/utils/mount/nfs.man b/utils/mount/nfs.man
index 2c0b687..b0a885a 100644
--- a/utils/mount/nfs.man
+++ b/utils/mount/nfs.man
@@ -613,6 +613,24 @@ If this option is not specified, the NFS client uses READDIRPLUS requests
 on NFS version 3 mounts to read small directories.
 Some applications perform better if the client uses only READDIR requests
 for all directories.  
+.TP 1.5i
+.BR negde " / " nonegde
+Selects whether to cache negative dentries,
+which record the non-existence of a file.
+If neither option is specified (or if 
+.B negde
+is specified),
+each negative dentry will be kept in the kernel dcache until it is invalidated
+when a change is detected in its parent directory,
+or until sufficient memory pressure forces it out.
+.IP
+Using the
+.B nonegde
+option may be helpful if the NFS server does not provide
+sufficiently fine-grained timestamps to consistently distinguish when the
+contents of a directory have been updated.  However, by not caching negative
+dentries, the client will have to contact the server whenever there is an
+attempt to access a previously non-existent file, which may impact performance.
 .SS "Valid options for the nfs4 file system type"
 Use these options, along with the options in the first subsection above,
 for mounting the
diff --git a/utils/mount/nfs_mount.h b/utils/mount/nfs_mount.h
index 7df8fb2..2deca87 100644
--- a/utils/mount/nfs_mount.h
+++ b/utils/mount/nfs_mount.h
@@ -50,21 +50,22 @@ struct nfs_mount_data {
 
 /* bits in the flags field */
 
-#define NFS_MOUNT_SOFT		0x0001	/* 1 */
-#define NFS_MOUNT_INTR		0x0002	/* 1 */
-#define NFS_MOUNT_SECURE	0x0004	/* 1 */
-#define NFS_MOUNT_POSIX		0x0008	/* 1 */
-#define NFS_MOUNT_NOCTO		0x0010	/* 1 */
-#define NFS_MOUNT_NOAC		0x0020	/* 1 */
-#define NFS_MOUNT_TCP		0x0040	/* 2 */
-#define NFS_MOUNT_VER3		0x0080	/* 3 */
-#define NFS_MOUNT_KERBEROS	0x0100	/* 3 */
-#define NFS_MOUNT_NONLM		0x0200	/* 3 */
-#define NFS_MOUNT_BROKEN_SUID	0x0400	/* 4 */
-#define NFS_MOUNT_NOACL     0x0800  /* 4 */
-#define NFS_MOUNT_SECFLAVOUR	0x2000	/* 5 */
-#define NFS_MOUNT_NORDIRPLUS	0x4000	/* 5 */
-#define NFS_MOUNT_UNSHARED	0x8000	/* 5 */
+#define NFS_MOUNT_SOFT		0x00001	/* 1 */
+#define NFS_MOUNT_INTR		0x00002	/* 1 */
+#define NFS_MOUNT_SECURE	0x00004	/* 1 */
+#define NFS_MOUNT_POSIX		0x00008	/* 1 */
+#define NFS_MOUNT_NOCTO		0x00010	/* 1 */
+#define NFS_MOUNT_NOAC		0x00020	/* 1 */
+#define NFS_MOUNT_TCP		0x00040	/* 2 */
+#define NFS_MOUNT_VER3		0x00080	/* 3 */
+#define NFS_MOUNT_KERBEROS	0x00100	/* 3 */
+#define NFS_MOUNT_NONLM		0x00200	/* 3 */
+#define NFS_MOUNT_BROKEN_SUID	0x00400	/* 4 */
+#define NFS_MOUNT_NOACL		0x00800	/* 4 */
+#define NFS_MOUNT_SECFLAVOUR	0x02000	/* 5 */
+#define NFS_MOUNT_NORDIRPLUS	0x04000	/* 5 */
+#define NFS_MOUNT_UNSHARED	0x08000	/* 5 */
+#define NFS_MOUNT_NONEGDE	0x10000	/* 5 */
 
 /* security pseudoflavors */
 
diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
index 23dd2f6..81ab371 100644
--- a/utils/mount/nfsmount.c
+++ b/utils/mount/nfsmount.c
@@ -422,6 +422,10 @@ parse_options(char *old_opts, struct nfs_mount_data *data,
 				if (!val)
 					data->flags |= NFS_MOUNT_UNSHARED;
 #endif
+			} else if (!strcmp(opt, "negde")) {
+				data->flags &= ~NFS_MOUNT_NONEGDE;
+				if (!val)
+					data->flags |= NFS_MOUNT_NONEGDE;
 			} else {
 			bad_option:
 				if (sloppy)
@@ -595,12 +599,13 @@ nfsmount(const char *spec, const char *node, int flags,
 	printf(_("mountprog = %lu, mountvers = %lu, nfsprog = %lu, nfsvers = %lu\n"),
 	       mnt_pmap->pm_prog, mnt_pmap->pm_vers,
 	       nfs_pmap->pm_prog, nfs_pmap->pm_vers);
-	printf(_("soft = %d, intr = %d, posix = %d, nocto = %d, noac = %d"),
+	printf(_("soft = %d, intr = %d, posix = %d, nocto = %d, noac = %d, nonegde = %d"),
 	       (data.flags & NFS_MOUNT_SOFT) != 0,
 	       (data.flags & NFS_MOUNT_INTR) != 0,
 	       (data.flags & NFS_MOUNT_POSIX) != 0,
 	       (data.flags & NFS_MOUNT_NOCTO) != 0,
-	       (data.flags & NFS_MOUNT_NOAC) != 0);
+	       (data.flags & NFS_MOUNT_NOAC) != 0,
+	       (data.flags & NFS_MOUNT_NONEGDE) != 0);
 #if NFS_MOUNT_VERSION >= 2
 	printf(_(", tcp = %d"),
 	       (data.flags & NFS_MOUNT_TCP) != 0);

Re: [PATCH 3/3] nfs-utils: Add nonegde mount option

[Chuck Lever]

On Jan 15, 2008, at 11:31 AM, Bob Bell wrote:
> From: Bob Bell <bell_robert@emc.com>
>
> Support a "nonegde" mount option to match the new NFS_MOUNT_NONEGDE  
> flag.
>
> Signed-off-by: Bob Bell <bell_robert@emc.com>
> ---
> utils/mount/nfs.man     |   18 ++++++++++++++++++
> utils/mount/nfs_mount.h |   31 ++++++++++++++++---------------
> utils/mount/nfsmount.c  |    9 +++++++--
> 3 files changed, 41 insertions(+), 17 deletions(-)
>
> diff --git a/utils/mount/nfs.man b/utils/mount/nfs.man
> index 2c0b687..b0a885a 100644
> --- a/utils/mount/nfs.man
> +++ b/utils/mount/nfs.man
> @@ -613,6 +613,24 @@ If this option is not specified, the NFS  
> client uses READDIRPLUS requests
> on NFS version 3 mounts to read small directories.
> Some applications perform better if the client uses only READDIR  
> requests
> for all directories.  +.TP 1.5i
> +.BR negde " / " nonegde
> +Selects whether to cache negative dentries,
> +which record the non-existence of a file.
> +If neither option is specified (or if +.B negde
> +is specified),
> +each negative dentry will be kept in the kernel dcache until it is  
> invalidated
> +when a change is detected in its parent directory,
> +or until sufficient memory pressure forces it out.
> +.IP
> +Using the
> +.B nonegde
> +option may be helpful if the NFS server does not provide
> +sufficiently fine-grained timestamps to consistently distinguish  
> when the
> +contents of a directory have been updated.  However, by not  
> caching negative
> +dentries, the client will have to contact the server whenever  
> there is an
> +attempt to access a previously non-existent file, which may impact  
> performance.
> .SS "Valid options for the nfs4 file system type"
> Use these options, along with the options in the first subsection  
> above,
> for mounting the
> diff --git a/utils/mount/nfs_mount.h b/utils/mount/nfs_mount.h
> index 7df8fb2..2deca87 100644
> --- a/utils/mount/nfs_mount.h
> +++ b/utils/mount/nfs_mount.h
> @@ -50,21 +50,22 @@ struct nfs_mount_data {
> /* bits in the flags field */
> -#define NFS_MOUNT_SOFT		0x0001	/* 1 */
> -#define NFS_MOUNT_INTR		0x0002	/* 1 */
> -#define NFS_MOUNT_SECURE	0x0004	/* 1 */
> -#define NFS_MOUNT_POSIX		0x0008	/* 1 */
> -#define NFS_MOUNT_NOCTO		0x0010	/* 1 */
> -#define NFS_MOUNT_NOAC		0x0020	/* 1 */
> -#define NFS_MOUNT_TCP		0x0040	/* 2 */
> -#define NFS_MOUNT_VER3		0x0080	/* 3 */
> -#define NFS_MOUNT_KERBEROS	0x0100	/* 3 */
> -#define NFS_MOUNT_NONLM		0x0200	/* 3 */
> -#define NFS_MOUNT_BROKEN_SUID	0x0400	/* 4 */
> -#define NFS_MOUNT_NOACL     0x0800  /* 4 */
> -#define NFS_MOUNT_SECFLAVOUR	0x2000	/* 5 */
> -#define NFS_MOUNT_NORDIRPLUS	0x4000	/* 5 */
> -#define NFS_MOUNT_UNSHARED	0x8000	/* 5 */
> +#define NFS_MOUNT_SOFT		0x00001	/* 1 */
> +#define NFS_MOUNT_INTR		0x00002	/* 1 */
> +#define NFS_MOUNT_SECURE	0x00004	/* 1 */
> +#define NFS_MOUNT_POSIX		0x00008	/* 1 */
> +#define NFS_MOUNT_NOCTO		0x00010	/* 1 */
> +#define NFS_MOUNT_NOAC		0x00020	/* 1 */
> +#define NFS_MOUNT_TCP		0x00040	/* 2 */
> +#define NFS_MOUNT_VER3		0x00080	/* 3 */
> +#define NFS_MOUNT_KERBEROS	0x00100	/* 3 */
> +#define NFS_MOUNT_NONLM		0x00200	/* 3 */
> +#define NFS_MOUNT_BROKEN_SUID	0x00400	/* 4 */
> +#define NFS_MOUNT_NOACL		0x00800	/* 4 */
> +#define NFS_MOUNT_SECFLAVOUR	0x02000	/* 5 */
> +#define NFS_MOUNT_NORDIRPLUS	0x04000	/* 5 */
> +#define NFS_MOUNT_UNSHARED	0x08000	/* 5 */
> +#define NFS_MOUNT_NONEGDE	0x10000	/* 5 */

Why is the patch replacing all the flag definitions?  Did you check  
for inadvertent white space damage?

> /* security pseudoflavors */
> diff --git a/utils/mount/nfsmount.c b/utils/mount/nfsmount.c
> index 23dd2f6..81ab371 100644
> --- a/utils/mount/nfsmount.c
> +++ b/utils/mount/nfsmount.c
> @@ -422,6 +422,10 @@ parse_options(char *old_opts, struct  
> nfs_mount_data *data,
> 				if (!val)
> 					data->flags |= NFS_MOUNT_UNSHARED;
> #endif
> +			} else if (!strcmp(opt, "negde")) {
> +				data->flags &= ~NFS_MOUNT_NONEGDE;
> +				if (!val)
> +					data->flags |= NFS_MOUNT_NONEGDE;
> 			} else {
> 			bad_option:
> 				if (sloppy)
> @@ -595,12 +599,13 @@ nfsmount(const char *spec, const char *node,  
> int flags,
> 	printf(_("mountprog = %lu, mountvers = %lu, nfsprog = %lu, nfsvers  
> = %lu\n"),
> 	       mnt_pmap->pm_prog, mnt_pmap->pm_vers,
> 	       nfs_pmap->pm_prog, nfs_pmap->pm_vers);
> -	printf(_("soft = %d, intr = %d, posix = %d, nocto = %d, noac = %d"),
> +	printf(_("soft = %d, intr = %d, posix = %d, nocto = %d, noac = % 
> d, nonegde = %d"),
> 	       (data.flags & NFS_MOUNT_SOFT) != 0,
> 	       (data.flags & NFS_MOUNT_INTR) != 0,
> 	       (data.flags & NFS_MOUNT_POSIX) != 0,
> 	       (data.flags & NFS_MOUNT_NOCTO) != 0,
> -	       (data.flags & NFS_MOUNT_NOAC) != 0);
> +	       (data.flags & NFS_MOUNT_NOAC) != 0,
> +	       (data.flags & NFS_MOUNT_NONEGDE) != 0);
> #if NFS_MOUNT_VERSION >= 2
> 	printf(_(", tcp = %d"),
> 	       (data.flags & NFS_MOUNT_TCP) != 0);
> -
> To unsubscribe from this list: send the line "unsubscribe linux- 
> nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


As I understand it, we aren't adding new options to the legacy part  
of the nfs-utils mount command any longer.  Instead, add support for  
the option in the kernel's NFS mount option parser in fs/nfs/super.c.

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

Re: [PATCH 3/3] nfs-utils: Add nonegde mount option

[Bob Bell]

On Tue, Jan 15, 2008 at 11:42:54AM -0500, Chuck Lever wrote:
>On Jan 15, 2008, at 11:31 AM, Bob Bell wrote:
>>diff --git a/utils/mount/nfs_mount.h b/utils/mount/nfs_mount.h
>>index 7df8fb2..2deca87 100644
>>--- a/utils/mount/nfs_mount.h
>>+++ b/utils/mount/nfs_mount.h
>>@@ -50,21 +50,22 @@ struct nfs_mount_data {
>>/* bits in the flags field */
>>-#define NFS_MOUNT_SOFT		0x0001	/* 1 */
>>-#define NFS_MOUNT_INTR		0x0002	/* 1 */
>>-#define NFS_MOUNT_SECURE	0x0004	/* 1 */
>>-#define NFS_MOUNT_POSIX		0x0008	/* 1 */
>>-#define NFS_MOUNT_NOCTO		0x0010	/* 1 */
>>-#define NFS_MOUNT_NOAC		0x0020	/* 1 */
>>-#define NFS_MOUNT_TCP		0x0040	/* 2 */
>>-#define NFS_MOUNT_VER3		0x0080	/* 3 */
>>-#define NFS_MOUNT_KERBEROS	0x0100	/* 3 */
>>-#define NFS_MOUNT_NONLM		0x0200	/* 3 */
>>-#define NFS_MOUNT_BROKEN_SUID	0x0400	/* 4 */
>>-#define NFS_MOUNT_NOACL     0x0800  /* 4 */
>>-#define NFS_MOUNT_SECFLAVOUR	0x2000	/* 5 */
>>-#define NFS_MOUNT_NORDIRPLUS	0x4000	/* 5 */
>>-#define NFS_MOUNT_UNSHARED	0x8000	/* 5 */
>>+#define NFS_MOUNT_SOFT		0x00001	/* 1 */
>>+#define NFS_MOUNT_INTR		0x00002	/* 1 */
>>+#define NFS_MOUNT_SECURE	0x00004	/* 1 */
>>+#define NFS_MOUNT_POSIX		0x00008	/* 1 */
>>+#define NFS_MOUNT_NOCTO		0x00010	/* 1 */
>>+#define NFS_MOUNT_NOAC		0x00020	/* 1 */
>>+#define NFS_MOUNT_TCP		0x00040	/* 2 */
>>+#define NFS_MOUNT_VER3		0x00080	/* 3 */
>>+#define NFS_MOUNT_KERBEROS	0x00100	/* 3 */
>>+#define NFS_MOUNT_NONLM		0x00200	/* 3 */
>>+#define NFS_MOUNT_BROKEN_SUID	0x00400	/* 4 */
>>+#define NFS_MOUNT_NOACL		0x00800	/* 4 */
>>+#define NFS_MOUNT_SECFLAVOUR	0x02000	/* 5 */
>>+#define NFS_MOUNT_NORDIRPLUS	0x04000	/* 5 */
>>+#define NFS_MOUNT_UNSHARED	0x08000	/* 5 */
>>+#define NFS_MOUNT_NONEGDE	0x10000	/* 5 */
>
>Why is the patch replacing all the flag definitions?  Did you check  
>for inadvertent white space damage?

Actually, I fixed one bit of erroneous whitespace I found (spaces 
instead of tab).  The reason the definitions all change is because 
a leading 0.  The existing flags were 0x0001 through 0x8000.  The new 
flag is 0x10000.  I thought would be nice to make the flags all 5 digits 
so that they visually lined up.  I don't have to do that; it does make 
the patch uglier, but IMHO I think the resulting file is nicer.

>As I understand it, we aren't adding new options to the legacy part  of 
>the nfs-utils mount command any longer.  Instead, add support for  the 
>option in the kernel's NFS mount option parser in fs/nfs/super.c.

Oh, perhaps I didn't fully investigate the context.  I was just 
following the pattern I saw.  I can see if it still works without that 
one snippet (I believe I already updated fs/nfs/super.c appropriately).  
I presume updating the man page is still appropriate though, correct?  
What about defining the flag?  If it's not going to be used, should it 
still be defined as a placeholder?  I'll default to "yes" unless I hear 
otherwise.

-- 
Bob Bell

Re: [PATCH 3/3] nfs-utils: Add nonegde mount option

[Chuck Lever]

Hey Bob -

On Jan 15, 2008, at 8:13 PM, Bob Bell wrote:
> On Tue, Jan 15, 2008 at 11:42:54AM -0500, Chuck Lever wrote:
>> On Jan 15, 2008, at 11:31 AM, Bob Bell wrote:
>>> diff --git a/utils/mount/nfs_mount.h b/utils/mount/nfs_mount.h
>>> index 7df8fb2..2deca87 100644
>>> --- a/utils/mount/nfs_mount.h
>>> +++ b/utils/mount/nfs_mount.h
>>> @@ -50,21 +50,22 @@ struct nfs_mount_data {
>>> /* bits in the flags field */
>>> -#define NFS_MOUNT_SOFT		0x0001	/* 1 */
>>> -#define NFS_MOUNT_INTR		0x0002	/* 1 */
>>> -#define NFS_MOUNT_SECURE	0x0004	/* 1 */
>>> -#define NFS_MOUNT_POSIX		0x0008	/* 1 */
>>> -#define NFS_MOUNT_NOCTO		0x0010	/* 1 */
>>> -#define NFS_MOUNT_NOAC		0x0020	/* 1 */
>>> -#define NFS_MOUNT_TCP		0x0040	/* 2 */
>>> -#define NFS_MOUNT_VER3		0x0080	/* 3 */
>>> -#define NFS_MOUNT_KERBEROS	0x0100	/* 3 */
>>> -#define NFS_MOUNT_NONLM		0x0200	/* 3 */
>>> -#define NFS_MOUNT_BROKEN_SUID	0x0400	/* 4 */
>>> -#define NFS_MOUNT_NOACL     0x0800  /* 4 */
>>> -#define NFS_MOUNT_SECFLAVOUR	0x2000	/* 5 */
>>> -#define NFS_MOUNT_NORDIRPLUS	0x4000	/* 5 */
>>> -#define NFS_MOUNT_UNSHARED	0x8000	/* 5 */
>>> +#define NFS_MOUNT_SOFT		0x00001	/* 1 */
>>> +#define NFS_MOUNT_INTR		0x00002	/* 1 */
>>> +#define NFS_MOUNT_SECURE	0x00004	/* 1 */
>>> +#define NFS_MOUNT_POSIX		0x00008	/* 1 */
>>> +#define NFS_MOUNT_NOCTO		0x00010	/* 1 */
>>> +#define NFS_MOUNT_NOAC		0x00020	/* 1 */
>>> +#define NFS_MOUNT_TCP		0x00040	/* 2 */
>>> +#define NFS_MOUNT_VER3		0x00080	/* 3 */
>>> +#define NFS_MOUNT_KERBEROS	0x00100	/* 3 */
>>> +#define NFS_MOUNT_NONLM		0x00200	/* 3 */
>>> +#define NFS_MOUNT_BROKEN_SUID	0x00400	/* 4 */
>>> +#define NFS_MOUNT_NOACL		0x00800	/* 4 */
>>> +#define NFS_MOUNT_SECFLAVOUR	0x02000	/* 5 */
>>> +#define NFS_MOUNT_NORDIRPLUS	0x04000	/* 5 */
>>> +#define NFS_MOUNT_UNSHARED	0x08000	/* 5 */
>>> +#define NFS_MOUNT_NONEGDE	0x10000	/* 5 */
>>
>> Why is the patch replacing all the flag definitions?  Did you  
>> check  for inadvertent white space damage?
>
> Actually, I fixed one bit of erroneous whitespace I found (spaces  
> instead of tab).  The reason the definitions all change is because  
> a leading 0.  The existing flags were 0x0001 through 0x8000.  The  
> new flag is 0x10000.  I thought would be nice to make the flags all  
> 5 digits so that they visually lined up.

<EYEBALL CHECK> OK, I see it now.

> I don't have to do that; it does make the patch uglier, but IMHO I  
> think the resulting file is nicer.

Why not just expand these to 8 hex digits, and be done with it?

>> As I understand it, we aren't adding new options to the legacy  
>> part  of the nfs-utils mount command any longer.  Instead, add  
>> support for  the option in the kernel's NFS mount option parser in  
>> fs/nfs/super.c.
>
> Oh, perhaps I didn't fully investigate the context.  I was just  
> following the pattern I saw.  I can see if it still works without  
> that one snippet (I believe I already updated fs/nfs/super.c  
> appropriately).  I presume updating the man page is still  
> appropriate though, correct?

Yes; as long as you have the very latest version.  See Steve's nfs- 
utils git repo (I think git://linux-nfs.org/nfs-utils is the correct  
one).

> What about defining the flag?  If it's not going to be used, should  
> it still be defined as a placeholder?  I'll default to "yes" unless  
> I hear otherwise.


In the name of having utils/mount/nfs_mount.h match what's in include/ 
linux/nfs_mount.h, I guess it is reasonable to define the flag in  
both places.  IMO it would be nicer if nfs-utils just used the kernel  
header instead.

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

Re: [PATCH 0/3] Intro: Better handling coarse-grained timestamps

[Bob Bell]

On Tue, Jan 15, 2008 at 11:27:01AM -0500, Bob Bell wrote:
>Secondly, a modification to the kernel has been made to disable the 
>caching of negative dentries when the NFS_MOUNT_NONEGDE flag is set.  
>Note that this does nothing to address outdated information being 
>returned by readdir().  However, it does allow a file to be open()ed or 
>lstat()ed if the filename is known.  Because this has a potential 
>performance impact, it is left as an option for the user to select.

Someone pointed me to this page on NFSv4.1 directory delegations: 
http://wiki.linux-nfs.org/wiki/index.php/NFSv4.1_Directory_Delegations

I found the following quotes intriguing:

"Even though the directories along the path are cached, [without 
directory delegations] negative dentry caching is not allowed because it 
potentially violates close-to-open consistency semantics."

"Close-to-open consistency currently requires that even in a case where 
previous LOOKUPs or OPENs for a given file have recently and repeatedly 
failed, subsequent LOOKUPs and OPENs must nevertheless be sent to the 
server (i.e., negative caching provides no benefit in those cases)."

I admittedly know little about what is being discussed here.  However, 
is it possible that the Linux NFS client should *NEVER* cache negative 
entries?  (well, maybe with the exception of "nocto")

I'm interested if anyone has thoughts on the matter, though I know don't 
enough to argue either side, so here's a really easy chance to win an 
argument. :)

-- 
Bob Bell

Re: [PATCH 0/3] Intro: Better handling coarse-grained timestamps

[Trond Myklebust]

On Tue, 2008-01-15 at 20:55 -0500, Bob Bell wrote:
> On Tue, Jan 15, 2008 at 11:27:01AM -0500, Bob Bell wrote:
> >Secondly, a modification to the kernel has been made to disable the 
> >caching of negative dentries when the NFS_MOUNT_NONEGDE flag is set.  
> >Note that this does nothing to address outdated information being 
> >returned by readdir().  However, it does allow a file to be open()ed or 
> >lstat()ed if the filename is known.  Because this has a potential 
> >performance impact, it is left as an option for the user to select.
> 
> Someone pointed me to this page on NFSv4.1 directory delegations: 
> http://wiki.linux-nfs.org/wiki/index.php/NFSv4.1_Directory_Delegations
> 
> I found the following quotes intriguing:
> 
> "Even though the directories along the path are cached, [without 
> directory delegations] negative dentry caching is not allowed because it 
> potentially violates close-to-open consistency semantics."

Negative dentry caching has nothing whatsoever to do with close-to-open
semantics.

> "Close-to-open consistency currently requires that even in a case where 
> previous LOOKUPs or OPENs for a given file have recently and repeatedly 
> failed, subsequent LOOKUPs and OPENs must nevertheless be sent to the 
> server (i.e., negative caching provides no benefit in those cases)."

Nope. All close-to-open cache consistency requires is that the client
revalidate the file/directory cached data upon an open()/opendir() call
by checking the inode mtime and/or change attribute (NFSv4 only).

I think we need to revisit that wiki entry...

Trond

Re: [PATCH 0/3] Intro: Better handling coarse-grained timestamps

[david m. richter]

On Tue, 15 Jan 2008, Trond Myklebust wrote:
>
> On Tue, 2008-01-15 at 20:55 -0500, Bob Bell wrote:
> > On Tue, Jan 15, 2008 at 11:27:01AM -0500, Bob Bell wrote:
> > >Secondly, a modification to the kernel has been made to disable the 
> > >caching of negative dentries when the NFS_MOUNT_NONEGDE flag is set.  
> > >Note that this does nothing to address outdated information being 
> > >returned by readdir().  However, it does allow a file to be open()ed or 
> > >lstat()ed if the filename is known.  Because this has a potential 
> > >performance impact, it is left as an option for the user to select.
> > 
> > Someone pointed me to this page on NFSv4.1 directory delegations: 
> > http://wiki.linux-nfs.org/wiki/index.php/NFSv4.1_Directory_Delegations
> > 
> > I found the following quotes intriguing:
> > 
> > "Even though the directories along the path are cached, [without 
> > directory delegations] negative dentry caching is not allowed because it 
> > potentially violates close-to-open consistency semantics."
> 
> Negative dentry caching has nothing whatsoever to do with close-to-open
> semantics.
> 
> > "Close-to-open consistency currently requires that even in a case where 
> > previous LOOKUPs or OPENs for a given file have recently and repeatedly 
> > failed, subsequent LOOKUPs and OPENs must nevertheless be sent to the 
> > server (i.e., negative caching provides no benefit in those cases)."
> 
> Nope. All close-to-open cache consistency requires is that the client
> revalidate the file/directory cached data upon an open()/opendir() call
> by checking the inode mtime and/or change attribute (NFSv4 only).
> 
> I think we need to revisit that wiki entry...

	so then, would it instead be correct to say that the negative 
dentry caching on the client afforded by the delegation is beneficial 
insofar as it obviates the client's need to revalidate the file/directory 
in question?  if so, i understand the flawed CTO wording and will fix the 
wiki.

	thanks,

	d
	.
> 
> Trond

Re: [PATCH 0/3] Intro: Better handling coarse-grained timestamps

[Trond Myklebust]

On Wed, 2008-01-16 at 12:16 -0500, david m. richter wrote:
> 	so then, would it instead be correct to say that the negative 
> dentry caching on the client afforded by the delegation is beneficial 
> insofar as it obviates the client's need to revalidate the file/directory 
> in question?  if so, i understand the flawed CTO wording and will fix the 
> wiki.

Delegations give you a guarantee that the directory contents (i.e. the
readdir() information) have not changed, and so the client no longer
needs to poll the directory for change information.

IOW: specifically they allow the client to optimise away the GETATTR
call in opendir(), and they allow it to optimise away most of
nfs_lookup_revalidate().

Cheers
  Trond

Re: [PATCH 0/3] Intro: Better handling coarse-grained timestamps

[david m. richter]

On Wed, 16 Jan 2008, Trond Myklebust wrote:
> 
> On Wed, 2008-01-16 at 12:16 -0500, david m. richter wrote:
> > 	so then, would it instead be correct to say that the negative 
> > dentry caching on the client afforded by the delegation is beneficial 
> > insofar as it obviates the client's need to revalidate the file/directory 
> > in question?  if so, i understand the flawed CTO wording and will fix the 
> > wiki.
> 
> Delegations give you a guarantee that the directory contents (i.e. the
> readdir() information) have not changed, and so the client no longer
> needs to poll the directory for change information.
> 
> IOW: specifically they allow the client to optimise away the GETATTR
> call in opendir(), and they allow it to optimise away most of
> nfs_lookup_revalidate().
> 
> Cheers
>   Trond

	good, i'm on the same page as you here.  thanks, trond,

	d
	.