Re: [PATCH] smb: client: require CONFIG_CIFS_DEBUG2=y for accounting tcon bytes stats

On 08/06, Steve French wrote:
>the problem I see with this is that for customer perf issues, it is
>often asked to look at the bytes read and bytes written stats to
>narrow down what is causing the perf issue, which tcon (and thus which
>server share)?

The point is, currently this is a debugging feature that has a perceptible
impact on non-debugging settings, so IMO it should definitely be disabled
by default, regardless of the choice on how to enable it.

That's why I suggested, in private to you, a runtime toggling approach by
writing something to /proc/fs/cifs/Stats to enable this.

>On Thu, Aug 6, 2026 at 2:06 PM Enzo Matsumiya <[email protected]> wrote:
>>
>> cifs_stats_bytes_{read,written} uses tcon->stat_lock to account for
>> R/W bytes.  This is done unconditionally on every read/write since
>> commit fcabb89299d7 ("cifs: simple stats should always be enabled").
>>
>> On a highly concurrent workload, a spinlock in the middle of the R/W
>> codepaths can become heavily contended, which is absurd given its
>> purpose (protect non-actionable debug/stat data).
>>
>> Make tcon->{bytes_read,bytes_written,stat_lock} usage depend on
>> CONFIG_CIFS_DEBUG2=y.
>>
>> Fixes: fcabb89299d7 ("cifs: simple stats should always be enabled")
>> Signed-off-by: Enzo Matsumiya <[email protected]>
>> ---
>>  fs/smb/client/cifs_debug.c |  4 +++-
>>  fs/smb/client/cifsglob.h   | 24 ++++++++++++++++++++----
>>  fs/smb/client/misc.c       |  2 ++
>>  fs/smb/client/smb1ops.c    | 19 +++++++++++++++----
>>  fs/smb/client/smb2ops.c    |  7 +++++--
>>  5 files changed, 45 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/smb/client/cifs_debug.c b/fs/smb/client/cifs_debug.c
>> index 4ed4f55a0bb7..3118174745e0 100644
>> --- a/fs/smb/client/cifs_debug.c
>> +++ b/fs/smb/client/cifs_debug.c
>> @@ -762,11 +762,13 @@ static ssize_t cifs_stats_proc_write(struct file *file,
>>                                         continue;
>>                                 list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
>>                                         atomic_set(&tcon->num_smbs_sent, 0);
>> +#ifdef CONFIG_CIFS_DEBUG2
>>                                         spin_lock(&tcon->stat_lock);
>>                                         tcon->bytes_read = 0;
>>                                         tcon->bytes_written = 0;
>> -                                       tcon->stats_from_time = ktime_get_real_seconds();
>>                                         spin_unlock(&tcon->stat_lock);
>> +#endif /* CONFIG_CIFS_DEBUG2 */
>> +                                       tcon->stats_from_time = ktime_get_real_seconds();
>>                                         if (server->ops->clear_stats)
>>                                                 server->ops->clear_stats(tcon);
>>                                 }
>> diff --git a/fs/smb/client/cifsglob.h b/fs/smb/client/cifsglob.h
>> index 79e4e84f8985..e0e945c1de0f 100644
>> --- a/fs/smb/client/cifsglob.h
>> +++ b/fs/smb/client/cifsglob.h
>> @@ -1240,9 +1240,11 @@ struct cifs_tcon {
>>                         atomic_t smb2_com_failed[NUMBER_OF_SMB2_COMMANDS];
>>                 } smb2_stats;
>>         } stats;
>> +#ifdef CONFIG_CIFS_DEBUG2
>>         __u64    bytes_read;
>>         __u64    bytes_written;
>>         spinlock_t stat_lock;  /* protects the two fields above */
>> +#endif /* CONFIG_CIFS_DEBUG2 */
>>         time64_t stats_from_time;
>>         FILE_SYSTEM_DEVICE_INFO fsDevInfo;
>>         FILE_SYSTEM_ATTRIBUTE_INFO fsAttrInfo; /* ok if fs name truncated */
>> @@ -1655,10 +1657,11 @@ convert_delimiter(char *path, char delim)
>>
>>  #define cifs_stats_inc atomic_inc
>>
>> +#ifdef CONFIG_CIFS_DEBUG2
>>  static inline void cifs_stats_bytes_written(struct cifs_tcon *tcon,
>>                                             unsigned int bytes)
>>  {
>> -       if (bytes) {
>> +       if (likely(bytes)) {
>>                 spin_lock(&tcon->stat_lock);
>>                 tcon->bytes_written += bytes;
>>                 spin_unlock(&tcon->stat_lock);
>> @@ -1668,10 +1671,23 @@ static inline void cifs_stats_bytes_written(struct cifs_tcon *tcon,
>>  static inline void cifs_stats_bytes_read(struct cifs_tcon *tcon,
>>                                          unsigned int bytes)
>>  {
>> -       spin_lock(&tcon->stat_lock);
>> -       tcon->bytes_read += bytes;
>> -       spin_unlock(&tcon->stat_lock);
>> +       if (likely(bytes)) {
>> +               spin_lock(&tcon->stat_lock);
>> +               tcon->bytes_read += bytes;
>> +               spin_unlock(&tcon->stat_lock);
>> +       }
>> +}
>> +#else /* CONFIG_CIFS_DEBUG2 */
>> +static inline void cifs_stats_bytes_written(struct cifs_tcon *tcon,
>> +                                           unsigned int bytes)
>> +{
>> +}
>> +
>> +static inline void cifs_stats_bytes_read(struct cifs_tcon *tcon,
>> +                                        unsigned int bytes)
>> +{
>>  }
>> +#endif /* !CONFIG_CIFS_DEBUG2 */
>>
>>
>>  /*
>> diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
>> index 46e1382e8e04..597dc3c68cd2 100644
>> --- a/fs/smb/client/misc.c
>> +++ b/fs/smb/client/misc.c
>> @@ -145,7 +145,9 @@ tcon_info_alloc(bool dir_leases_enabled, enum smb3_tcon_ref_trace trace)
>>         INIT_LIST_HEAD(&ret_buf->tcon_list);
>>         INIT_LIST_HEAD(&ret_buf->cifs_sb_list);
>>         spin_lock_init(&ret_buf->open_file_lock);
>> +#ifdef CONFIG_CIFS_DEBUG2
>>         spin_lock_init(&ret_buf->stat_lock);
>> +#endif /* CONFIG_CIFS_DEBUG2 */
>>         spin_lock_init(&ret_buf->sb_list_lock);
>>         atomic_set(&ret_buf->num_local_opens, 0);
>>         atomic_set(&ret_buf->num_remote_opens, 0);
>> diff --git a/fs/smb/client/smb1ops.c b/fs/smb/client/smb1ops.c
>> index dc5a8c1da623..484e12cb9181 100644
>> --- a/fs/smb/client/smb1ops.c
>> +++ b/fs/smb/client/smb1ops.c
>> @@ -817,14 +817,25 @@ cifs_clear_stats(struct cifs_tcon *tcon)
>>  static void
>>  cifs_print_stats(struct seq_file *m, struct cifs_tcon *tcon)
>>  {
>> +       u64 bytes_read = 0, bytes_written = 0;
>> +
>> +#ifdef CONFIG_CIFS_DEBUG2
>> +       bytes_read = tcon->bytes_read;
>> +       bytes_written = tcon->bytes_written;
>> +#endif /* CONFIG_CIFS_DEBUG2 */
>> +
>>         seq_printf(m, " Oplocks breaks: %d",
>>                    atomic_read(&tcon->stats.cifs_stats.num_oplock_brks));
>>         seq_printf(m, "\nReads:  %d Bytes: %llu",
>> -                  atomic_read(&tcon->stats.cifs_stats.num_reads),
>> -                  (long long)(tcon->bytes_read));
>> +                  atomic_read(&tcon->stats.cifs_stats.num_reads), bytes_read);
>> +#ifndef CONFIG_CIFS_DEBUG2
>> +       seq_puts(m, " (CONFIG_CIFS_DEBUG2 is disabled)");
>> +#endif /* !CONFIG_CIFS_DEBUG2 */
>>         seq_printf(m, "\nWrites: %d Bytes: %llu",
>> -                  atomic_read(&tcon->stats.cifs_stats.num_writes),
>> -                  (long long)(tcon->bytes_written));
>> +                  atomic_read(&tcon->stats.cifs_stats.num_writes), bytes_written);
>> +#ifndef CONFIG_CIFS_DEBUG2
>> +       seq_puts(m, " (CONFIG_CIFS_DEBUG2 is disabled)");
>> +#endif /* !CONFIG_CIFS_DEBUG2 */
>>         seq_printf(m, "\nFlushes: %d",
>>                    atomic_read(&tcon->stats.cifs_stats.num_flushes));
>>         seq_printf(m, "\nLocks: %d HardLinks: %d Symlinks: %d",
>> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
>> index 192649fec25d..e1e28f202d0c 100644
>> --- a/fs/smb/client/smb2ops.c
>> +++ b/fs/smb/client/smb2ops.c
>> @@ -1425,9 +1425,12 @@ smb2_print_stats(struct seq_file *m, struct cifs_tcon *tcon)
>>          *  Can't display SMB2_NEGOTIATE, SESSION_SETUP, LOGOFF, CANCEL and ECHO
>>          *  totals (requests sent) since those SMBs are per-session not per tcon
>>          */
>> +#ifdef CONFIG_CIFS_DEBUG2
>>         seq_printf(m, "\nBytes read: %llu  Bytes written: %llu",
>> -                  (long long)(tcon->bytes_read),
>> -                  (long long)(tcon->bytes_written));
>> +                  tcon->bytes_read, tcon->bytes_written);
>> +#else /* CONFIG_CIFS_DEBUG2 */
>> +       seq_puts(m, "\nBytes read: 0 Bytes written: 0 (CONFIG_CIFS_DEBUG2 is disabled)");
>> +#endif /* !CONFIG_CIFS_DEBUG2 */
>>         seq_printf(m, "\nOpen files: %d total (local), %d open on server",
>>                    atomic_read(&tcon->num_local_opens),
>>                    atomic_read(&tcon->num_remote_opens));
>> --
>> 2.55.0
>>
>
>
>-- 
>Thanks,
>
>Steve
>