directory lease series patches from Enzo #95
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We can do the same cleanup on laundromat. On invalidate_all_cached_dirs(), run laundromat worker with 0 timeout and flush it for immediate + sync cleanup. Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
Make put_work an 'async dput' and then move cfid to dying list so laundromat can cleanup the rest. Other changes: - add drop_cfid() helper to drop entries counter, dput and close synchronously, when possible Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
Move cfid to dying list directly on cached_dir_lease_break(), and schedule laundromat for cleanup there too. Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
Since any cleanup is now done on the local list in laundromat, the dying list can be removed. - entries stays on the main list until they're scheduled for cleanup (->last_access_time == 1) - cached_fids->num_entries is decremented only when cfid transitions from on_list true -> false cached_fid lifecycle on the list becomes: - list_add() on find_or_create_cached_dir() - list_move() to local list on laundromat - list_del() on release callback, if on_list == true (unlikely, see comment in the function) Other changes: - add invalidate_cfid() helper Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
That field is currently used to indicate when a cfid should be removed from the entries list. Since we now keep cfids on the list until they're going down, we can remove the field and use the other existing fields for the same effect. Other changes: - cfids->num_entries follows semantics of list_*() ops - add cfid_expired() helper - check is_valid_cached_dir() even on success when leaving open_cached_dir() Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
close_all_cached_dirs(), invalidate_all_cached_dirs() and free_cached_dirs() have become too similar now, merge their functionality in a single static invalidate_all_cfids() function. This also allows removing free_cached_dirs() altogether as it only requires cancelling the work afterwards (done directly in tconInfoFree()). Other changes: - remove struct cached_dir_dentry Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
free_cached_dir() is no longer used anywhere else. Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
This patch splits the function into 2 separate ones; it not only makes the code clearer, but also allows further and easier enhancements to both. So move the initialization part into init_cached_dir() and add find_cached_dir() for lookups. Other: - drop_cached_dir_by_name(): * use find_cached_dir() * remove no longer used args Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
Enable cfid lookups to be matched with the 3 modes (path, dentry, and lease key) currently used in a single function. Caller exposed function (find_cached_dir()) checks if cfid is mid-creation in open_cached_dir() and retries the lookup, avoiding opening the same path again. Changes: - expose find_cached_dir() - add CFID_LOOKUP_* modes - remove @lookup_only arg from open_cached_dir(), replace, in calllers, with find_cached_dir() where it was true - remove open_cached_dir_by_dentry(), replace with find_cached_dir() - use find_cached_dir() in cached_dir_lease_break() Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
- s/drop_cached_dir_by_name/drop_cached_dir/ make it a generic find + invalidate function to replace drop_cached_dir_by_name() and cached_dir_lease_break() We now funnel any cleanup to laundromat, so we can make the release callback free only dirents, path, and cfid itself. Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
cached_fid validity (usable by callers) is already based on ->time != 0, having other flags/booleans to check the same thing can be confusing and make things unnecessarily complex. This patch removes cached_fid booleans ->has_lease, ->is_open, ->file_all_info_is_valid. Replace their semantics with already existing, simpler checks: - ->is_open is replaced by checking if persistent_fid != 0 - ->has_lease is currently used as a "is valid" check, but we already validate it based on ->time anyway, so drop it - ->file_all_info becomes a pointer and its presence becomes its validity This patch also concretly defines the "is opening" semantic; it's based on ->time == 0, which is used as "creation time", so the only time it's 0 is between allocation and valid (opened/cached) or invalid, and it also never transitions back to 0 again. (->last_access_time follows the same, but it's already used for expiration checks) Other: - add CFID_INVALID_TIME (value 1); this allows us to differentiate between opening (0), valid (jiffies), or invalid (1) - rename time/last_access_time to ctime/atime to follow other common usage of such fields Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
…dren In SMB2_open_init() lookup for a cached parent of target path and set ParentLeaseKey in lease context if found. Introduce invalidate_cached_dirents() to allow revalidation of cached dirents but still keep the cached directory. Testing with e.g. xfstests generic/637 (small simple test) shows a performance gain of ~17% less create/open ops, and ~83% less lease breaks: Before patch Create/open: # tshark -Y 'smb2.create.disposition == 1' -r unpatched.pcap 169 Lease breaks: # tshark -Y 'smb2.cmd == 18' -r unpatched.pcap 12 ----------------- After patch: Create/open: # tshark -Y 'smb2.create.disposition == 1' -r patched.pcap 144 Lease breaks: # tshark -Y 'smb2.cmd == 18' -r patched.pcap 2 Other: - set oparms->cifs_sb in open_cached_dir() as we need it in check_cached_parent(); use CIFS_OPARMS() too - introduce CFID_LOOKUP_PARENT lookup mode (for string paths only) - add cached_fids->dirsep to save dir separator, used by parent lookups Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
Currently, even when we have a valid cached dir, cifs_readdir will not make use of it for the Find request, and will reopen a separate handle in query_dir_first. Fix this by setting cifsFile->fid to cfid->fid and resetting search info parameters. Also add cifs_search_info->reset_scan to indicate SMB2_query_directory_init to include the SMB2_RESTART_SCANS flag. With this, we use query_dir_next directly instead of query_dir_first. This patch also keeps the cfid reference all through cifs_readdir(). To prevent bogus/invalid usage of it, check if cfid is still valid after each possible network call (i.e. where possible reconnects may have happened). Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
The file struct passed down to cifs_readdir() is a stack variable, which means it makes no sense to keep/track it across a cached dir lifetime. Instead, use it to track concurrent accesses to the same cached path, and wait for the previous one to finish filling/emitting. Without this patch, virtually every 'ls' will issue a Find request, even when we have both directory and dirents cached and valid. With this patch, the chances of cache hits increases a lot, so on highly concurrent scenarios, the amount of network calls are drastically reduced. Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
Replace with ->unique_id and ->dtype -- the only fields used from cifs_fattr. Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
When we have an inode on upper levels, pass is_dir down to
smb2_query_path_info() so we can lookup for a cached dir there.
Since we now have a possible recursive lookup in open_cached_dir() e.g.:
cifs_readdir
open_cached_dir
lookup_noperm_positive_unlocked
...
cifs_d_revalidate
...
smb2_query_path_info
find_cached_dir
the cfid must be added to the entries list only after dentry lookup, so
we don't hang on an infinite recursion while waiting for itself to open.
Signed-off-by: Enzo Matsumiya <[email protected]>
Signed-off-by: Steve French <[email protected]>
A dentry is passed to cifs_statfs(), so pass down d_is_dir() to smb2_queryfs() so we can cache/reuse this dir. Other: - make smb2_compound_op a static function, as it's not used anywhere else Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
Don't check root dir dentry in cifs_dentry_needs_reval() as its inode was created before the cfid, so the time check will fail and trigger an unnecessary revalidation. Also account for dir_cache_timeout in time comparison, because if we have a cached dir, we have a lease for it, and, thus, may assume its children are (still) valid. To confirm that, let the ac*max checks go through for granular results. Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
This patch adds usage of RCU and seqlocks for cached dir (list and entries). Traversing the list under RCU allows faster lookups (no locks) and also guarantees that entries being read are not gone (i.e. prevents UAF). seqlocks provides atomicity/consistency when reading entries, allowing callers to re-check cfid in case of write-side invalidation. Combined with refcounting, this new approach provides safety for callers and flexibility for future enhancements. Other: - now that we can inform lookups of cfid changes, set cfid->dentry earlier in open_cached_dir() to allow by-dentry lookups to find this cfid Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
A bit of refactoring, and merge path_no_prefix() into path_to_dentry(). Signed-off-by: Enzo Matsumiya <[email protected]> Signed-off-by: Steve French <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR adds directory lease functionality to the SMB client with 20 patches from Enzo, implementing support for directory leasing, cached directory management, and parent lease key handling.
- Adds parent lease key support to enable directory leasing with proper hierarchical relationships
- Implements cached directory lookup and management with RCU-based concurrency improvements
- Refactors directory cache invalidation and search functionality to support directory leases
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| fs/smb/client/smb2proto.h | Updates function signatures to support directory operations and removes compound query function |
| fs/smb/client/smb2pdu.c | Implements parent lease key checking and directory lease context handling |
| fs/smb/client/smb2ops.c | Refactors query operations to support directory-aware caching and compound queries |
| fs/smb/client/smb2misc.c | Updates lease break handling to use new cached directory API |
| fs/smb/client/smb2inode.c | Adds directory-aware path querying and cached directory invalidation |
| fs/smb/client/smb1ops.c | Updates SMB1 operations to match new function signatures |
| fs/smb/client/readdir.c | Major refactor of directory reading with cached directory support |
| fs/smb/client/misc.c | Updates tcon cleanup to properly handle cached directories |
| fs/smb/client/inode.c | Implements directory-aware validation and cached directory lookup |
| fs/smb/client/file.c | Updates file invalidation to use new cached directory API |
| fs/smb/client/dir.c | Refactors directory operations with cached directory invalidation |
| fs/smb/client/cifsglob.h | Adds directory-aware function signatures and search info fields |
| fs/smb/client/cifsfs.c | Updates filesystem operations to support directory-aware queries |
| fs/smb/client/cifs_debug.c | Updates debug output for new cached directory structure |
| fs/smb/client/cached_dir.h | Major refactor with RCU-based lookup modes and seqlock protection |
| fs/smb/client/cached_dir.c | Complete rewrite implementing RCU-based cached directory management |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (server->capabilities & SMB2_GLOBAL_CAP_DIRECTORY_LEASING) | ||
| rc = check_cached_parent(tcon->cfids, oparms); | ||
|
|
||
| if (!rc || *oplock != SMB2_OPLOCK_LEVEL_NONE) |
There was a problem hiding this comment.
The condition !rc || *oplock != SMB2_OPLOCK_LEVEL_NONE will always call add_lease_context when *oplock != SMB2_OPLOCK_LEVEL_NONE regardless of the rc value from check_cached_parent. This could lead to adding a lease context even when parent checking failed with a critical error.
Suggested change
| if (!rc || *oplock != SMB2_OPLOCK_LEVEL_NONE) | |
| if (!rc) |
| cfid->dentry = dentry; | ||
| cfid->tcon = tcon; | ||
| dentry = NULL; | ||
| write_sequnlock(&cfid->seqlock); |
There was a problem hiding this comment.
Misspelled function name 'write_sequnlock' should be 'write_seqlock_unlock' or similar - this appears to be a typo.
| seq_printf(m, "\n"); | ||
| } | ||
| spin_unlock(&cfids->cfid_list_lock); | ||
| read_sequnlock_excl(&cfids->entries_seqlock); |
There was a problem hiding this comment.
Misspelled function name 'read_sequnlock_excl' should be 'read_seqlock_unlock_excl' or similar - this appears to be a typo.
Suggested change
| read_sequnlock_excl(&cfids->entries_seqlock); | |
| read_seqlock_unlock_excl(&cfids->entries_seqlock); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
20 directory lease related patches