Re: Checking page_count(page) in invalidate_complete_page

Chuck Lever
2006-10-03T02:14:50Z

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

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.
>
> 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...

[ 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.

[-- Attachment #2: nfs-check-return-codes.diff --]
[-- Type: text/plain, Size: 5097 bytes --]

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));
+ }
+}
+
/* 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);

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));
+ }
+}
+
+/**
* 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 *);