[PATCH] CIFS: Use invalidate_inode_pages2 instead of invalidate_remote_inode

[PATCH] CIFS: Use invalidate_inode_pages2 instead of invalidate_remote_inode

[Pavel Shilovsky]

We should invalidate a page even if it is locked now by someone, because
we can leave an invalid data there and then think it's uptodate according
to an oplock level (exclusive or II).

Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/inode.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 8852470..1da1f26 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1691,12 +1691,15 @@ cifs_invalidate_mapping(struct inode *inode)
 
 	cifs_i->invalid_mapping = false;
 
-	/* write back any cached data */
-	if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
-		rc = filemap_write_and_wait(inode->i_mapping);
-		mapping_set_error(inode->i_mapping, rc);
+	if (inode->i_mapping) {
+		/* write back any cached data */
+		if (inode->i_mapping->nrpages != 0) {
+			rc = filemap_write_and_wait(inode->i_mapping);
+			mapping_set_error(inode->i_mapping, rc);
+		}
+		invalidate_inode_pages2(inode->i_mapping);
 	}
-	invalidate_remote_inode(inode);
+
 	cifs_fscache_reset_inode_cookie(inode);
 }
 
-- 
1.7.1

Re: [PATCH] CIFS: Use invalidate_inode_pages2 instead of invalidate_remote_inode

[Pavel Shilovsky]

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

2011/2/22 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> We should invalidate a page even if it is locked now by someone, because
> we can leave an invalid data there and then think it's uptodate according
> to an oplock level (exclusive or II).
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/inode.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 8852470..1da1f26 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1691,12 +1691,15 @@ cifs_invalidate_mapping(struct inode *inode)
>
>        cifs_i->invalid_mapping = false;
>
> -       /* write back any cached data */
> -       if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
> -               rc = filemap_write_and_wait(inode->i_mapping);
> -               mapping_set_error(inode->i_mapping, rc);
> +       if (inode->i_mapping) {
> +               /* write back any cached data */
> +               if (inode->i_mapping->nrpages != 0) {
> +                       rc = filemap_write_and_wait(inode->i_mapping);
> +                       mapping_set_error(inode->i_mapping, rc);
> +               }
> +               invalidate_inode_pages2(inode->i_mapping);
>        }
> -       invalidate_remote_inode(inode);
> +
>        cifs_fscache_reset_inode_cookie(inode);
>  }
>
> --
> 1.7.1
>
>

I attached python script-reproducer. All shares should be mount with
new strictcache mount option.

-- 
Best regards,
Pavel Shilovsky.

[-- Attachment #2: cache_validation.py --]
[-- Type: text/x-python, Size: 728 bytes --]

#!/usr/bin/env python
#
# We have to mount the same share to test, test1, test2 directories that locate in the directory we
# execute this script from.

from os import open, close, O_RDWR, O_CREAT, write, read, O_RDONLY, O_WRONLY, O_TRUNC, lseek, SEEK_END, SEEK_SET
import time

tries = 1

while True:
	print 'try #', tries
	f = open('test/_test4321_', O_RDWR | O_CREAT | O_TRUNC)
	close(f)

	f1 = open('test1/_test4321_', O_RDWR)
	write(f1, 'a')
	close(f1)

	f2 = open('test2/_test4321_', O_WRONLY)
	write(f2, 'x')
	close(f2)

	f3 = open('test1/_test4321_', O_RDONLY)
	res = read(f3, 1)
	print 'must be x:', res, '-',
	if res != 'x':
		print 'FAIL'
		time.sleep(1)
		exit()
	time.sleep(0.1)

	close(f3)
	print 'OK'
	tries += 1

Re: [PATCH] CIFS: Use invalidate_inode_pages2 instead of invalidate_remote_inode

[Pavel Shilovsky]

2011/2/22 Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>:
> We should invalidate a page even if it is locked now by someone, because
> we can leave an invalid data there and then think it's uptodate according
> to an oplock level (exclusive or II).
>
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/inode.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 8852470..1da1f26 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1691,12 +1691,15 @@ cifs_invalidate_mapping(struct inode *inode)
>
>        cifs_i->invalid_mapping = false;
>
> -       /* write back any cached data */
> -       if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
> -               rc = filemap_write_and_wait(inode->i_mapping);
> -               mapping_set_error(inode->i_mapping, rc);
> +       if (inode->i_mapping) {
> +               /* write back any cached data */
> +               if (inode->i_mapping->nrpages != 0) {
> +                       rc = filemap_write_and_wait(inode->i_mapping);
> +                       mapping_set_error(inode->i_mapping, rc);
> +               }
> +               invalidate_inode_pages2(inode->i_mapping);
>        }
> -       invalidate_remote_inode(inode);
> +
>        cifs_fscache_reset_inode_cookie(inode);
>  }
>
> --
> 1.7.1
>
>

After investigating some more time, I found out that at least the
description of this patch isn't valid. Let's drop this one.

The problem is that we use invalidate_remote_inode that don't think
about return value from invalidate_mapping_pages and return void. The
problem is that invalidate_mapping_pages return 0 instead of positive
value. I will continue investigation of this problem.

-- 
Best regards,
Pavel Shilovsky.