Message ID | 20241216183148.4291-1-bharathsm@microsoft.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | smb: enable reuse of deferred file handles for write operations | expand |
merged into cifs-2.6.git for-next pending review and more testing On Mon, Dec 16, 2024 at 12:36 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: > > Previously, deferred file handles were reused only for read > operations, this commit extends to reusing deferred handles > for write operations. > > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > --- > fs/smb/client/file.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c > index a58a3333ecc3..98deff1de74c 100644 > --- a/fs/smb/client/file.c > +++ b/fs/smb/client/file.c > @@ -990,7 +990,11 @@ int cifs_open(struct inode *inode, struct file *file) > } > > /* Get the cached handle as SMB2 close is deferred */ > - rc = cifs_get_readable_path(tcon, full_path, &cfile); > + if (OPEN_FMODE(file->f_flags) & FMODE_WRITE) { > + rc = cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile); > + } else { > + rc = cifs_get_readable_path(tcon, full_path, &cfile); > + } > if (rc == 0) { > if (file->f_flags == cfile->f_flags) { > file->private_data = cfile; > -- > 2.43.0 > >
On Tue, Dec 17, 2024 at 2:22 AM Steve French <smfrench@gmail.com> wrote: > > merged into cifs-2.6.git for-next pending review and more testing > > On Mon, Dec 16, 2024 at 12:36 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: > > > > Previously, deferred file handles were reused only for read > > operations, this commit extends to reusing deferred handles > > for write operations. > > > > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > > --- > > fs/smb/client/file.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c > > index a58a3333ecc3..98deff1de74c 100644 > > --- a/fs/smb/client/file.c > > +++ b/fs/smb/client/file.c > > @@ -990,7 +990,11 @@ int cifs_open(struct inode *inode, struct file *file) > > } > > > > /* Get the cached handle as SMB2 close is deferred */ > > - rc = cifs_get_readable_path(tcon, full_path, &cfile); > > + if (OPEN_FMODE(file->f_flags) & FMODE_WRITE) { > > + rc = cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile); Wondering if FIND_WR_ANY is okay for all use cases? Specifically, I'm checking where FIND_WR_FSUID_ONLY is relevant. @Steve French Is this for multiuser mounts? I don't think so, since multiuser mounts come with their own tcon, and we search writable files in our tcon's open list. > > + } else { > > + rc = cifs_get_readable_path(tcon, full_path, &cfile); > > + } > > if (rc == 0) { > > if (file->f_flags == cfile->f_flags) { > > file->private_data = cfile; > > -- > > 2.43.0 > > > > > > > -- > Thanks, > > Steve > Other than that one thing to look at, the changes look good to me.
On 2024-12-19 15:03:48 +0530, Shyam Prasad N wrote: >On Tue, Dec 17, 2024 at 2:22 AM Steve French <smfrench@gmail.com> wrote: >> >> merged into cifs-2.6.git for-next pending review and more testing >> >> On Mon, Dec 16, 2024 at 12:36 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: >> > >> > Previously, deferred file handles were reused only for read >> > operations, this commit extends to reusing deferred handles >> > for write operations. >> > >> > Signed-off-by: Bharath SM <bharathsm@microsoft.com> >> > --- >> > fs/smb/client/file.c | 6 +++++- >> > 1 file changed, 5 insertions(+), 1 deletion(-) >> > >> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c >> > index a58a3333ecc3..98deff1de74c 100644 >> > --- a/fs/smb/client/file.c >> > +++ b/fs/smb/client/file.c >> > @@ -990,7 +990,11 @@ int cifs_open(struct inode *inode, struct file *file) >> > } >> > >> > /* Get the cached handle as SMB2 close is deferred */ >> > - rc = cifs_get_readable_path(tcon, full_path, &cfile); >> > + if (OPEN_FMODE(file->f_flags) & FMODE_WRITE) { >> > + rc = cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile); > >Wondering if FIND_WR_ANY is okay for all use cases? >Specifically, I'm checking where FIND_WR_FSUID_ONLY is relevant. >@Steve French Is this for multiuser mounts? I don't think so, since >multiuser mounts come with their own tcon, and we search writable >files in our tcon's open list. I think this should be FIND_WR_FSUID_ONLY, yeah. (IMHO, that should be the default, and FIND_WR_ANY should be renamed something indicating it should only be used in specific situations and it's probably not what the caller wants.) I have a series I need to resurrect and polish that fixes a few problems along these lines, but it doesn't touch the 'writable file' path. >> > + } else { >> > + rc = cifs_get_readable_path(tcon, full_path, &cfile); >> > + } >> > if (rc == 0) { >> > if (file->f_flags == cfile->f_flags) { >> > file->private_data = cfile; >> > -- >> > 2.43.0 >> > >> > >> >> >> -- >> Thanks, >> >> Steve >> >Other than that one thing to look at, the changes look good to me. > >-- >Regards, >Shyam ~Paul
Thanks Shyam and Paul Aurich for reviewing and sharing comments.! I have attached an updated patch. On Fri, Dec 20, 2024 at 4:17 AM Paul Aurich <paul@darkrain42.org> wrote: > > On 2024-12-19 15:03:48 +0530, Shyam Prasad N wrote: > >On Tue, Dec 17, 2024 at 2:22 AM Steve French <smfrench@gmail.com> wrote: > >> > >> merged into cifs-2.6.git for-next pending review and more testing > >> > >> On Mon, Dec 16, 2024 at 12:36 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: > >> > > >> > Previously, deferred file handles were reused only for read > >> > operations, this commit extends to reusing deferred handles > >> > for write operations. > >> > > >> > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > >> > --- > >> > fs/smb/client/file.c | 6 +++++- > >> > 1 file changed, 5 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c > >> > index a58a3333ecc3..98deff1de74c 100644 > >> > --- a/fs/smb/client/file.c > >> > +++ b/fs/smb/client/file.c > >> > @@ -990,7 +990,11 @@ int cifs_open(struct inode *inode, struct file *file) > >> > } > >> > > >> > /* Get the cached handle as SMB2 close is deferred */ > >> > - rc = cifs_get_readable_path(tcon, full_path, &cfile); > >> > + if (OPEN_FMODE(file->f_flags) & FMODE_WRITE) { > >> > + rc = cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile); > > > >Wondering if FIND_WR_ANY is okay for all use cases? > >Specifically, I'm checking where FIND_WR_FSUID_ONLY is relevant. > >@Steve French Is this for multiuser mounts? I don't think so, since > >multiuser mounts come with their own tcon, and we search writable > >files in our tcon's open list. > > I think this should be FIND_WR_FSUID_ONLY, yeah. (IMHO, that should be the > default, and FIND_WR_ANY should be renamed something indicating it should only > be used in specific situations and it's probably not what the caller wants.) > > I have a series I need to resurrect and polish that fixes a few problems along > these lines, but it doesn't touch the 'writable file' path. > > >> > + } else { > >> > + rc = cifs_get_readable_path(tcon, full_path, &cfile); > >> > + } > >> > if (rc == 0) { > >> > if (file->f_flags == cfile->f_flags) { > >> > file->private_data = cfile; > >> > -- > >> > 2.43.0 > >> > > >> > > >> > >> > >> -- > >> Thanks, > >> > >> Steve > >> > >Other than that one thing to look at, the changes look good to me. > > > >-- > >Regards, > >Shyam > > ~Paul >
Merged into cifs-2.6.git for-next pending more testing On Mon, Dec 23, 2024 at 2:12 AM Bharath SM <bharathsm.hsk@gmail.com> wrote: > > Thanks Shyam and Paul Aurich for reviewing and sharing comments.! > I have attached an updated patch. > > On Fri, Dec 20, 2024 at 4:17 AM Paul Aurich <paul@darkrain42.org> wrote: > > > > On 2024-12-19 15:03:48 +0530, Shyam Prasad N wrote: > > >On Tue, Dec 17, 2024 at 2:22 AM Steve French <smfrench@gmail.com> wrote: > > >> > > >> merged into cifs-2.6.git for-next pending review and more testing > > >> > > >> On Mon, Dec 16, 2024 at 12:36 PM Bharath SM <bharathsm.hsk@gmail.com> wrote: > > >> > > > >> > Previously, deferred file handles were reused only for read > > >> > operations, this commit extends to reusing deferred handles > > >> > for write operations. > > >> > > > >> > Signed-off-by: Bharath SM <bharathsm@microsoft.com> > > >> > --- > > >> > fs/smb/client/file.c | 6 +++++- > > >> > 1 file changed, 5 insertions(+), 1 deletion(-) > > >> > > > >> > diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c > > >> > index a58a3333ecc3..98deff1de74c 100644 > > >> > --- a/fs/smb/client/file.c > > >> > +++ b/fs/smb/client/file.c > > >> > @@ -990,7 +990,11 @@ int cifs_open(struct inode *inode, struct file *file) > > >> > } > > >> > > > >> > /* Get the cached handle as SMB2 close is deferred */ > > >> > - rc = cifs_get_readable_path(tcon, full_path, &cfile); > > >> > + if (OPEN_FMODE(file->f_flags) & FMODE_WRITE) { > > >> > + rc = cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile); > > > > > >Wondering if FIND_WR_ANY is okay for all use cases? > > >Specifically, I'm checking where FIND_WR_FSUID_ONLY is relevant. > > >@Steve French Is this for multiuser mounts? I don't think so, since > > >multiuser mounts come with their own tcon, and we search writable > > >files in our tcon's open list. > > > > I think this should be FIND_WR_FSUID_ONLY, yeah. (IMHO, that should be the > > default, and FIND_WR_ANY should be renamed something indicating it should only > > be used in specific situations and it's probably not what the caller wants.) > > > > I have a series I need to resurrect and polish that fixes a few problems along > > these lines, but it doesn't touch the 'writable file' path. > > > > >> > + } else { > > >> > + rc = cifs_get_readable_path(tcon, full_path, &cfile); > > >> > + } > > >> > if (rc == 0) { > > >> > if (file->f_flags == cfile->f_flags) { > > >> > file->private_data = cfile; > > >> > -- > > >> > 2.43.0 > > >> > > > >> > > > >> > > >> > > >> -- > > >> Thanks, > > >> > > >> Steve > > >> > > >Other than that one thing to look at, the changes look good to me. > > > > > >-- > > >Regards, > > >Shyam > > > > ~Paul > >
diff --git a/fs/smb/client/file.c b/fs/smb/client/file.c index a58a3333ecc3..98deff1de74c 100644 --- a/fs/smb/client/file.c +++ b/fs/smb/client/file.c @@ -990,7 +990,11 @@ int cifs_open(struct inode *inode, struct file *file) } /* Get the cached handle as SMB2 close is deferred */ - rc = cifs_get_readable_path(tcon, full_path, &cfile); + if (OPEN_FMODE(file->f_flags) & FMODE_WRITE) { + rc = cifs_get_writable_path(tcon, full_path, FIND_WR_ANY, &cfile); + } else { + rc = cifs_get_readable_path(tcon, full_path, &cfile); + } if (rc == 0) { if (file->f_flags == cfile->f_flags) { file->private_data = cfile;
Previously, deferred file handles were reused only for read operations, this commit extends to reusing deferred handles for write operations. Signed-off-by: Bharath SM <bharathsm@microsoft.com> --- fs/smb/client/file.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)