Re: Checking page_count(page) in invalidate_complete_page

Trond Myklebust
2006-10-03T04:18:37Z

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 *);