| Message ID | 003501cfbc62$fac9f4b0$f05dde10$@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
What if a file is opened twice - once with o_direct and once without? what happens? On Wed, Aug 20, 2014 at 5:39 AM, Namjae Jeon <namjae.jeon@samsung.com> wrote: > 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@samsung.com> > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com> > --- > 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 >
> > 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. > > On Wed, Aug 20, 2014 at 5:39 AM, Namjae Jeon <namjae.jeon@samsung.com> wrote: > > 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@samsung.com> > > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com> > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 20 Aug 2014 19:39:11 +0900 Namjae Jeon <namjae.jeon@samsung.com> wrote: > 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@samsung.com> > Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com> > --- > 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 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.
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. On Thu, Aug 21, 2014 at 6:26 AM, Jeff Layton <jlayton@poochiereds.net> wrote: > On Wed, 20 Aug 2014 19:39:11 +0900 > Namjae Jeon <namjae.jeon@samsung.com> wrote: > >> 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@samsung.com> >> Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com> >> --- >> 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 > > 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@poochiereds.net>
On Thu, 21 Aug 2014 08:24:35 -0500 Steve French <smfrench@gmail.com> wrote: > 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? > On Thu, Aug 21, 2014 at 6:26 AM, Jeff Layton <jlayton@poochiereds.net> wrote: > > On Wed, 20 Aug 2014 19:39:11 +0900 > > Namjae Jeon <namjae.jeon@samsung.com> wrote: > > > >> 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@samsung.com> > >> Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com> > >> --- > >> 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 > > > > 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@poochiereds.net> > > >
On Thu, Aug 21, 2014 at 10:02 AM, Jeff Layton <jlayton@samba.org> wrote: > On Thu, 21 Aug 2014 08:24:35 -0500 > Steve French <smfrench@gmail.com> wrote: > >> 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
On Thu, 21 Aug 2014 10:18:37 -0500 Steve French <smfrench@gmail.com> wrote: > On Thu, Aug 21, 2014 at 10:02 AM, Jeff Layton <jlayton@samba.org> wrote: > > On Thu, 21 Aug 2014 08:24:35 -0500 > > Steve French <smfrench@gmail.com> wrote: > > > >> 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 > > 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.
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