2010/10/26 Jeff Layton <[email protected]>:
> On Tue, 26 Oct 2010 17:01:26 +0400
> Pavel Shilovsky <[email protected]> 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 <[email protected]>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
