Re: Checking page_count(page) in invalidate_complete_page

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

On Tue, 2006-10-03 at 14:50 -0400, Chuck Lever wrote:

> >> 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.
> 
> We're looking for potential races between direct I/O and cache 
> invalidation, among others.  Is your concern that this may report false 
> positives?

No. I simply don't see what the use case is for this statistic. AFAICS
it is purely a debugging tool for _developers_. That would have
absolutely no place at all in /proc/self/mountstats, which is supposed
to provide useful statistics for _administrators_.

> I'm not sure this invalidation is useful in any event.  Direct writes 
> are treated like some other client has modified the file, so cached 
> pages will get invalidated eventually anyway.  Maybe we should just 
> remove this one?

That would break the principle that if one process modifies the file,
then all processes on the same client will immediately see those
changes.

> >> +	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?
> 
> Such a message will be reported to distributors or lkml, and we will be 
> able to collect data about the scenario where there is a problem.

i.e. we're throwing our hands up into the air, and saying "we don't
understand what is going on here"?

Sorry, but that is not an option. If we can't design a clearly defined
API for cache invalidation, with well defined errors, then we should all
go home and start learning Solaris programming now.

> Another option for customers is to run application-level data 
> consistency checks when this error is reported.

And then sue us?

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