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
