Re: Checking page_count(page) in invalidate_complete_page

Trond Myklebust
2006-10-03T19:10:01Z

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?