Re: Checking page_count(page) in invalidate_complete_page

[Trond Myklebust <Trond.Myklebust@netapp.com>]

On Mon, 2006-10-02 at 22:14 -0400, Chuck Lever wrote:
> Steve Dickson wrote:
> > Andrew Morton wrote:
> >>
> >> This is our user's data we're talking about here.
> > Point...
> >
> >>
> >> If that printk comes out then we need to fix the kernel so that it no
> >> longer wants to print that printk.  We don't want to just hide it.

So what _is_ stopping us from fixing it right now? Are we missing an
audit of the possible errors? That can be arranged...

> > I'm concern about the printk popping when we are flushing the
> > readdir cache (i.e. stale data) and either flooding the console
> > to a ton a messages (basically bring a system to its knees for
> > no good reason) or scaring the hell out people by saying we have a
> > major problem when in reality we are just flushing stale data...
> > 
> > So I definitely agree the printk should be there and be on by default,
> > but I so think it would be a good idea to have way to turn it off
> > if need be...

Why? If we know there is a problem, then why wait to fix it?

> [ Sorry for the attachment... anyone know how to include a diff inline 
> with Thunderbird? ]
> 
> The attached patch is my suggestion for reporting the cache invalidation 
> failure from within the NFS client.  Please review and comment.  My 
> testing with this patch applied has not triggered a single message, but 
> I haven't tried any memory exhaustion scenarios.
> 
> I honestly doubt that we will see log floods.  The original problem that 
> was causing stale data to remain cached has been addressed.  The reclaim 
> race will almost certainly be rare.
> plain text document attachment (nfs-check-return-codes.diff)
> NFS: Add return code checks for page invalidation
> 
> Print a warning if the page invalidation functions don't behave as
> expected.  A BUG is probably overkill here since the client's internal data
> structures will remain consistent.
> 
> We're trying to catch cases where invaliding an inode's page cache races
> with vmscan or direct I/O, resulting in stale data remaining in the page
> cache.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfs/dir.c           |   34 +++++++++++++++++++++++++++++-----
>  fs/nfs/direct.c        |    2 +-
>  fs/nfs/inode.c         |   25 +++++++++++++++++++++++--
>  fs/nfs/iostat.h        |    1 +
>  include/linux/nfs_fs.h |    1 +
>  5 files changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 7432f1a..0bb1a42 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -156,6 +156,32 @@ typedef struct {
>  	int		error;
>  } nfs_readdir_descriptor_t;
>  
> +/*
> + * Trim off all pages past page zero.  This ensures consistent page
> + * alignment of cached data.
> + *
> + * NB: This assumes we have exclusive access to this mapping either
> + *     through inode->i_mutex or some other mechanism.
> + */
> +static void nfs_truncate_directory_cache(struct inode *inode)
> +{
> +	int result;
> +
> +	dfprintk(DIRCACHE, "NFS: %s: truncating directory (%s/%Ld)\n",
> +			__FUNCTION__, inode->i_sb->s_id,
> +			(long long)NFS_FILEID(inode));
> +
> +	result = invalidate_inode_pages2_range(inode->i_mapping,
> +							PAGE_CACHE_SIZE, -1);
> +	if (unlikely(result < 0)) {
> +		nfs_inc_stats(inode, NFSIOS_INVALIDATEFAILED);
> +		printk(KERN_ERR
> +			"NFS: error %d invalidating cache for dir (%s/%Ld)\n",
> +				result, inode->i_sb->s_id,
> +				(long long)NFS_FILEID(inode));

See gripe below.

> +	}
> +}
> +
>  /* Now we cache directories properly, by stuffing the dirent
>   * data directly in the page cache.
>   *
> @@ -199,12 +225,10 @@ int nfs_readdir_filler(nfs_readdir_descr
>  	spin_lock(&inode->i_lock);
>  	NFS_I(inode)->cache_validity |= NFS_INO_INVALID_ATIME;
>  	spin_unlock(&inode->i_lock);
> -	/* Ensure consistent page alignment of the data.
> -	 * Note: assumes we have exclusive access to this mapping either
> -	 *	 through inode->i_mutex or some other mechanism.
> -	 */
> +
>  	if (page->index == 0)
> -		invalidate_inode_pages2_range(inode->i_mapping, PAGE_CACHE_SIZE, -1);
> +		nfs_truncate_directory_cache(inode);
> +
>  	unlock_page(page);
>  	return 0;
>   error:
> diff --git a/fs/nfs/direct.c b/fs/nfs/direct.c
> index 377839b..fe69c39 100644
> --- a/fs/nfs/direct.c
> +++ b/fs/nfs/direct.c
> @@ -823,7 +823,7 @@ ssize_t nfs_file_direct_write(struct kio
>  	 *      occur before the writes complete.  Kind of racey.
>  	 */
>  	if (mapping->nrpages)
> -		invalidate_inode_pages2(mapping);
> +		nfs_invalidate_mapping(mapping->host, mapping);

This looks wrong. Why are we bumping the NFSIOS_DATAINVALIDATE counter
on a direct write? We're not registering a cache consistency problem
here.

>  
>  	if (retval > 0)
>  		iocb->ki_pos = pos + retval;
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index bc9376c..e1cf978 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -657,6 +657,27 @@ int nfs_revalidate_inode(struct nfs_serv
>  }
>  
>  /**
> + * nfs_invalidate_mapping - Invalidate the inode's page cache
> + * @inode - pointer to host inode
> + * @mapping - pointer to mapping
> + */
> +void nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping)
> +{
> +	int result;
> +
> +	nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE);
> +
> +	result = invalidate_inode_pages2(mapping);
> +	if (unlikely(result) < 0) {
> +		nfs_inc_stats(inode, NFSIOS_INVALIDATEFAILED);
> +		printk(KERN_ERR
> +			"NFS: error %d invalidating pages for inode (%s/%Ld)\n",
> +				result, inode->i_sb->s_id,
> +				(long long)NFS_FILEID(inode));

So what _are_ users expected to do about this? Sue us? Complain bitterly
to lkml, and then get told that the VM is broken?

IOW: What the hell is the point of these messages?

> +	}
> +}
> +
> +/**
>   * nfs_revalidate_mapping - Revalidate the pagecache
>   * @inode - pointer to host inode
>   * @mapping - pointer to mapping
> @@ -673,10 +694,10 @@ int nfs_revalidate_mapping(struct inode 
>  		ret = __nfs_revalidate_inode(NFS_SERVER(inode), inode);
>  
>  	if (nfsi->cache_validity & NFS_INO_INVALID_DATA) {
> -		nfs_inc_stats(inode, NFSIOS_DATAINVALIDATE);
>  		if (S_ISREG(inode->i_mode))
>  			nfs_sync_mapping(mapping);
> -		invalidate_inode_pages2(mapping);
> +
> +		nfs_invalidate_mapping(inode, mapping);
>  
>  		spin_lock(&inode->i_lock);
>  		nfsi->cache_validity &= ~NFS_INO_INVALID_DATA;
> diff --git a/fs/nfs/iostat.h b/fs/nfs/iostat.h
> index 6350ecb..df41150 100644
> --- a/fs/nfs/iostat.h
> +++ b/fs/nfs/iostat.h
> @@ -104,6 +104,7 @@ enum nfs_stat_eventcounters {
>  	NFSIOS_SHORTREAD,
>  	NFSIOS_SHORTWRITE,
>  	NFSIOS_DELAY,
> +	NFSIOS_INVALIDATEFAILED,
>  	__NFSIOS_COUNTSMAX,
>  };
>  
> diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
> index 98c9b9f..dc3cac3 100644
> --- a/include/linux/nfs_fs.h
> +++ b/include/linux/nfs_fs.h
> @@ -306,6 +306,7 @@ extern int nfs_attribute_timeout(struct 
>  extern int nfs_revalidate_inode(struct nfs_server *server, struct inode *inode);
>  extern int __nfs_revalidate_inode(struct nfs_server *, struct inode *);
>  extern int nfs_revalidate_mapping(struct inode *inode, struct address_space *mapping);
> +extern void nfs_invalidate_mapping(struct inode *inode, struct address_space *mapping);
>  extern int nfs_setattr(struct dentry *, struct iattr *);
>  extern void nfs_setattr_update_inode(struct inode *inode, struct iattr *attr);
>  extern void nfs_begin_attr_update(struct inode *);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>