From mboxrd@z Thu Jan 1 00:00:00 1970 Subject: Re: Checking page_count(page) in invalidate_complete_page From: Trond Myklebust In-Reply-To: <4521C79A.6090102@oracle.com> References: <4518333E.2060101@oracle.com> <45185D7E.6070104@yahoo.com.au> <451862C5.1010900@oracle.com> <45186481.1090306@yahoo.com.au> <45186DC3.7000902@oracle.com> <451870C6.6050008@yahoo.com.au> <4518835D.3080702@oracle.com> <451886FB.50306@yahoo.com.au> <451BF7BC.1040807@oracle.com> <20060928093640.14ecb1b1.akpm@osdl.org> <20060928094023.e888d533.akpm@osdl.org> <451BFB84.5070903@oracle.com> <20060928100306.0b58f3c7.akpm@osdl.org> <451C01C8.7020104@oracle.com> <451C6AAC.1080203@yahoo.com.au> <451D8371.2070101@oracle.com> <1159562724.13651.39.camel@lappy> <451D89E7.7020307@oracle.com> <1159564637.13651.44.camel@lappy> <20060929144421.48f9f1bd.akpm@osdl.org> <451D94A7.9060905@oracle.com> <20060929152951.0b763f6a.akpm@osdl.org> <451F425F.8030609@oracle.com> <4520FFB6.3040801@RedHat.com> <1159795522.6143.7.camel@lade.trondhjem.org> <20061002095727.05cd052f.akpm@osdl.org> <4521460B.8000504@RedHat.com> <20061002112005.d02f84f7.akpm@osdl.o! rg> <45216233.5010602@RedHat.com> <4521C79A.6090102@oracle.com> Content-Type: text/plain Content-Transfer-Encoding: 7bit Date: Tue, 03 Oct 2006 00:18:37 -0400 Message-Id: <1159849117.5420.17.camel@lade.trondhjem.org> Mime-Version: 1.0 Sender: owner-linux-mm@kvack.org Return-Path: To: chuck.lever@oracle.com Cc: Steve Dickson , Andrew Morton , linux-mm@kvack.org List-ID: 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 > --- > > 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: email@kvack.org