Henrique review2 - 3 changesets #119
Conversation
is_open, has_lease and on_list are stored in the same bitfield byte in
struct cached_fid but are updated in different code paths that may run
concurrently. Bitfield assignments generate byte read–modify–write
operations (e.g. `orb $mask, addr` on x86_64), so updating one flag can
restore stale values of the others.
A possible interleaving is:
CPU1: load old byte (has_lease=1, on_list=1)
CPU2: clear both flags (store 0)
CPU1: RMW store (old | IS_OPEN) -> reintroduces cleared bits
To avoid this class of races, convert these flags to separate bool
fields.
Signed-off-by: Henrique Carvalho <[email protected]>
Signed-off-by: Steve French <[email protected]>
It was possible for two query interface works to be concurrently trying to update the interfaces. Prevent this by checking and updating iface_last_update under iface_lock. Signed-off-by: Henrique Carvalho <[email protected]> Signed-off-by: Steve French <[email protected]>
There is a missing ses->iface_lock in cifs_setup_session, around ses->iface_last_update. Signed-off-by: Henrique Carvalho <[email protected]> Signed-off-by: Steve French <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adjusts how CIFS/SMB client code rate-limits and synchronizes server interface list refreshes, and tweaks cached directory handle flag storage.
Changes:
- Move the “don’t query interfaces too frequently” logic to
SMB3_request_interfaces()underses->iface_lock. - Protect
ses->iface_last_updatereset incifs_setup_session()withses->iface_lock. - Replace
struct cached_fidbitfieldboolflags with plainboolfields.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| fs/smb/client/smb2ops.c | Relocates interface refresh throttling and updates iface_last_update handling in SMB3_request_interfaces(). |
| fs/smb/client/connect.c | Adds locking around iface_last_update = 0 to match iface_lock protection. |
| fs/smb/client/cached_dir.h | Changes cached_fid flag fields from bitfields to standard bools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| spin_lock(&ses->iface_lock); | ||
| if (ses->iface_last_update && | ||
| time_before(jiffies, ses->iface_last_update + | ||
| (SMB_INTERFACE_POLL_INTERVAL * HZ))) | ||
| (SMB_INTERFACE_POLL_INTERVAL * HZ))) { | ||
| spin_unlock(&ses->iface_lock); | ||
| return 0; | ||
| } | ||
|
|
||
| ses->iface_last_update = jiffies; | ||
|
|
||
| spin_unlock(&ses->iface_lock); |
There was a problem hiding this comment.
SMB3_request_interfaces() now updates ses->iface_last_update before issuing SMB2_ioctl(). If the ioctl fails with a transient error (anything other than -EOPNOTSUPP), this will still throttle subsequent refresh attempts for SMB_INTERFACE_POLL_INTERVAL, whereas previously iface_last_update was only updated after parsing (i.e., not updated when the ioctl failed). Consider only updating iface_last_update on success / -EOPNOTSUPP, or restoring the previous value on other errors while still preventing concurrent ioctl calls (e.g., via an in-progress flag or rollback under iface_lock).
No description provided.