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

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

[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 <namjae.jeon-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Ashish Sangwan <a.sangwan-Sze3O3UU22JBDgjK7y7TUQ@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

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

[Steve French]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Ashish Sangwan <a.sangwan-Sze3O3UU22JBDgjK7y7TUQ@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
>



-- 
Thanks,

Steve

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

[Namjae Jeon]

> 
> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > Signed-off-by: Ashish Sangwan <a.sangwan-Sze3O3UU22JBDgjK7y7TUQ@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
> >
> 
> 
> 
> --
> Thanks,
> 
> Steve

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

[Jeff Layton]

On Wed, 20 Aug 2014 19:39:11 +0900
Namjae Jeon <namjae.jeon-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Ashish Sangwan <a.sangwan-Sze3O3UU22JBDgjK7y7TUQ@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

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+C8xQ@public.gmane.org>

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

[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.

On Thu, Aug 21, 2014 at 6:26 AM, Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> wrote:
> On Wed, 20 Aug 2014 19:39:11 +0900
> Namjae Jeon <namjae.jeon-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Ashish Sangwan <a.sangwan-Sze3O3UU22JBDgjK7y7TUQ@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
>
> 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+C8xQ@public.gmane.org>



-- 
Thanks,

Steve

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

[Jeff Layton]

On Thu, 21 Aug 2014 08:24:35 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org> wrote:
> > On Wed, 20 Aug 2014 19:39:11 +0900
> > Namjae Jeon <namjae.jeon-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >> Signed-off-by: Ashish Sangwan <a.sangwan-Sze3O3UU22JBDgjK7y7TUQ@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
> >
> > 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+C8xQ@public.gmane.org>
> 
> 
> 


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

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

[Steve French]

On Thu, Aug 21, 2014 at 10:02 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, 21 Aug 2014 08:24:35 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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


-- 
Thanks,

Steve

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

[Jeff Layton]

On Thu, 21 Aug 2014 10:18:37 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, Aug 21, 2014 at 10:02 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Thu, 21 Aug 2014 08:24:35 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 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.

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