@@ -752,9 +752,9 @@ static ssize_t cifs_stats_proc_write(struct file *file,
for (i = 0; i < NUMBER_OF_SMB2_COMMANDS; i++) {
atomic_set(&server->num_cmds[i], 0);
atomic_set(&server->smb2slowcmd[i], 0);
- server->time_per_cmd[i] = 0;
- server->slowest_cmd[i] = 0;
- server->fastest_cmd[i] = 0;
+ atomic64_set(&server->time_per_cmd[i], 0);
+ WRITE_ONCE(server->slowest_cmd[i], 0);
+ WRITE_ONCE(server->fastest_cmd[i], 0);
}
#endif /* CONFIG_CIFS_STATS2 */
list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) {
@@ -826,9 +826,9 @@ static int cifs_stats_proc_show(struct seq_file *m, void *v)
for (j = 0; j < NUMBER_OF_SMB2_COMMANDS; j++)
seq_printf(m, " %d\t\t%d\t%llu\t\t%u\t%u\n", j,
atomic_read(&server->num_cmds[j]),
- server->time_per_cmd[j],
- server->fastest_cmd[j],
- server->slowest_cmd[j]);
+ (unsigned long long)atomic64_read(&server->time_per_cmd[j]),
+ READ_ONCE(server->fastest_cmd[j]),
+ READ_ONCE(server->slowest_cmd[j]));
for (j = 0; j < NUMBER_OF_SMB2_COMMANDS; j++)
if (atomic_read(&server->smb2slowcmd[j])) {
spin_lock(&server->srv_lock);
@@ -772,7 +772,7 @@ struct TCP_Server_Info {
#ifdef CONFIG_CIFS_STATS2
atomic_t num_cmds[NUMBER_OF_SMB2_COMMANDS]; /* total requests by cmd */
atomic_t smb2slowcmd[NUMBER_OF_SMB2_COMMANDS]; /* count resps > 1 sec */
- __u64 time_per_cmd[NUMBER_OF_SMB2_COMMANDS]; /* total time per cmd */
+ atomic64_t time_per_cmd[NUMBER_OF_SMB2_COMMANDS]; /* total time per cmd */
__u32 slowest_cmd[NUMBER_OF_SMB2_COMMANDS];
__u32 fastest_cmd[NUMBER_OF_SMB2_COMMANDS];
#endif /* STATS2 */
@@ -67,16 +67,26 @@ void __release_mid(struct TCP_Server_Info *server, struct mid_q_entry *midEntry)
if (smb_cmd < NUMBER_OF_SMB2_COMMANDS) {
if (atomic_read(&server->num_cmds[smb_cmd]) == 0) {
- server->slowest_cmd[smb_cmd] = roundtrip_time;
- server->fastest_cmd[smb_cmd] = roundtrip_time;
+ /* First completion for this command: initialize min/max. */
+ WRITE_ONCE(server->slowest_cmd[smb_cmd], roundtrip_time);
+ WRITE_ONCE(server->fastest_cmd[smb_cmd], roundtrip_time);
} else {
- if (server->slowest_cmd[smb_cmd] < roundtrip_time)
- server->slowest_cmd[smb_cmd] = roundtrip_time;
- else if (server->fastest_cmd[smb_cmd] > roundtrip_time)
- server->fastest_cmd[smb_cmd] = roundtrip_time;
+ u32 old;
+
+ old = READ_ONCE(server->slowest_cmd[smb_cmd]);
+ while (old < roundtrip_time &&
+ !try_cmpxchg_relaxed(&server->slowest_cmd[smb_cmd],
+ &old, roundtrip_time))
+ ;
+
+ old = READ_ONCE(server->fastest_cmd[smb_cmd]);
+ while (old > roundtrip_time &&
+ !try_cmpxchg_relaxed(&server->fastest_cmd[smb_cmd],
+ &old, roundtrip_time))
+ ;
}
cifs_stats_inc(&server->num_cmds[smb_cmd]);
- server->time_per_cmd[smb_cmd] += roundtrip_time;
+ atomic64_add(roundtrip_time, &server->time_per_cmd[smb_cmd]);
}
/*
* commands taking longer than one second (default) can be indications
KCSAN reports concurrent read/write races on server->time_per_cmd[], server->slowest_cmd[], and server->fastest_cmd[] inside __release_mid(). These debug counters are updated by every thread completing an smb2 request without holding any lock, which can lead to inaccurate accumulated totals and min/max values. Reduce the data races by protecting the statistics updates: - Convert time_per_cmd[] to atomic64_t and use atomic64_add(). - Use try_cmpxchg_relaxed() retry loops to safely update slowest_cmd[] and fastest_cmd[] - Use WRITE_ONCE() for the initial assignment on first completion, accepting the narrow race which might overwrite the very first statistic. Update cifs_stats_proc_show() and cifs_stats_proc_write() to use the corresponding atomic and READ_ONCE/WRITE_ONCE accessors. Signed-off-by: Frank Sorenson <sorenson@redhat.com> --- Hi all, I'm sending this as an RFC because I'd like guidance on the preferred approach for fixing these STATS2 data races. This patch fixes the races by using atomic64_t and cmpxchg loops for higher accuracy of the timing statistics, with a slight overhead due to the use of another atomic in a hot path. Obviously, locking is out of the question here, so the alternatives I see are: 1. accept this patch to prioritize accuracy; the other two counter fields in STATS2 are already atomic, so this makes the third counter match. And the try_cmpxchg_relaxed loops for slowest_cmd and fastest_cmd should be exercised infrequently, so overhead should be minimal. 2. change nothing about the logic, but wrap the existing bare assignments in WRITE_ONCE()/READ_ONCE() and accept that concurrent updates will sometimes silently clobber each other. Let me know which approach fits best, and I'll submit a formal v1. fs/smb/client/cifs_debug.c | 12 ++++++------ fs/smb/client/cifsglob.h | 2 +- fs/smb/client/transport.c | 24 +++++++++++++++++------- 3 files changed, 24 insertions(+), 14 deletions(-)