[PATCH] NFS: flush data when locking a file to ensure cache coherence for mmap.

[PATCH] NFS: flush data when locking a file to ensure cache coherence for mmap.

[NeilBrown]

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



When a byte range lock (or flock) is taken out on an NFS file, the
validity of the cached data is checked and the inode is marked
NFS_INODE_INVALID_DATA.  However the cached data isn't flushed from
the page cache.

This is sufficient for future read() requests or mmap() requests as
they call nfs_revalidate_mapping() which performs the flush if
necessary.

However an existing mapping is not affected.  Accessing data through
that mapping will continue to return old data even though the inode is
marked NFS_INODE_INVALID_DATA.

This can easily be confirmed using the 'nfs' tool in
  git://github.com/okirch/twopence-nfs.git
and running

   nfs coherence FILENAME
on one client, and
   nfs coherence -r FILENAME
on another client.

It appears that prior to Linux 2.6.0 this worked correctly.

However commit:

http://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=ca9268fe3ddd075714005adecd4afbd7f9ab87d0

removed the call to inode_invalidate_pages() from nfs_zap_caches().  I
haven't tested this code, but inspection suggests that prior to this
commit, file locking would invalidate all inode pages.

This patch adds a call to nfs_revalidate_mapping_protected() after a
successful SETLK so that invalid data is flushed.  With this patch the
above test passes.

Cc: Olaf Kirch <okir@suse.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 fs/nfs/file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index 717a8d6af52d..781cc6c9c60b 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -822,6 +822,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
 			__nfs_revalidate_inode(NFS_SERVER(inode), inode);
 		else
 			nfs_zap_caches(inode);
+		nfs_revalidate_mapping_protected(inode, filp->f_mapping);
 	}
 out:
 	return status;
-- 
2.8.3


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] NFS: flush data when locking a file to ensure cache coherence for mmap.

[NeilBrown]

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


PING .... any comments on this patch, or the problem it tries to
address?

Thanks,
NeilBrown



[-- Attachment #2.1: Type: text/plain, Size: 2016 bytes --]

On Fri, Jun 03 2016, NeilBrown wrote:

> When a byte range lock (or flock) is taken out on an NFS file, the
> validity of the cached data is checked and the inode is marked
> NFS_INODE_INVALID_DATA.  However the cached data isn't flushed from
> the page cache.
>
> This is sufficient for future read() requests or mmap() requests as
> they call nfs_revalidate_mapping() which performs the flush if
> necessary.
>
> However an existing mapping is not affected.  Accessing data through
> that mapping will continue to return old data even though the inode is
> marked NFS_INODE_INVALID_DATA.
>
> This can easily be confirmed using the 'nfs' tool in
>   git://github.com/okirch/twopence-nfs.git
> and running
>
>    nfs coherence FILENAME
> on one client, and
>    nfs coherence -r FILENAME
> on another client.
>
> It appears that prior to Linux 2.6.0 this worked correctly.
>
> However commit:
>
> http://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=ca9268fe3ddd075714005adecd4afbd7f9ab87d0
>
> removed the call to inode_invalidate_pages() from nfs_zap_caches().  I
> haven't tested this code, but inspection suggests that prior to this
> commit, file locking would invalidate all inode pages.
>
> This patch adds a call to nfs_revalidate_mapping_protected() after a
> successful SETLK so that invalid data is flushed.  With this patch the
> above test passes.
>
> Cc: Olaf Kirch <okir@suse.com>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  fs/nfs/file.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index 717a8d6af52d..781cc6c9c60b 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -822,6 +822,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
>  			__nfs_revalidate_inode(NFS_SERVER(inode), inode);
>  		else
>  			nfs_zap_caches(inode);
> +		nfs_revalidate_mapping_protected(inode, filp->f_mapping);
>  	}
>  out:
>  	return status;
> -- 
> 2.8.3

[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] NFS: flush data when locking a file to ensure cache coherence for mmap.

[NeilBrown]

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


PING again

Thanks,
NeilBrown

On Fri, Jul 22 2016, NeilBrown wrote:

> PING .... any comments on this patch, or the problem it tries to
> address?
>
> Thanks,
> NeilBrown
>
>
> On Fri, Jun 03 2016, NeilBrown wrote:
>
>> When a byte range lock (or flock) is taken out on an NFS file, the
>> validity of the cached data is checked and the inode is marked
>> NFS_INODE_INVALID_DATA.  However the cached data isn't flushed from
>> the page cache.
>>
>> This is sufficient for future read() requests or mmap() requests as
>> they call nfs_revalidate_mapping() which performs the flush if
>> necessary.
>>
>> However an existing mapping is not affected.  Accessing data through
>> that mapping will continue to return old data even though the inode is
>> marked NFS_INODE_INVALID_DATA.
>>
>> This can easily be confirmed using the 'nfs' tool in
>>   git://github.com/okirch/twopence-nfs.git
>> and running
>>
>>    nfs coherence FILENAME
>> on one client, and
>>    nfs coherence -r FILENAME
>> on another client.
>>
>> It appears that prior to Linux 2.6.0 this worked correctly.
>>
>> However commit:
>>
>> http://git.kernel.org/cgit/linux/kernel/git/history/history.git/commit/?id=ca9268fe3ddd075714005adecd4afbd7f9ab87d0
>>
>> removed the call to inode_invalidate_pages() from nfs_zap_caches().  I
>> haven't tested this code, but inspection suggests that prior to this
>> commit, file locking would invalidate all inode pages.
>>
>> This patch adds a call to nfs_revalidate_mapping_protected() after a
>> successful SETLK so that invalid data is flushed.  With this patch the
>> above test passes.
>>
>> Cc: Olaf Kirch <okir@suse.com>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  fs/nfs/file.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
>> index 717a8d6af52d..781cc6c9c60b 100644
>> --- a/fs/nfs/file.c
>> +++ b/fs/nfs/file.c
>> @@ -822,6 +822,7 @@ do_setlk(struct file *filp, int cmd, struct file_lock *fl, int is_local)
>>  			__nfs_revalidate_inode(NFS_SERVER(inode), inode);
>>  		else
>>  			nfs_zap_caches(inode);
>> +		nfs_revalidate_mapping_protected(inode, filp->f_mapping);
>>  	}
>>  out:
>>  	return status;
>> -- 
>> 2.8.3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]