On Fri, Jan 5, 2024 at 4:58 AM Shyam Prasad N <nspmangalore@xxxxxxxxx> wrote: > > On Fri, Jan 5, 2024 at 4:08 PM Stefan Metzmacher <metze@xxxxxxxxx> wrote: > > > > Hi Shyam, > > > > >> 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. > > > > And that's the original problem you try to solve, correct? > Correct. I thought you were proposing this as a solution. > > > > Then there's nothing we can do expect for using a dentry pased > > lease key and live with the fact that they don't allow write caching > > anymore. The RH state should still be granted to both lease keys... > > Yes. It's not ideal. But I guess we need to live with it. > Thanks for the inputs. > > Steve/Paulo/Tom: What do you feel about fixing this in two phases? > > First, take Meetakshi's earlier patch, which would fix the problem of > unnecessary lease breaks (and possible deadlock situation with the > server) due to unlink/rename/setfilesize for files that do not have > multiple hard links. i > .e. during these operations, check if link count for the file is 1. > Only if that is the case, send the lease key for the file. This would > mean that the problem remains for files that have multiple hard links. > But at least the hard link xfstest would pass. Since this approach could be a huge performance degradation for some (albeit rare) workloads (e.g. if hardlinks exist for files, but such files are not opened by different names at the same time), I prefer the two phase approach that simply retries when we get invalid argument without the lease key (which has no risk since the current code just fails, and retry will "fix" the issue albeit not as good as being able to cache the second open) > As a following patch, work on the full fix. i.e. maintain a list of > lease keys for the file, keyed by the dentry. > This patch would replace the cinode->lease_key with a map/list, lookup > the correct lease from the list on usage. > This would obviously remove the check for the link count done by the > above patch. I don't like the idea of hurting caching for all hardlinked files as a hack, so for the longer term solution I prefer a solution that caches the dentry pointer with the lease key, although that brings up the obvious question of whether the dentry could be freed and reallocated in some deferred close cases and cause the lease key to be valid but us not to match it due to new dentry). I lean toward something like: 1) store the dentry for the lease key, not just the lease key in the cifs inode info (today we only store the lease key). We could store an array of lease key/dentry pairs but I worry that this would run the risk of use after free and/or lock contention bugs (and additional memory usage if a malicious app tried to open all hardlinked files) 2) if link count is greater than 1, check that the dentry matches when deciding whether to use the lease key (presumably we don't have to worry about it link count is 1) -- Thanks, Steve
