Hi Here is why xfstest 591 must be failing when the lease key is being reused for the unlink compound operation and closetimeo is set to any value other than 0 (deferred close is supported). The splice_test program xfstest 591 uses calls unlink() at the end of the program to delete the file A without closing the file handle first. This file is thus marked for delete and the server returns STATUS_DELETE_PENDING. If the mount options have specified anything other than closetimeo=0 i.e. if we have enabled deferred closes, the open handle to this file (which was not closed by the application), would have deferred closed once the application ends. When the shell script runs the splice_test program again, the same handle from the previous run of the splice_test program - that was supposed to be closed because the file is deleted- is used by the next open to open file A since handle caching is still allowed for its lease. This leads to the error: No such file or directory which we see in the file 591.out.bad. This error was not observed when unlink operation broke leases because when the server issued a lease break to the client, it made the client flush its remaining writes/locks to the server and downgrade its lease from RWH to R. Since handle caching is not involved here anymore, the handle was also not reused anymore by the next open. Now that the patch has removed the lease break communication with the server, something similar to what happens when a client gets lease break notification might need to be done. One solution could be to flush all cached writes to the server and downgrade all leases for open handles to file A to R or 0 as soon as unlink is issued for the file A. In this case, even if some file handles are deferred closed, they would not be reused. A simple repro for this bug when the above patch 1/2 is applied (courtesy of @bharath): 1. Mount share with closetimeo=30 2. (shell1): tail -f /dev/null > myfile 3. (shell2): rm myfile 4. (shell1): ctrl+c to close myfile handle. This would be deferred closed 5. Touch myfile will fail for a period = closetimeo value Thanks Meetakshi On Sat, Jan 6, 2024 at 12:12 AM Steve French <smfrench@xxxxxxxxx> wrote: > > 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
