Re: [PATCH 2/2] smb: client: retry compound request without reusing lease

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

There was a pending fix to reuse lease key in compound operations on
the smb client, essentially proposed to resolve a customer-reported
bug. A scenario similar to what the client faced would be this:
Imagine a file that has a lot of dirty pages to be written to the
server. If rename/unlink/set path size compound operations are
performed on this file, with the current implementation, we do not
send the lease key. Resultantly, this would lead to the server
assuming that the request came from a new client and it would send a
lease break notification to the same client, on the same connection.
This lease break can lead the client to consume several credits while
writing its dirty pages to the server. A deadlock may arise when the
server stops granting more credits to this connection and the deadlock
would be lifted only when the lease timer on the server expires.

The fix initially proposed just copied the lease key from the inode
and passed it along to SMB2_open_init() from unlink, rename and set
path size compound operations. Incidentally, that exposed a
shortcoming in the smb client-side code. As per MS-SMB2, lease keys
are associated with the file name. Linux cifs client maintains lease
keys with the inode. If the file has any hardlinks, it is possible
that the lease for a file be wrongly reused for an operation on the
hardlink or vice versa. In these cases, the mentioned compound
operations fail with STATUS_INVALID_PARAMETER.

A simple, but hacky fix for the above, as per my discussion with Steve
would be to have a two-phased approach and resend the compound op
request again without the lease key if STATUS_INVALID_PARAMETER is
received. This fix could be easily backported to older kernels since
it would be pretty straightforward and does not involve a lot of code
changes. Such a fallback mechanism should not hurt any functionality
but might impact performance especially in cases where the error is
not because of the usage of wrong lease key and we might end up doing
an extra roundtrip. From what I know, use cases where two or more
file+hardlinks are being used together at the same time are kind of
rare, so we might not run into cases where resending requests in this
fashion has to be performed often.

I understand this is not a perfect or correct fix to the problem, but
for the time being, it might help solve the customer reported issue I
mentioned at the start and could also be backported readily. Since
hurting caching for all hardlinked files is not ideal, I am already
working on another fix that stores the dentry pointer with the lease
key in the cinode object.

>From my discussion with Steve and Shyam, the fix proposed was this:
Instead of having a list of dentries/key dentry pairs, we can store a
dentry pointer in addition to the lease key in the cinode object. The
dentry (as a proxy for filename/path) would be the one the lease key
is associated with. There would be a reference counter to maintain the
number of open handles for that file(path)/dentry. When a new file is
created, not only its lease key, but its dentry too be set in the
cinode object. Whenever there is a new handle opened to this dentry,
the reference count to the leasekey/dentry in the cinode object would
be increased. While reusing the lease key, check if dentry (from the
request) matches the dentry stored in the cinode. If it does, copy the
lease key, if it does not, do not use that lease key. When all file
handles to a dentry have been closed, clear the dentry and lease key
from the cinode object. This has the potential to fix the hardlink
issue since dentries for different hardlinks would be different and
one would not end up reusing lease from another. Some implementation
details I am unsure about is should the open request for a file with
mismatched dentry (which would not get to reuse the lease key) send no
lease context at all, or create a new lease key and use that, or
something else?

I am writing this mail to seek advice/feedback on the situation. Any
suggestions or better solutions to any of the issues mentioned in this
mail are very much appreciated.

Thanks
Meetakshi




On Fri, Jan 5, 2024 at 3:30 PM Stefan Metzmacher <metze@xxxxxxxxx> wrote:
>
> Am 05.01.24 um 10:39 schrieb Shyam Prasad N via samba-technical:
> > On Fri, Jan 5, 2024 at 2:39 AM Tom Talpey <tom@xxxxxxxxxx> wrote:
> >>
> >> On 1/3/2024 9:37 AM, Paulo Alcantara wrote:
> >>> Meetakshi Setiya <meetakshisetiyaoss@xxxxxxxxx> writes:
> >>>
> >>>> As per the discussion with Tom on the previous version of the changes, I
> >>>> conferred with Shyam and Steve about possible workarounds and this seemed like a
> >>>> choice which did the job without much perf drawbacks and code changes. One
> >>>> highlighted difference between the two could be that in the previous
> >>>> version, lease
> >>>> would not be reused for any file with hardlinks at all, even though the inode
> >>>> may hold the correct lease for that particular file. The current changes
> >>>> would take care of this by sending the lease at least once, irrespective of the
> >>>> number of hardlinks.
> >>>
> >>> Thanks for the explanation.  However, the code change size is no excuse
> >>> for providing workarounds rather than the actual fix.
> >>
> >> I have to agree. And it really isn't much of a workaround either.
> >>
> >
> > The original problem, i.e. compound operations like
> > unlink/rename/setsize not sending a lease key is very prevalent on the
> > field.
> > Unfortunately, fixing that exposed this problem with hard links.
> > So Steve suggested getting this fix to a shape where it's fixing the
> > original problem, even if it means that it does not fix it for the
> > case of where there are open handles to multiple hard links to the
> > same file.
> > Only thing we need to be careful of is that it does not make things
> > worse for other workloads.
> >
> >>> A possible way to handle such case would be keeping a list of
> >>> pathname:lease_key pairs inside the inode, so in smb2_compound_op() you
> >>> could look up the lease key by using @dentry.  I'm not sure if there's a
> >>> better way to handle it as I haven't looked into it further.
> >>
> >
> > This seems like a reasonable change to make. That will make sure that
> > we stick to what the protocol recommends.
> > I'm not sure that this change will be a simple one. There could be
> > several places where we make an assumption that the lease is
> > associated with an inode, and not a link.
> >
> > And I'm not yet fully convinced that the spec itself is doing the
> > right thing by tying the lease with the link, rather than the file.
> > Shouldn't the lease protect the data of the file, and not the link
> > itself? If opening two links to the same file with two different lease
> > keys end up breaking each other's leases, what's the point?
>
> I guess the reason for making the lease key per path on
> the client is that the client can't know about possible hardlinks
> before opening the file, but that open wants to use a leasekey...
> Or a "stat" open that won't any lease needs to be done first,
> which doubles the roundtrip for every open.
>
> And hard links are not that common...
>
> Maybe choosing und using a new leasekey would be the
> way to start with and when a hardlink is detected
> the open on the hardlink is closed again and retried
> with the former lease key, which would also upgrade it again.
>
> But sharing the handle lease for two pathnames seems wrong,
> as the idea of the handle lease is to cache the patchname on the client.
>
> While sharing the RW lease between two hardlinks would be desired.
>
> metze





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux