Re: connectathon test failures

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Oct 09, 2008 at 01:36:13PM -0400, Trond Myklebust wrote:
> OK. Having thought a little bit more about this, I think we can just
> delete the wraparound protection in nfs_update_inode(). We already have
> similar protection in nfs_inode_attrs_need_update()...

I retested with this patch, and it passed.  Thanks!

--b.

> 
> Cheers
>   Trond
> --------------------------------------------------------------------------
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> Date: Thu, 9 Oct 2008 13:27:55 -0400
> NFS: Fix attribute updates
> 
> This fixes a regression seen when running the Connectathon testsuite
> against an ext3 filesystem. The reason was that the inode was constantly
> being marked as 'just updated' by the jiffy wraparound test.
> This again meant that newer GETATTR calls were failing to pass the
> nfs_inode_attrs_need_update() test unless the changes caused a ctime update
> on the server, since they were perceived as having been started before the
> latest inode update.
> 
> Given that nfs_inode_attrs_need_update() already checks for wraparound
> of nfsi->last_updated, we can drop the buggy "protection" in
> nfs_update_inode().
> 
> Also make a slight micro-optimisation of nfs_inode_attrs_need_update(): we
> are more often going to see time_after(fattr->time_start, nfsi->last_updated)
> be true, rather than seeing an update of ctime/size, so put that test
> first to ensure that we optimise away the ctime/size tests.
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/nfs/inode.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index e25009f..6554281 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -933,10 +933,10 @@ static int nfs_inode_attrs_need_update(const struct inode *inode, const struct n
>  {
>  	const struct nfs_inode *nfsi = NFS_I(inode);
>  
> -	return nfs_ctime_need_update(inode, fattr) ||
> -			nfs_size_need_update(inode, fattr) ||
> -			time_after(fattr->time_start, nfsi->last_updated) ||
> -			time_after(nfsi->last_updated, jiffies);
> +	return time_after(fattr->time_start, nfsi->last_updated) ||
> +		nfs_ctime_need_update(inode, fattr) ||
> +		nfs_size_need_update(inode, fattr) ||
> +		time_after(nfsi->last_updated, jiffies);
>  }
>  
>  static int nfs_refresh_inode_locked(struct inode *inode, struct nfs_fattr *fattr)
> @@ -1167,11 +1167,6 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>  				nfsi->attrtimeo = NFS_MAXATTRTIMEO(inode);
>  			nfsi->attrtimeo_timestamp = now;
>  		}
> -		/*
> -		 * Avoid jiffy wraparound issues with nfsi->last_updated
> -		 */
> -		if (!time_in_range(nfsi->last_updated, nfsi->read_cache_jiffies, now))
> -			nfsi->last_updated = nfsi->read_cache_jiffies;
>  	}
>  	invalid &= ~NFS_INO_INVALID_ATTR;
>  	/* Don't invalidate the data if we were to blame */
> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
> --
> 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