cifs: Allow directIO read/write during cache=strict

PATCH Applied Namjae Jeon 4 participants 8 messages Aug 20, 2014 – Aug 21, 2014 5 messages not in the archive

Subsystems (2)

SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

Namjae Jeon

author

2014-08-20 10:39 UTC #0 lore raw

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

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 <[email protected]>

Signed-off-by Ashish Sangwan <[email protected]>

---
 fs/cifs/dir.c  |    4 ++++
 fs/cifs/file.c |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

fs/cifs/dir.c +4 -0

index 3db0c5f..30e377c 100644
@@ -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)

fs/cifs/file.c +4 -0

index bee733e..0d07740 100644
@@ -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

Signature · 1 line

1.7.7

Steve French

2014-08-21 04:52 UTC #1 lore raw

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

Quoted 45 lines · Namjae Jeon

Signed-off-by Namjae Jeon <[email protected]>

Signed-off-by Ashish Sangwan <[email protected]>

---
 fs/cifs/dir.c  |    4 ++++
 fs/cifs/file.c |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

fs/cifs/dir.c +4 -0

index 3db0c5f..30e377c 100644
@@ -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)

fs/cifs/file.c +4 -1

index bee733e..0d07740 100644
@@ -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

Signature · 3 lines

Thanks,

Steve

A message that is not in the archive (sent off-list or not yet received): CAH2r5mvn86=76_O7QT0WMftjnr3Ek4t5PiqcWCy1HpaKVO=GFg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Namjae Jeon

author

2014-08-21 10:07 UTC #3 lore raw

Quoted 3 lines

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.

Quoted 54 lines · Namjae Jeon

Quoted 43 lines

Signed-off-by Namjae Jeon <[email protected]>

Signed-off-by Ashish Sangwan <[email protected]>

---
 fs/cifs/dir.c  |    4 ++++
 fs/cifs/file.c |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)

fs/cifs/dir.c +4 -0

index 3db0c5f..30e377c 100644
@@ -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)

fs/cifs/file.c +4 -1

index bee733e..0d07740 100644
@@ -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
--

Quoted 2 lines

1.7.7

--
Thanks,

Steve

Jeff Layton

2014-08-21 11:26 UTC #4 lore raw

In reply to Namjae Jeon

Quoted 32 lines · On Wed, 20 Aug 2014 19:39:11 +0900 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.
Signed-off-by Namjae Jeon <[email protected]>
Signed-off-by Ashish Sangwan <[email protected]>
---
 fs/cifs/dir.c  |    4 ++++
 fs/cifs/file.c |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)
fs/cifs/dir.c +4 -0
index 3db0c5f..30e377c 100644
@@ -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)
fs/cifs/file.c +0 -0
index bee733e..0d07740 100644

fs/cifs/file.c int cifs_open(struct inode *inode, struct file *file) · line 476

Quoted 10 lines

fs/cifs/file.c +4 -0

@@ -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

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.

Signature · 1 line

Jeff Layton <[email protected]>

A message that is not in the archive (sent off-list or not yet received): 20140821072656.372e613c-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org

Steve French

2014-08-21 13:24 UTC #6 lore raw

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.

Quoted 52 lines · Jeff Layton

Quoted 32 lines · On Wed, 20 Aug 2014 19:39:11 +0900 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.
Signed-off-by Namjae Jeon <[email protected]>
Signed-off-by Ashish Sangwan <[email protected]>
---
 fs/cifs/dir.c  |    4 ++++
 fs/cifs/file.c |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)
fs/cifs/dir.c +4 -0
index 3db0c5f..30e377c 100644
@@ -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)
fs/cifs/file.c +0 -0
index bee733e..0d07740 100644
fs/cifs/file.c int cifs_open(struct inode *inode, struct file *file) · line 476
Quoted 10 lines
fs/cifs/file.c +4 -0
@@ -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
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 <[email protected]>

Signature · 3 lines

Thanks,

Steve

A message that is not in the archive (sent off-list or not yet received): CAH2r5mu6LH1YLqWhi4-1dgJH0BQ2CS5Lbym1mGP9EuvgtCP1Wg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Jeff Layton

2014-08-21 15:02 UTC #8 lore raw

Quoted 5 lines · On Thu, 21 Aug 2014 08:24:35 -0500 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?

Quoted 56 lines · Jeff Layton

Quoted 52 lines
Quoted 32 lines · On Wed, 20 Aug 2014 19:39:11 +0900 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.
Signed-off-by Namjae Jeon <[email protected]>
Signed-off-by Ashish Sangwan <[email protected]>
---
 fs/cifs/dir.c  |    4 ++++
 fs/cifs/file.c |    4 ++++
 2 files changed, 8 insertions(+), 0 deletions(-)
fs/cifs/dir.c +4 -0
index 3db0c5f..30e377c 100644
@@ -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)
fs/cifs/file.c +0 -0
index bee733e..0d07740 100644
fs/cifs/file.c int cifs_open(struct inode *inode, struct file *file) · line 476
Quoted 10 lines
fs/cifs/file.c +4 -0
@@ -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
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 <[email protected]>

Signature · 1 line

Jeff Layton <[email protected]>

A message that is not in the archive (sent off-list or not yet received): 20140821110254.4f2dcc95-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org

Steve French

2014-08-21 15:18 UTC #10 lore raw

Quoted 10 lines · Jeff Layton

Quoted 5 lines · On Thu, 21 Aug 2014 08:24:35 -0500 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?

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

Signature · 3 lines

Thanks,

Steve

A message that is not in the archive (sent off-list or not yet received): CAH2r5mvAhxXXzG-Rgt2BD4a8EYsdGRcfUeEZC=AF5XTNO6ZXeg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Jeff Layton

2014-08-21 15:56 UTC #12 lore raw

Quoted 10 lines · On Thu, 21 Aug 2014 10:18:37 -0500 Steve French…

Quoted 9 lines · Jeff Layton
Quoted 5 lines · On Thu, 21 Aug 2014 08:24:35 -0500 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.

Quoted 9 lines

Quoted 1 line
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.

Signature · 1 line

Jeff Layton <[email protected]>