[PATCH v2 0/4] cifs: cache invalidation fixes when cache=loose

[Jeff Layton]

This is the second posting of this patchset. The end result of this set
should be indentical to the v1 set, but this fixes some problems in the
intermediate changes that would have introduced bugs for someone doing
a bisect.

This patchset fixes up some cache invalidation problems that can occur
when cache=loose. The main point is to ensure that we revalidate the
cache prior to satisfying a read.

At the same time, I added a fix for a potential problem that cropped up
with NFS a while back, and to which CIFS is also vulnerable.

This was originally reported by Tetsuo Handa, and as far as I can tell
this fixes his testcase, assuming that you also ensure that kernel
oplocks are enabled.

Given that we're late in the cycle, I'd like to see these merged for
v3.16 but getting them into -next soon would be good.

Jeff Layton (4):
cifs: convert booleans in cifsInodeInfo to a flags field
cifs: new helper function: cifs_revalidate_mapping
cifs: fix potential races in cifs_revalidate_mapping
cifs: revalidate mapping prior to satisfying aio_read request

fs/cifs/cifsfs.c | 24 ++++++++++++------
fs/cifs/cifsfs.h | 2 ++
fs/cifs/cifsglob.h | 7 +++---
fs/cifs/file.c | 16 ++++++------
fs/cifs/inode.c | 71 ++++++++++++++++++++++++++++++++++++++++++------------
5 files changed, 86 insertions(+), 34 deletions(-)

[Jeff Layton]

This post might be inappropriate. Click to display it.

[Jeff Layton]

In later patches, we'll need to have a bitlock, so go ahead and convert
these bools to use atomic bitops instead.

Also, clean up the initialization of the flags field. There's no need
to unset each bit individually just after it was zeroed on allocation.

Signed-off-by: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+***@public.gmane.org>
---
fs/cifs/cifsfs.c | 6 +-----
fs/cifs/cifsglob.h | 6 +++---
fs/cifs/file.c | 4 ++--
fs/cifs/inode.c | 19 +++++++++++--------
4 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 5be1f997ecde..3ef0299ed43e 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -251,11 +251,7 @@ cifs_alloc_inode(struct super_block *sb)
* server, can not assume caching of file data or metadata.
*/
cifs_set_oplock_level(cifs_inode, 0);
- cifs_inode->delete_pending = false;
- cifs_inode->invalid_mapping = false;
- clear_bit(CIFS_INODE_PENDING_OPLOCK_BREAK, &cifs_inode->flags);
- clear_bit(CIFS_INODE_PENDING_WRITERS, &cifs_inode->flags);
- clear_bit(CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2, &cifs_inode->flags);
+ cifs_inode->flags = 0;
spin_lock_init(&cifs_inode->writers_lock);
cifs_inode->writers = 0;
cifs_inode->vfs_inode.i_blkbits = 14; /* 2**14 = CIFS_MAX_MSGSIZE */
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 30f6e9251a4a..69da55b750e7 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1113,12 +1113,12 @@ struct cifsInodeInfo {
__u32 cifsAttrs; /* e.g. DOS archive bit, sparse, compressed, system */
unsigned int oplock; /* oplock/lease level we have */
unsigned int epoch; /* used to track lease state changes */
- bool delete_pending; /* DELETE_ON_CLOSE is set */
- bool invalid_mapping; /* pagecache is invalid */
- unsigned long flags;
#define CIFS_INODE_PENDING_OPLOCK_BREAK (0) /* oplock break in progress */
#define CIFS_INODE_PENDING_WRITERS (1) /* Writes in progress */
#define CIFS_INODE_DOWNGRADE_OPLOCK_TO_L2 (2) /* Downgrade oplock to L2 */
+#define CIFS_INO_DELETE_PENDING (3) /* delete pending on server */
+#define CIFS_INO_INVALID_MAPPING (4) /* pagecache is invalid */
+ unsigned long flags;
spinlock_t writers_lock;
unsigned int writers; /* Number of writers on this inode */
unsigned long time; /* jiffies of last update of inode */
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 5ed03e0b8b40..c8a570e87900 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -392,7 +392,7 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
* again and get at least level II oplock.
*/
if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
- CIFS_I(inode)->invalid_mapping = true;
+ set_bit(CIFS_INO_INVALID_MAPPING, &cifsi->flags);
cifs_set_oplock_level(cifsi, 0);
}
spin_unlock(&cifs_file_list_lock);
@@ -2562,7 +2562,7 @@ ssize_t cifs_user_writev(struct kiocb *iocb, const struct iovec *iov,

written = cifs_iovec_write(iocb->ki_filp, iov, nr_segs, &pos);
if (written > 0) {
- CIFS_I(inode)->invalid_mapping = true;
+ set_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags);
iocb->ki_pos = pos;
}

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a22d667f1069..fa9ef8d902b5 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -117,7 +117,7 @@ cifs_revalidate_cache(struct inode *inode, struct cifs_fattr *fattr)

cifs_dbg(FYI, "%s: invalidating inode %llu mapping\n",
__func__, cifs_i->uniqueid);
- cifs_i->invalid_mapping = true;
+ set_bit(CIFS_INO_INVALID_MAPPING, &cifs_i->flags);
}

/*
@@ -177,7 +177,10 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr)
else
cifs_i->time = jiffies;

- cifs_i->delete_pending = fattr->cf_flags & CIFS_FATTR_DELETE_PENDING;
+ if (fattr->cf_flags & CIFS_FATTR_DELETE_PENDING)
+ set_bit(CIFS_INO_DELETE_PENDING, &cifs_i->flags);
+ else
+ clear_bit(CIFS_INO_DELETE_PENDING, &cifs_i->flags);

cifs_i->server_eof = fattr->cf_eof;
/*
@@ -1121,7 +1124,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
}

/* try to set DELETE_ON_CLOSE */
- if (!cifsInode->delete_pending) {
+ if (!test_bit(CIFS_INO_DELETE_PENDING, &cifsInode->flags)) {
rc = CIFSSMBSetFileDisposition(xid, tcon, true, fid.netfid,
current->tgid);
/*
@@ -1138,7 +1141,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
rc = -EBUSY;
goto undo_rename;
}
- cifsInode->delete_pending = true;
+ set_bit(CIFS_INO_DELETE_PENDING, &cifsInode->flags);
}

out_close:
@@ -1761,14 +1764,14 @@ cifs_invalidate_mapping(struct inode *inode)
int rc = 0;
struct cifsInodeInfo *cifs_i = CIFS_I(inode);

- cifs_i->invalid_mapping = false;
+ clear_bit(CIFS_INO_INVALID_MAPPING, &cifs_i->flags);

if (inode->i_mapping && inode->i_mapping->nrpages != 0) {
rc = invalidate_inode_pages2(inode->i_mapping);
if (rc) {
cifs_dbg(VFS, "%s: could not invalidate inode %p\n",
__func__, inode);
- cifs_i->invalid_mapping = true;
+ set_bit(CIFS_INO_INVALID_MAPPING, &cifs_i->flags);
}
}

@@ -1842,7 +1845,7 @@ int cifs_revalidate_file(struct file *filp)
if (rc)
return rc;

- if (CIFS_I(inode)->invalid_mapping)
+ if (test_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags))
rc = cifs_invalidate_mapping(inode);
return rc;
}
@@ -1857,7 +1860,7 @@ int cifs_revalidate_dentry(struct dentry *dentry)
if (rc)
return rc;

- if (CIFS_I(inode)->invalid_mapping)
+ if (test_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags))
rc = cifs_invalidate_mapping(inode);
return rc;
}

[Jeff Layton]

Consolidate a bit of code. In a later patch we'll expand this to fix
some races.

Signed-off-by: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+***@public.gmane.org>
---
fs/cifs/cifsfs.h | 1 +
fs/cifs/inode.c | 16 ++++++++++------
2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifsfs.h b/fs/cifs/cifsfs.h
index 26a754f49ba1..40d5a4df1e38 100644
--- a/fs/cifs/cifsfs.h
+++ b/fs/cifs/cifsfs.h
@@ -67,6 +67,7 @@ extern int cifs_revalidate_dentry_attr(struct dentry *);
extern int cifs_revalidate_file(struct file *filp);
extern int cifs_revalidate_dentry(struct dentry *);
extern int cifs_invalidate_mapping(struct inode *inode);
+extern int cifs_revalidate_mapping(struct inode *inode);
extern int cifs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
extern int cifs_setattr(struct dentry *, struct iattr *);

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index fa9ef8d902b5..ff420b275777 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1779,6 +1779,14 @@ cifs_invalidate_mapping(struct inode *inode)
return rc;
}

+int
+cifs_revalidate_mapping(struct inode *inode)
+{
+ if (test_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags))
+ return cifs_invalidate_mapping(inode);
+ return 0;
+}
+
int cifs_revalidate_file_attr(struct file *filp)
{
int rc = 0;
@@ -1845,9 +1853,7 @@ int cifs_revalidate_file(struct file *filp)
if (rc)
return rc;

- if (test_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags))
- rc = cifs_invalidate_mapping(inode);
- return rc;
+ return cifs_revalidate_mapping(inode);
}

/* revalidate a dentry's inode attributes */
@@ -1860,9 +1866,7 @@ int cifs_revalidate_dentry(struct dentry *dentry)
if (rc)
return rc;

- if (test_bit(CIFS_INO_INVALID_MAPPING, &CIFS_I(inode)->flags))
- rc = cifs_invalidate_mapping(inode);
- return rc;
+ return cifs_revalidate_mapping(inode);
}

int cifs_getattr(struct vfsmount *mnt, struct dentry *dentry,

[Jeff Layton]

Before satisfying a read with cache=loose, we should always check
that the pagecache is valid before allowing a read to be satisfied
out of it.

Reported-by: Tetsuo Handa <penguin-kernel-JPay3/***@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+***@public.gmane.org>
---
fs/cifs/cifsfs.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 3ef0299ed43e..6d3626ba4464 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -729,6 +729,20 @@ out_nls:
goto out;
}

+static ssize_t
+cifs_loose_aio_read(struct kiocb *iocb, const struct iovec *iov,
+ unsigned long nr_segs, loff_t pos)
+{
+ int rc;
+ struct inode *inode = file_inode(iocb->ki_filp);
+
+ rc = cifs_revalidate_mapping(inode);
+ if (rc)
+ return rc;
+
+ return generic_file_aio_read(iocb, iov, nr_segs, pos);
+}
+
static ssize_t cifs_file_aio_write(struct kiocb *iocb, const struct iovec *iov,
unsigned long nr_segs, loff_t pos)
{
@@ -886,7 +900,7 @@ const struct inode_operations cifs_symlink_inode_ops = {
const struct file_operations cifs_file_ops = {
.read = do_sync_read,
.write = do_sync_write,
- .aio_read = generic_file_aio_read,
+ .aio_read = cifs_loose_aio_read,
.aio_write = cifs_file_aio_write,
.open = cifs_open,
.release = cifs_close,
@@ -944,7 +958,7 @@ const struct file_operations cifs_file_direct_ops = {
const struct file_operations cifs_file_nobrl_ops = {
.read = do_sync_read,
.write = do_sync_write,
- .aio_read = generic_file_aio_read,
+ .aio_read = cifs_loose_aio_read,
.aio_write = cifs_file_aio_write,
.open = cifs_open,
.release = cifs_close,

[Steve French]

series merged into cifs-2.6.git for-next

Post by Jeff Layton
This is the second posting of this patchset. The end result of this set
should be indentical to the v1 set, but this fixes some problems in the
intermediate changes that would have introduced bugs for someone doing
a bisect.
This patchset fixes up some cache invalidation problems that can occur
when cache=loose. The main point is to ensure that we revalidate the
cache prior to satisfying a read.
At the same time, I added a fix for a potential problem that cropped up
with NFS a while back, and to which CIFS is also vulnerable.
This was originally reported by Tetsuo Handa, and as far as I can tell
this fixes his testcase, assuming that you also ensure that kernel
oplocks are enabled.
Given that we're late in the cycle, I'd like to see these merged for
v3.16 but getting them into -next soon would be good.
cifs: convert booleans in cifsInodeInfo to a flags field
cifs: new helper function: cifs_revalidate_mapping
cifs: fix potential races in cifs_revalidate_mapping
cifs: revalidate mapping prior to satisfying aio_read request
fs/cifs/cifsfs.c | 24 ++++++++++++------
fs/cifs/cifsfs.h | 2 ++
fs/cifs/cifsglob.h | 7 +++---
fs/cifs/file.c | 16 ++++++------
fs/cifs/inode.c | 71 ++++++++++++++++++++++++++++++++++++++++++------------
5 files changed, 86 insertions(+), 34 deletions(-)
--
1.9.0