[PATCH 2/7] cifs: Allow directIO read/write during cache=strict

Discussion:

Namjae Jeon

2014-08-20 10:39:11 UTC

Currently cifs have all or nothing approach for directIO operations.
cache=strict mode does not allow directIO while cache=none mode performs
all the operations as directIO even when user does not specify O_DIRECT
flag. This patch enables strict cache mode to honour directIO semantics.

Signed-off-by: Namjae Jeon <namjae.jeon-***@public.gmane.org>
Signed-off-by: Ashish Sangwan <a.sangwan-***@public.gmane.org>
---
fs/cifs/dir.c | 4 ++++
fs/cifs/file.c | 4 ++++
2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 3db0c5f..30e377c 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -497,6 +497,10 @@ cifs_atomic_open(struct inode *inode, struct dentry *direntry,
goto out;
}

+ if (file->f_flags & O_DIRECT &&
+ CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+ file->f_op = &cifs_file_direct_ops;
+
file_info = cifs_new_fileinfo(&fid, file, tlink, oplock);
if (file_info == NULL) {
if (server->ops->close)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index bee733e..0d07740 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -467,6 +467,10 @@ int cifs_open(struct inode *inode, struct file *file)
cifs_dbg(FYI, "inode = 0x%p file flags are 0x%x for %s\n",
inode, file->f_flags, full_path);

+ if (file->f_flags & O_DIRECT &&
+ cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+ file->f_op = &cifs_file_direct_ops;
+
if (server->oplocks)
oplock = REQ_OPLOCK;
else

--
1.7.7

Steve French

2014-08-21 04:52:50 UTC

Permalink

What if a file is opened twice - once with o_direct and once without?
what happens?

Post by Namjae Jeon
Currently cifs have all or nothing approach for directIO operations.
cache=strict mode does not allow directIO while cache=none mode performs
all the operations as directIO even when user does not specify O_DIRECT
flag. This patch enables strict cache mode to honour directIO semantics.
---
fs/cifs/dir.c | 4 ++++
fs/cifs/file.c | 4 ++++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 3db0c5f..30e377c 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -497,6 +497,10 @@ cifs_atomic_open(struct inode *inode, struct dentry *direntry,
goto out;
}
+ if (file->f_flags & O_DIRECT &&
+ CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+ file->f_op = &cifs_file_direct_ops;
+
file_info = cifs_new_fileinfo(&fid, file, tlink, oplock);
if (file_info == NULL) {
if (server->ops->close)
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index bee733e..0d07740 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -467,6 +467,10 @@ int cifs_open(struct inode *inode, struct file *file)
cifs_dbg(FYI, "inode = 0x%p file flags are 0x%x for %s\n",
inode, file->f_flags, full_path);
+ if (file->f_flags & O_DIRECT &&
+ cifs_sb->mnt_cifs_flags & CIFS_MOUNT_STRICT_IO)
+ file->f_op = &cifs_file_direct_ops;
+
if (server->oplocks)
oplock = REQ_OPLOCK;
else
--
1.7.7

--
Thanks,

Steve

Namjae Jeon

2014-08-21 10:07:31 UTC

Permalink

Post by Steve French
What if a file is opened twice - once with o_direct and once without?
what happens?

IMHO nothing bad happens.
Per file file->fop is initialized to inode->i_fop in do_dentry_open.
We are not changing inode->i_fop, so they still point to cached file operations.
The file which is open with O_DIRECT will get its fop changed to
cifs_file_direct_ops while the other file can still use cifs_file_strict_ops.
Also, while checking, I notice that additional check for CIFS_MOUNT_NO_BRL is
needed.
if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_BRL)
inode->i_fop = &cifs_file_direct_nobrl_ops;
else
inode->i_fop = &cifs_file_direct_ops;
If there is no objection, I will send v2 included above additional check.

Thanks.

Post by Steve French

--
Thanks,
Steve

Jeff Layton

2014-08-21 11:26:56 UTC

Permalink

On Wed, 20 Aug 2014 19:39:11 +0900

Looks fine for the most part. You should also avoid requesting an
oplock if you're going to open O_DIRECT since you won't be using it
anyway.

--
Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+***@public.gmane.org>

Steve French

2014-08-21 13:24:35 UTC

Permalink

If you are opening O_DIRECT why wouldn't you use oplock, ie still
request oplock, simply to minimize metadata traffic (you don't have to
send stat across the wire). The reads and writes aren't cached but
the inode metadata would be.

Post by Jeff Layton
On Wed, 20 Aug 2014 19:39:11 +0900

Looks fine for the most part. You should also avoid requesting an
oplock if you're going to open O_DIRECT since you won't be using it
anyway.
--

--
Thanks,

Steve

Jeff Layton

2014-08-21 15:02:54 UTC

Permalink

On Thu, 21 Aug 2014 08:24:35 -0500

Post by Steve French
If you are opening O_DIRECT why wouldn't you use oplock, ie still
request oplock, simply to minimize metadata traffic (you don't have to
send stat across the wire). The reads and writes aren't cached but
the inode metadata would be.

What updates the i_size if a DIO write extends the file?

Post by Steve French

Post by Jeff Layton
On Wed, 20 Aug 2014 19:39:11 +0900

Looks fine for the most part. You should also avoid requesting an
oplock if you're going to open O_DIRECT since you won't be using it
anyway.
--

--
Jeff Layton <jlayton-***@public.gmane.org>

Steve French

2014-08-21 15:18:37 UTC

Permalink

Post by Jeff Layton
On Thu, 21 Aug 2014 08:24:35 -0500

What updates the i_size if a DIO write extends the file?

Doesn't direct writes call cifs_user_writev which calls
cifs_iovec_write which calls cifs_write_from_iter which calls
cifs_uncached_writev_complete

so looks like i_size should get set

--
Thanks,

Steve

Jeff Layton

2014-08-21 15:56:08 UTC

Permalink

On Thu, 21 Aug 2014 10:18:37 -0500

Post by Steve French

Post by Jeff Layton
On Thu, 21 Aug 2014 08:24:35 -0500

What updates the i_size if a DIO write extends the file?

Doesn't direct writes call cifs_user_writev which calls
cifs_iovec_write which calls cifs_write_from_iter which calls
cifs_uncached_writev_complete
so looks like i_size should get set

Ok, fair enough then. I still think it's a little silly to request an
oplock that you won't really be using, but I suppose there's no reason
to specifically forbid it.

--
Jeff Layton <jlayton-***@public.gmane.org>