On Tue, 9 Nov 2010 21:02:19 +0300
Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> 2010/11/9 Jeff Layton <jlayton@xxxxxxxxxxxxxxx>:
> > On Fri, 5 Nov 2010 11:29:32 +0300
> > Pavel Shilovsky <piastryyy@xxxxxxxxx> wrote:
> >
> >> On strict cache mode 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@xxxxxxxxx>
> >> ---
> >> fs/cifs/cifs_fs_sb.h | 1 +
> >> fs/cifs/file.c | 6 ++++++
> >> 2 files changed, 7 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifs_fs_sb.h b/fs/cifs/cifs_fs_sb.h
> >> index e9a393c..be7b159 100644
> >> --- a/fs/cifs/cifs_fs_sb.h
> >> +++ b/fs/cifs/cifs_fs_sb.h
> >> @@ -40,6 +40,7 @@
> >> #define CIFS_MOUNT_FSCACHE 0x8000 /* local caching enabled */
> >> #define CIFS_MOUNT_MF_SYMLINKS 0x10000 /* Minshall+French Symlinks enabled */
> >> #define CIFS_MOUNT_MULTIUSER 0x20000 /* multiuser mount */
> >> +#define CIFS_MOUNT_STRICT_IO 0x40000 /* strict cache mode */
> >>
> >> struct cifs_sb_info {
> >> struct rb_root tlink_tree;
> >> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> >> index 777e7f4..b36de2e 100644
> >> --- a/fs/cifs/file.c
> >> +++ b/fs/cifs/file.c
> >> @@ -264,7 +264,9 @@ 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(inode);
> >> + struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
> >> struct cifsLockInfo *li, *tmp;
> >> + bool last_open_file = false;
> >>
> >> spin_lock(&cifs_file_list_lock);
> >> if (--cifs_file->count > 0) {
> >> @@ -279,10 +281,14 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
> >> if (list_empty(&cifsi->openFileList)) {
> >> cFYI(1, "closing last open instance for inode %p",
> >> cifs_file->dentry->d_inode);
> >> + last_open_file = true;
> >> cifs_set_oplock_level(inode, 0);
> >> }
> >> spin_unlock(&cifs_file_list_lock);
> >>
> >> + if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO) && last_open_file)
> >> + invalidate_remote_inode(inode);
> >> +
> >
> > Now that I think about it...this looks racy. Suppose someone races in
> > after you set last_open_file, opens the file and does a write....
>
> Can you explain what bad happens in this case?
>
It looks like you'll be invalidating potentially up-to-date data.
Either way...I've been steadily trying to move the code to a more
well-defined codepath for invalidating dirty data. Zapping the cached
pages here is unnecessary. It would be better to just mark the inode as
having an invalid mapping and move on, unless you see some other reason
why we must zap all of the pages here.
> >
> > I think you'd be better served by setting the invalid_mapping flag on
> > the cifs_i instead and letting the normal revalidation codepath handle
> > this. The cache will then be invalidated the next time
> > cifs_revalidate_file/dentry is run.
> >
> >> if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
> >> int xid, rc;
> >>
> >
>
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
