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
