Hi Metze, 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... > That does makes sense. > 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. That would not work today, as the former lease key would be associated with the other hardlink. And would result in the server returning STATUS_INVALID_PARAMETER. > > 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 -- Regards, Shyam
