Re: Checking page_count(page) in invalidate_complete_page

Chuck Lever
2006-10-03T19:21:42Z

Trond Myklebust wrote:
> 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_.

Don't you think admins need to know when the client has potentially
caused data corruption?

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

We could achieve that simply by setting the NFS_INO_INVALID_DATA flag in
nfs_file_direct_write() and/or nfs_direct_write_complete().