nfs_refresh_verifier / nfs_renew_times confusion

nfs_refresh_verifier / nfs_renew_times confusion

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1323 bytes --]

Hi Trond-

I'm reviewing 2.6.23-rc6 + NFS_ALL to prepare for porting my BKL 
elimination patch series.  I'm confused by a couple of things I found in 
fs/nfs/dir.c.

1.

static int nfs_check_verifier(struct inode *dir, struct dentry *dentry)
{
         unsigned long verf;

         if (IS_ROOT(dentry))
                 return 1;
         verf = dentry->d_time;
         if (nfs_caches_unstable(dir)
                         || verf != NFS_I(dir)->cache_change_attribute)
                 return 0;
         return 1;
}

Why use the extra variable "verf" here?

2.

Three functions are defined:

static inline void nfs_set_verifier(struct dentry * dentry, unsigned 
long verf)
{
         dentry->d_time = verf;
}

static void nfs_refresh_verifier(struct dentry * dentry, unsigned long verf)
{
         nfs_set_verifier(dentry, verf);
}

static inline void nfs_renew_times(struct dentry * dentry)
{
         dentry->d_time = jiffies;
}

but later in the code I see this sequence several times:

         nfs_renew_times(dentry);
         nfs_refresh_verifier(dentry, verifier);

as well as this sequence:

         nfs_renew_times(dentry);
         nfs_set_verifier(dentry, nfs_save_change_attribute(dir));

In both cases, the second call overwrites the dentry's d_time field 
which was set to "jiffies" in the first call.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 290 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
url:http://oss.oracle.com/~cel
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: nfs_refresh_verifier / nfs_renew_times confusion

[Trond Myklebust]

On Wed, 2007-09-19 at 16:20 -0400, Chuck Lever wrote:
> Hi Trond-
> 
> I'm reviewing 2.6.23-rc6 + NFS_ALL to prepare for porting my BKL 
> elimination patch series.  I'm confused by a couple of things I found in 
> fs/nfs/dir.c.
> 
> 1.
> 
> static int nfs_check_verifier(struct inode *dir, struct dentry *dentry)
> {
>          unsigned long verf;
> 
>          if (IS_ROOT(dentry))
>                  return 1;
>          verf = dentry->d_time;
>          if (nfs_caches_unstable(dir)
>                          || verf != NFS_I(dir)->cache_change_attribute)
>                  return 0;
>          return 1;
> }
> 
> Why use the extra variable "verf" here?

Historical reasons: we used to cache the verifier in the
dentry->d_fsdata, which is a (void *). The new sillyrename code OTOH,
needs d_fsdata in order to cache the asynchronous unlink data, so the
verifier was moved into d_time.

> 2.
> 
> Three functions are defined:
> 
> static inline void nfs_set_verifier(struct dentry * dentry, unsigned 
> long verf)
> {
>          dentry->d_time = verf;
> }
> 
> static void nfs_refresh_verifier(struct dentry * dentry, unsigned long verf)
> {
>          nfs_set_verifier(dentry, verf);
> }
> 
> static inline void nfs_renew_times(struct dentry * dentry)
> {
>          dentry->d_time = jiffies;
> }
> 
> but later in the code I see this sequence several times:
> 
>          nfs_renew_times(dentry);
>          nfs_refresh_verifier(dentry, verifier);
> 
> as well as this sequence:
> 
>          nfs_renew_times(dentry);
>          nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
> 
> In both cases, the second call overwrites the dentry's d_time field 
> which was set to "jiffies" in the first call.

Time to clean all that up. The legacy nfs_renew_times() has no meaning
now that we use d_time to cache the verifier. Would you like to send me
a patch removing them?

Trond


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: nfs_refresh_verifier / nfs_renew_times confusion

[Chuck Lever]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

Trond Myklebust wrote:
> Time to clean all that up. The legacy nfs_renew_times() has no meaning
> now that we use d_time to cache the verifier. Would you like to send me
> a patch removing them?

I can -- there are some minor nits.

In nfs_dentry_iput() we have this:

static void nfs_dentry_iput(struct dentry *dentry, struct inode *inode)
{
         nfs_inode_return_delegation(inode);
         if (S_ISDIR(inode->i_mode))
                 /* drop any readdir cache as it could easily be old */
                 NFS_I(inode)->cache_validity |= NFS_INO_INVALID_DATA;

         if (dentry->d_flags & DCACHE_NFSFS_RENAMED) {
                 lock_kernel();
                 drop_nlink(inode);
                 nfs_complete_unlink(dentry, inode);
                 unlock_kernel();
         }
         /* When creating a negative dentry, we want to renew d_time */
         nfs_renew_times(dentry);
         ^^^^^^^^^^^^^^^^^^^^^^^^
         iput(inode);
}

This is the only instance of nfs_renew_times() by itself.  Seems like 
this might break the behavior of negative dentry caching.  Should this 
be removed entirely, or replaced somehow with nfs_refresh_verifier()?

It looks like nfs_set_verifier() and nfs_refresh_verifier() reduce to 
exactly the same logic, but we have this at the end of nfs_readdir_lookup:

         nfs_renew_times(dentry);
         nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
         return dentry;
out_renew:
         nfs_renew_times(dentry);
         nfs_refresh_verifier(dentry, nfs_save_change_attribute(dir));
         return dentry;
}

Both of which are exactly equivalent now.  I can address this as well.

[-- Attachment #2: chuck.lever.vcf --]
[-- Type: text/x-vcard, Size: 290 bytes --]

begin:vcard
fn:Chuck Lever
n:Lever;Chuck
org:Oracle Corporation;Corporate Architecture: Linux Projects Group
adr:;;1015 Granger Avenue;Ann Arbor;MI;48104;USA
title:Principal Member of Staff
tel;work:+1 248 614 5091
x-mozilla-html:FALSE
url:http://oss.oracle.com/~cel
version:2.1
end:vcard


[-- Attachment #3: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: nfs_refresh_verifier / nfs_renew_times confusion

[Trond Myklebust]

On Wed, 2007-09-19 at 16:48 -0400, Chuck Lever wrote:
> Trond Myklebust wrote:
> > Time to clean all that up. The legacy nfs_renew_times() has no meaning
> > now that we use d_time to cache the verifier. Would you like to send me
> > a patch removing them?
> 
> I can -- there are some minor nits.
> 
> In nfs_dentry_iput() we have this:
> 
> static void nfs_dentry_iput(struct dentry *dentry, struct inode *inode)
> {
>          nfs_inode_return_delegation(inode);
>          if (S_ISDIR(inode->i_mode))
>                  /* drop any readdir cache as it could easily be old */
>                  NFS_I(inode)->cache_validity |= NFS_INO_INVALID_DATA;
> 
>          if (dentry->d_flags & DCACHE_NFSFS_RENAMED) {
>                  lock_kernel();
>                  drop_nlink(inode);
>                  nfs_complete_unlink(dentry, inode);
>                  unlock_kernel();
>          }
>          /* When creating a negative dentry, we want to renew d_time */
>          nfs_renew_times(dentry);
>          ^^^^^^^^^^^^^^^^^^^^^^^^
>          iput(inode);
> }
> 
> This is the only instance of nfs_renew_times() by itself.  Seems like 
> this might break the behavior of negative dentry caching.  Should this 
> be removed entirely, or replaced somehow with nfs_refresh_verifier()?

Just remove it. Unfortunately I don't think that we can avoid
revalidating the resulting negative dentry since the UNLINK call is
asynchronous, and so the new verifier on the directory will only be
known a posteriori.

> It looks like nfs_set_verifier() and nfs_refresh_verifier() reduce to 
> exactly the same logic, but we have this at the end of nfs_readdir_lookup:
> 
>          nfs_renew_times(dentry);
>          nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
>          return dentry;
> out_renew:
>          nfs_renew_times(dentry);
>          nfs_refresh_verifier(dentry, nfs_save_change_attribute(dir));
>          return dentry;
> }
> 
> Both of which are exactly equivalent now.  I can address this as well.

Thanks.

Trond


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs