Re: [PATCH] smb: enable reuse of deferred file handles for write operations

[Paul Aurich <paul@darkrain42.org>]

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