| Message ID | 20230703120709.3610-1-bharathsm@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb: add missing create options for O_DIRECT and O_SYNC flags | expand |
Bharath SM <bharathsm.hsk@gmail.com> writes: > The CREATE_NO_BUFFER and CREATE_WRITE_THROUGH file create options > are correctly set in cifs_nt_open and cifs_reopen functions based > on O_DIRECT and O_SYNC flags. However flags are missing during the > file creation process in cifs_atomic_open, this was leading to > successful write operations with O_DIRECT even in case on un-aligned > size. Fixed by setting create options based on open file flags. > > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > --- > fs/smb/client/dir.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c > index 30b1e1bfd204..4178a7fb2ac2 100644 > --- a/fs/smb/client/dir.c > +++ b/fs/smb/client/dir.c > @@ -292,6 +292,12 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned > * ACLs > */ > > + if (oflags & O_SYNC) > + create_options |= CREATE_WRITE_THROUGH; > + > + if (oflags & O_DIRECT) > + create_options |= CREATE_NO_BUFFER; > + Looks good, thanks. I see that cifs_nt_open(), cifs_reopen_file() and cifs_do_create() use cifs_create_options(). I'd rather have those flags set in cifs_create_options() by passing a new @flags to it, so if we need to make any changes later, only cifs_create_options() will require it. What do you think?
On Mon, Jul 3, 2023 at 9:33 PM Paulo Alcantara <pc@manguebit.com> wrote: > > Bharath SM <bharathsm.hsk@gmail.com> writes: > > > The CREATE_NO_BUFFER and CREATE_WRITE_THROUGH file create options > > are correctly set in cifs_nt_open and cifs_reopen functions based > > on O_DIRECT and O_SYNC flags. However flags are missing during the > > file creation process in cifs_atomic_open, this was leading to > > successful write operations with O_DIRECT even in case on un-aligned > > size. Fixed by setting create options based on open file flags. > > > > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > > --- > > fs/smb/client/dir.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c > > index 30b1e1bfd204..4178a7fb2ac2 100644 > > --- a/fs/smb/client/dir.c > > +++ b/fs/smb/client/dir.c > > @@ -292,6 +292,12 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned > > * ACLs > > */ > > > > + if (oflags & O_SYNC) > > + create_options |= CREATE_WRITE_THROUGH; > > + > > + if (oflags & O_DIRECT) > > + create_options |= CREATE_NO_BUFFER; > > + > > Looks good, thanks. > > I see that cifs_nt_open(), cifs_reopen_file() and cifs_do_create() use > cifs_create_options(). > > I'd rather have those flags set in cifs_create_options() by passing a > new @flags to it, so if we need to make any changes later, only > cifs_create_options() will require it. What do you think? Thanks, I see there are around 34 places where we call cifs_create_options() and currently it's taking cifs_sb,create options as arguments. Should we make another helper function with name "create_options=cifs_parse_flags(flags)" then pass create_options into cifs_create_options() ? I think we can do this cleanup as the next patch. What do you think.?
Bharath SM <bharathsm.hsk@gmail.com> writes: > On Mon, Jul 3, 2023 at 9:33 PM Paulo Alcantara <pc@manguebit.com> wrote: >> >> Bharath SM <bharathsm.hsk@gmail.com> writes: >> >> > The CREATE_NO_BUFFER and CREATE_WRITE_THROUGH file create options >> > are correctly set in cifs_nt_open and cifs_reopen functions based >> > on O_DIRECT and O_SYNC flags. However flags are missing during the >> > file creation process in cifs_atomic_open, this was leading to >> > successful write operations with O_DIRECT even in case on un-aligned >> > size. Fixed by setting create options based on open file flags. >> > >> > Signed-off-by: Bharath SM <bharathsm@microsoft.com> >> > --- >> > fs/smb/client/dir.c | 6 ++++++ >> > 1 file changed, 6 insertions(+) >> > >> > diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c >> > index 30b1e1bfd204..4178a7fb2ac2 100644 >> > --- a/fs/smb/client/dir.c >> > +++ b/fs/smb/client/dir.c >> > @@ -292,6 +292,12 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned >> > * ACLs >> > */ >> > >> > + if (oflags & O_SYNC) >> > + create_options |= CREATE_WRITE_THROUGH; >> > + >> > + if (oflags & O_DIRECT) >> > + create_options |= CREATE_NO_BUFFER; >> > + >> >> Looks good, thanks. >> >> I see that cifs_nt_open(), cifs_reopen_file() and cifs_do_create() use >> cifs_create_options(). >> >> I'd rather have those flags set in cifs_create_options() by passing a >> new @flags to it, so if we need to make any changes later, only >> cifs_create_options() will require it. What do you think? > > I see there are around 34 places where we call cifs_create_options() > and currently it's taking cifs_sb,create options as arguments. Should > we make another helper function with name > "create_options=cifs_parse_flags(flags)" then pass create_options into > cifs_create_options() ? > I think we can do this cleanup as the next patch. What do you think.? Sounds good. Thanks.
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c index 30b1e1bfd204..4178a7fb2ac2 100644 --- a/fs/smb/client/dir.c +++ b/fs/smb/client/dir.c @@ -292,6 +292,12 @@ static int cifs_do_create(struct inode *inode, struct dentry *direntry, unsigned * ACLs */ + if (oflags & O_SYNC) + create_options |= CREATE_WRITE_THROUGH; + + if (oflags & O_DIRECT) + create_options |= CREATE_NO_BUFFER; + if (!server->ops->open) { rc = -ENOSYS; goto out;
The CREATE_NO_BUFFER and CREATE_WRITE_THROUGH file create options are correctly set in cifs_nt_open and cifs_reopen functions based on O_DIRECT and O_SYNC flags. However flags are missing during the file creation process in cifs_atomic_open, this was leading to successful write operations with O_DIRECT even in case on un-aligned size. Fixed by setting create options based on open file flags. Signed-off-by: Bharath SM <bharathsm@microsoft.com> --- fs/smb/client/dir.c | 6 ++++++ 1 file changed, 6 insertions(+)