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? > A list would also allow for better handling of lease revocation. > It seems to me this approach basically discards the original lease, > putting the client's cached data at risk, no? And what happens if > EINVAL comes back again, or the connection breaks? Is this a > recoverable situation? > > Also, what's up with the xfstest the robot mailed about? Meetakshi is investigating this one. Initial investigations led us to believe that this is related to deferred closes. The failing tests do show that the close is getting delayed, and that setting closetimeo=0 causes the test to pass. However, it is not clear why the test started failing only now. We'll have the answers soon. > > Tom. -- Regards, Shyam
