[PATCH 2/4] CIFS: Invalidate inode pages on the last close

[PATCH 2/4] CIFS: Invalidate inode pages on the last close

[Pavel Shilovsky]

When we close the last file handle of the inode we should invalidate it
to prevent data coherency problem when we open it again but it has been
modified by other clients.

Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/cifs/file.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index d7c212a..02a045e 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -150,8 +150,14 @@ client_can_cache:
 		pCifsInode->clientCanCacheAll = true;
 		pCifsInode->clientCanCacheRead = true;
 		cFYI(1, "Exclusive Oplock granted on inode %p", inode);
-	} else if ((oplock & 0xF) == OPLOCK_READ)
+	} else if ((oplock & 0xF) == OPLOCK_READ) {
+		pCifsInode->clientCanCacheAll = false;
 		pCifsInode->clientCanCacheRead = true;
+		cFYI(1, "Level II Oplock franted on inode %p", inode);
+	} else {
+		pCifsInode->clientCanCacheAll = false;
+		pCifsInode->clientCanCacheRead = false;
+	}

 	return rc;
 }
@@ -271,8 +277,10 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
  */
 void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 {
+	struct inode *inode = cifs_file->dentry->d_inode;
 	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
-	struct cifsInodeInfo *cifsi = CIFS_I(cifs_file->dentry->d_inode);
+	struct cifsInodeInfo *cifsi = CIFS_I(inode);
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
 	struct cifsLockInfo *li, *tmp;

 	spin_lock(&cifs_file_list_lock);
@@ -290,6 +298,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 			cifs_file->dentry->d_inode);
 		cifsi->clientCanCacheRead = false;
 		cifsi->clientCanCacheAll  = false;
+		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+			invalidate_remote_inode(inode);
 	}
 	spin_unlock(&cifs_file_list_lock);

-- 
1.6.3.3

Re: [PATCH 2/4] CIFS: Invalidate inode pages on the last close

[Jeff Layton]

On Tue, 26 Oct 2010 17:01:26 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> When we close the last file handle of the inode we should invalidate it
> to prevent data coherency problem when we open it again but it has been
> modified by other clients.
> 
> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  fs/cifs/file.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index d7c212a..02a045e 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -150,8 +150,14 @@ client_can_cache:
>  		pCifsInode->clientCanCacheAll = true;
>  		pCifsInode->clientCanCacheRead = true;
>  		cFYI(1, "Exclusive Oplock granted on inode %p", inode);
> -	} else if ((oplock & 0xF) == OPLOCK_READ)
> +	} else if ((oplock & 0xF) == OPLOCK_READ) {
> +		pCifsInode->clientCanCacheAll = false;
>  		pCifsInode->clientCanCacheRead = true;
> +		cFYI(1, "Level II Oplock franted on inode %p", inode);
> +	} else {
> +		pCifsInode->clientCanCacheAll = false;
> +		pCifsInode->clientCanCacheRead = false;
> +	}
> 

The oplock handling in cifs is rather ad-hoc. There's a lot of
cut-and-pasted code that sets that clientCanCache flags. The flags
themselves are also sort of hard to understand -- i.e. why is is "All"
and not "Write"?

This would be good opportunity to consolidate that code into helper
functions and just call them from the appropriate places.

Perhaps a helper function that takes an oplock value and sets the
clientCanCache flags appropriately? Or...maybe consider doing away with
the canCache flags and just store the oplock value in the inode and
have helper macros or something that read that value and tell you what
can be done with it.

   i.e.: CLIENT_CAN_CACHE_READ(inode), CLIENT_CAN_CACHE_WRITE(inode)



>  	return rc;
>  }
> @@ -271,8 +277,10 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>   */
>  void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  {
> +	struct inode *inode = cifs_file->dentry->d_inode;
>  	struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
> -	struct cifsInodeInfo *cifsi = CIFS_I(cifs_file->dentry->d_inode);
> +	struct cifsInodeInfo *cifsi = CIFS_I(inode);
> +	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>  	struct cifsLockInfo *li, *tmp;
> 
>  	spin_lock(&cifs_file_list_lock);
> @@ -290,6 +298,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>  			cifs_file->dentry->d_inode);
>  		cifsi->clientCanCacheRead = false;
>  		cifsi->clientCanCacheAll  = false;
> +		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
> +			invalidate_remote_inode(inode);
				^^^^^^^^
		I don't think it's safe to call this under a spinlock.
		It would probably be better instead to flag the inode
		as needing invalidation by setting the
		invalidate_mapping flag to true.

>  	}
>  	spin_unlock(&cifs_file_list_lock);
> 

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/4] CIFS: Invalidate inode pages on the last close

[Steve French]

On Tue, Oct 26, 2010 at 9:25 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 26 Oct 2010 17:01:26 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> When we close the last file handle of the inode we should invalidate it
>> to prevent data coherency problem when we open it again but it has been
>> modified by other clients.
>>
>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  fs/cifs/file.c |   14 ++++++++++++--
>>  1 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index d7c212a..02a045e 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -150,8 +150,14 @@ client_can_cache:
>>               pCifsInode->clientCanCacheAll = true;
>>               pCifsInode->clientCanCacheRead = true;
>>               cFYI(1, "Exclusive Oplock granted on inode %p", inode);
>> -     } else if ((oplock & 0xF) == OPLOCK_READ)
>> +     } else if ((oplock & 0xF) == OPLOCK_READ) {
>> +             pCifsInode->clientCanCacheAll = false;
>>               pCifsInode->clientCanCacheRead = true;
>> +             cFYI(1, "Level II Oplock franted on inode %p", inode);
>> +     } else {
>> +             pCifsInode->clientCanCacheAll = false;
>> +             pCifsInode->clientCanCacheRead = false;
>> +     }
>>
>
> The oplock handling in cifs is rather ad-hoc. There's a lot of
> cut-and-pasted code that sets that clientCanCache flags. The flags
> themselves are also sort of hard to understand -- i.e. why is is "All"
> and not "Write"?
>
> This would be good opportunity to consolidate that code into helper
> functions and just call them from the appropriate places.
>
> Perhaps a helper function that takes an oplock value and sets the
> clientCanCache flags appropriately? Or...maybe consider doing away with
> the canCache flags and just store the oplock value in the inode and
> have helper macros or something that read that value and tell you what
> can be done with it.
>
>   i.e.: CLIENT_CAN_CACHE_READ(inode), CLIENT_CAN_CACHE_WRITE(inode)

Remember that the reason we don't have a can_cache_write is that there
is no equivalent smb/cifs protocol concept as "can_cache_write"
independent of caching reads - there are basically three types:
1) batch oplocks (read/write and open/close caching) which we don't
use (although will in smb2)
2) read/write
3) read




-- 
Thanks,

Steve

Re: [PATCH 2/4] CIFS: Invalidate inode pages on the last close

[Jeff Layton]

On Tue, 26 Oct 2010 09:46:34 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Oct 26, 2010 at 9:25 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 26 Oct 2010 17:01:26 +0400
> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> When we close the last file handle of the inode we should invalidate it
> >> to prevent data coherency problem when we open it again but it has been
> >> modified by other clients.
> >>
> >> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  fs/cifs/file.c |   14 ++++++++++++--
> >>  1 files changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> >> index d7c212a..02a045e 100644
> >> --- a/fs/cifs/file.c
> >> +++ b/fs/cifs/file.c
> >> @@ -150,8 +150,14 @@ client_can_cache:
> >>               pCifsInode->clientCanCacheAll = true;
> >>               pCifsInode->clientCanCacheRead = true;
> >>               cFYI(1, "Exclusive Oplock granted on inode %p", inode);
> >> -     } else if ((oplock & 0xF) == OPLOCK_READ)
> >> +     } else if ((oplock & 0xF) == OPLOCK_READ) {
> >> +             pCifsInode->clientCanCacheAll = false;
> >>               pCifsInode->clientCanCacheRead = true;
> >> +             cFYI(1, "Level II Oplock franted on inode %p", inode);
> >> +     } else {
> >> +             pCifsInode->clientCanCacheAll = false;
> >> +             pCifsInode->clientCanCacheRead = false;
> >> +     }
> >>
> >
> > The oplock handling in cifs is rather ad-hoc. There's a lot of
> > cut-and-pasted code that sets that clientCanCache flags. The flags
> > themselves are also sort of hard to understand -- i.e. why is is "All"
> > and not "Write"?
> >
> > This would be good opportunity to consolidate that code into helper
> > functions and just call them from the appropriate places.
> >
> > Perhaps a helper function that takes an oplock value and sets the
> > clientCanCache flags appropriately? Or...maybe consider doing away with
> > the canCache flags and just store the oplock value in the inode and
> > have helper macros or something that read that value and tell you what
> > can be done with it.
> >
> >   i.e.: CLIENT_CAN_CACHE_READ(inode), CLIENT_CAN_CACHE_WRITE(inode)
> 
> Remember that the reason we don't have a can_cache_write is that there
> is no equivalent smb/cifs protocol concept as "can_cache_write"
> independent of caching reads - there are basically three types:
> 1) batch oplocks (read/write and open/close caching) which we don't
> use (although will in smb2)
> 2) read/write
> 3) read
> 

I understand that, but from a coding standpoint I don't care. All I
care about is: "What can I do with the cached data attached to this
inode right now?"

That's why I think it might be saner to get rid of the bools
and keep the oplock flag in the inode and have macros that tell me what
I can actually do with it.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/4] CIFS: Invalidate inode pages on the last close

[Steve French]

On Tue, Oct 26, 2010 at 9:52 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 26 Oct 2010 09:46:34 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Tue, Oct 26, 2010 at 9:25 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Tue, 26 Oct 2010 17:01:26 +0400
>> > Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> >
>> >> When we close the last file handle of the inode we should invalidate it
>> >> to prevent data coherency problem when we open it again but it has been
>> >> modified by other clients.
>> >>
>> >> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> ---
>> >>  fs/cifs/file.c |   14 ++++++++++++--
>> >>  1 files changed, 12 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> >> index d7c212a..02a045e 100644
>> >> --- a/fs/cifs/file.c
>> >> +++ b/fs/cifs/file.c
>> >> @@ -150,8 +150,14 @@ client_can_cache:
>> >>               pCifsInode->clientCanCacheAll = true;
>> >>               pCifsInode->clientCanCacheRead = true;
>> >>               cFYI(1, "Exclusive Oplock granted on inode %p", inode);
>> >> -     } else if ((oplock & 0xF) == OPLOCK_READ)
>> >> +     } else if ((oplock & 0xF) == OPLOCK_READ) {
>> >> +             pCifsInode->clientCanCacheAll = false;
>> >>               pCifsInode->clientCanCacheRead = true;
>> >> +             cFYI(1, "Level II Oplock franted on inode %p", inode);
>> >> +     } else {
>> >> +             pCifsInode->clientCanCacheAll = false;
>> >> +             pCifsInode->clientCanCacheRead = false;
>> >> +     }
>> >>
>> >
>> > The oplock handling in cifs is rather ad-hoc. There's a lot of
>> > cut-and-pasted code that sets that clientCanCache flags. The flags
>> > themselves are also sort of hard to understand -- i.e. why is is "All"
>> > and not "Write"?
>> >
>> > This would be good opportunity to consolidate that code into helper
>> > functions and just call them from the appropriate places.
>> >
>> > Perhaps a helper function that takes an oplock value and sets the
>> > clientCanCache flags appropriately? Or...maybe consider doing away with
>> > the canCache flags and just store the oplock value in the inode and
>> > have helper macros or something that read that value and tell you what
>> > can be done with it.
>> >
>> >   i.e.: CLIENT_CAN_CACHE_READ(inode), CLIENT_CAN_CACHE_WRITE(inode)
>>
>> Remember that the reason we don't have a can_cache_write is that there
>> is no equivalent smb/cifs protocol concept as "can_cache_write"
>> independent of caching reads - there are basically three types:
>> 1) batch oplocks (read/write and open/close caching) which we don't
>> use (although will in smb2)
>> 2) read/write
>> 3) read
>>
>
> I understand that, but from a coding standpoint I don't care. All I
> care about is: "What can I do with the cached data attached to this
> inode right now?"
>
> That's why I think it might be saner to get rid of the bools
> and keep the oplock flag in the inode and have macros that tell me what
> I can actually do with it.

Booleans are cheap, probably use less space (depending on compiler and arch).
Seems low priority to make that kind of change - lots of more
important functional enhancements, and it might grow the inode.

-- 
Thanks,

Steve

Re: [PATCH 2/4] CIFS: Invalidate inode pages on the last close

[Christoph Hellwig]

And please, no camel case for new struct members (or types or function
names for that matter)

Re: [PATCH 2/4] CIFS: Invalidate inode pages on the last close

[Pavel Shilovsky]

2010/10/26 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue, 26 Oct 2010 17:01:26 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> When we close the last file handle of the inode we should invalidate it
>> to prevent data coherency problem when we open it again but it has been
>> modified by other clients.
>>
>> Signed-off-by: Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  fs/cifs/file.c |   14 ++++++++++++--
>>  1 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
>> index d7c212a..02a045e 100644
>> --- a/fs/cifs/file.c
>> +++ b/fs/cifs/file.c
>> @@ -150,8 +150,14 @@ client_can_cache:
>>               pCifsInode->clientCanCacheAll = true;
>>               pCifsInode->clientCanCacheRead = true;
>>               cFYI(1, "Exclusive Oplock granted on inode %p", inode);
>> -     } else if ((oplock & 0xF) == OPLOCK_READ)
>> +     } else if ((oplock & 0xF) == OPLOCK_READ) {
>> +             pCifsInode->clientCanCacheAll = false;
>>               pCifsInode->clientCanCacheRead = true;
>> +             cFYI(1, "Level II Oplock franted on inode %p", inode);
>> +     } else {
>> +             pCifsInode->clientCanCacheAll = false;
>> +             pCifsInode->clientCanCacheRead = false;
>> +     }
>>
>
> The oplock handling in cifs is rather ad-hoc. There's a lot of
> cut-and-pasted code that sets that clientCanCache flags. The flags
> themselves are also sort of hard to understand -- i.e. why is is "All"
> and not "Write"?
>
> This would be good opportunity to consolidate that code into helper
> functions and just call them from the appropriate places.

Good idea. I will try to do it.

>
> Perhaps a helper function that takes an oplock value and sets the
> clientCanCache flags appropriately? Or...maybe consider doing away with
> the canCache flags and just store the oplock value in the inode and
> have helper macros or something that read that value and tell you what
> can be done with it.
>
>   i.e.: CLIENT_CAN_CACHE_READ(inode), CLIENT_CAN_CACHE_WRITE(inode)
>
>
>
>>       return rc;
>>  }
>> @@ -271,8 +277,10 @@ cifs_new_fileinfo(__u16 fileHandle, struct file *file,
>>   */
>>  void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>>  {
>> +     struct inode *inode = cifs_file->dentry->d_inode;
>>       struct cifsTconInfo *tcon = tlink_tcon(cifs_file->tlink);
>> -     struct cifsInodeInfo *cifsi = CIFS_I(cifs_file->dentry->d_inode);
>> +     struct cifsInodeInfo *cifsi = CIFS_I(inode);
>> +     struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
>>       struct cifsLockInfo *li, *tmp;
>>
>>       spin_lock(&cifs_file_list_lock);
>> @@ -290,6 +298,8 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
>>                       cifs_file->dentry->d_inode);
>>               cifsi->clientCanCacheRead = false;
>>               cifsi->clientCanCacheAll  = false;
>> +             if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
>> +                     invalidate_remote_inode(inode);
>                                ^^^^^^^^
>                I don't think it's safe to call this under a spinlock.
>                It would probably be better instead to flag the inode
>                as needing invalidation by setting the
>                invalidate_mapping flag to true.
>

At first I was going not to do it under a spinlock, but Wine
application suddently crashes in this case. So, I didn't understand
the reason of so strange behavior and leave it under the spinlock - it
works well.

>>       }
>>       spin_unlock(&cifs_file_list_lock);
>>

-- 
Best regards,
Pavel Shilovsky.