Re: [PATCH v2 02/53] CIFS: Allow SMB2 statistics to be tracked

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 28, 2011 at 11:48 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Fri, 28 Oct 2011 23:54:13 +0400
> Pavel Shilovsky <piastry@xxxxxxxxxxx> wrote:
>
>> From: Steve French <sfrench@xxxxxxxxxx>
>>
>> Adding SMB2 statistics requires changes to the way cifs handles stats.
>> Since there are only 19 command codes, it also is easier to track by exact
>> command code than it was for cifs.  Turn the counters for protocol
>> ops sent to be a union (one struct for cifs, one for smb2).  While at it
>> split out the functions which clear stats and prints stats into their own
>> subfunctions so they are easy to read and don't go past 80 columns.
>>
>> Signed-off-by: Steve French <sfrench@xxxxxxxxxx>
>> Signed-off-by: Pavel Shilovsky <piastry@xxxxxxxxxxx>
>> ---
>>  fs/cifs/cifs_debug.c |  146 ++++++++++++++++++++++++++++++-------------------
>>  fs/cifs/cifsglob.h   |   56 ++++++++++++-------
>>  fs/cifs/cifssmb.c    |   54 +++++++++---------
>>  fs/cifs/misc.c       |    2 +-
>>  4 files changed, 152 insertions(+), 106 deletions(-)
>>
>> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
>> index 84e8c07..8ccdb15 100644
>> --- a/fs/cifs/cifs_debug.c
>> +++ b/fs/cifs/cifs_debug.c
>> @@ -249,6 +249,55 @@ static const struct file_operations cifs_debug_data_proc_fops = {
>>  };
>>
>>  #ifdef CONFIG_CIFS_STATS
>> +
>> +#ifdef CONFIG_CIFS_SMB2
>> +static void smb2_clear_stats(struct cifs_tcon *tcon)
>> +{
>> +     int i;
>> +
>> +     atomic_set(&tcon->num_smbs_sent, 0);
>> +     for (i = 0; i < NUMBER_OF_SMB2_COMMANDS; i++) {
>> +             atomic_set(&tcon->stats.smb2_stats.smb2_com_sent[i], 0);
>> +             atomic_set(&tcon->stats.smb2_stats.smb2_com_fail[i], 0);
>> +     }
>> +}
>> +#endif /* CONFIG_CIFS_SMB2 */
>> +
>> +static void clear_cifs_stats(struct cifs_tcon *tcon)
>> +{
>> +     atomic_set(&tcon->num_smbs_sent, 0);
>> +
>> +#ifdef CONFIG_CIFS_SMB2
>> +     if (tcon->ses->server->is_smb2) {
>> +             smb2_clear_stats(tcon);
>> +             return;
>> +     }
>> +#endif /* CONFIG_CIFS_SMB2 */
>> +
>
>     ^^^^^^^^^^^^^
> The above logic should be encapsulated in smb2_clear_stats, and that
> function should just be a noop when CONFIG_CIFS_SMB2 is not set.

makes sense - could be easier to read your way


>> +     /* cifs specific statistics, not applicable to smb2 sessions */
>> +     atomic_set(&tcon->stats.cifs_stats.num_writes, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_reads, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_flushes, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_oplock_brks, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_opens, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_posixopens, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_posixmkdirs, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_closes, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_deletes, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_mkdirs, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_rmdirs, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_renames, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_t2renames, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_ffirst, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_fnext, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_fclose, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_hardlinks, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_symlinks, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_locks, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_acl_get, 0);
>> +     atomic_set(&tcon->stats.cifs_stats.num_acl_set, 0);
>> +}
>> +
>>  static ssize_t cifs_stats_proc_write(struct file *file,
>>               const char __user *buffer, size_t count, loff_t *ppos)
>>  {
>> @@ -279,25 +328,7 @@ static ssize_t cifs_stats_proc_write(struct file *file,
>>                                       tcon = list_entry(tmp3,
>>                                                         struct cifs_tcon,
>>                                                         tcon_list);
>> -                                     atomic_set(&tcon->num_smbs_sent, 0);
>> -                                     atomic_set(&tcon->num_writes, 0);
>> -                                     atomic_set(&tcon->num_reads, 0);
>> -                                     atomic_set(&tcon->num_oplock_brks, 0);
>> -                                     atomic_set(&tcon->num_opens, 0);
>> -                                     atomic_set(&tcon->num_posixopens, 0);
>> -                                     atomic_set(&tcon->num_posixmkdirs, 0);
>> -                                     atomic_set(&tcon->num_closes, 0);
>> -                                     atomic_set(&tcon->num_deletes, 0);
>> -                                     atomic_set(&tcon->num_mkdirs, 0);
>> -                                     atomic_set(&tcon->num_rmdirs, 0);
>> -                                     atomic_set(&tcon->num_renames, 0);
>> -                                     atomic_set(&tcon->num_t2renames, 0);
>> -                                     atomic_set(&tcon->num_ffirst, 0);
>> -                                     atomic_set(&tcon->num_fnext, 0);
>> -                                     atomic_set(&tcon->num_fclose, 0);
>> -                                     atomic_set(&tcon->num_hardlinks, 0);
>> -                                     atomic_set(&tcon->num_symlinks, 0);
>> -                                     atomic_set(&tcon->num_locks, 0);
>> +                                     clear_cifs_stats(tcon);
>>                               }
>>                       }
>>               }
>> @@ -307,6 +338,44 @@ static ssize_t cifs_stats_proc_write(struct file *file,
>>       return count;
>>  }
>>
>> +static void cifs_stats_print(struct seq_file *m, struct cifs_tcon *tcon)
>> +{
>> +     if (tcon->need_reconnect)
>> +             seq_puts(m, "\tDISCONNECTED ");
>> +     seq_printf(m, "\nSMBs: %d Oplock Breaks: %d",
>> +             atomic_read(&tcon->num_smbs_sent),
>> +             atomic_read(&tcon->stats.cifs_stats.num_oplock_brks));
>> +     seq_printf(m, "\nReads:  %d Bytes: %lld",
>> +             atomic_read(&tcon->stats.cifs_stats.num_reads),
>> +             (long long)(tcon->bytes_read));
>> +     seq_printf(m, "\nWrites: %d Bytes: %lld",
>> +             atomic_read(&tcon->stats.cifs_stats.num_writes),
>> +             (long long)(tcon->bytes_written));
>> +     seq_printf(m, "\nFlushes: %d",
>> +             atomic_read(&tcon->stats.cifs_stats.num_flushes));
>> +     seq_printf(m, "\nLocks: %d HardLinks: %d Symlinks: %d",
>> +             atomic_read(&tcon->stats.cifs_stats.num_locks),
>> +             atomic_read(&tcon->stats.cifs_stats.num_hardlinks),
>> +             atomic_read(&tcon->stats.cifs_stats.num_symlinks));
>> +     seq_printf(m, "\nOpens: %d Closes: %d Deletes: %d",
>> +             atomic_read(&tcon->stats.cifs_stats.num_opens),
>> +             atomic_read(&tcon->stats.cifs_stats.num_closes),
>> +             atomic_read(&tcon->stats.cifs_stats.num_deletes));
>> +     seq_printf(m, "\nPosix Opens: %d Posix Mkdirs: %d",
>> +             atomic_read(&tcon->stats.cifs_stats.num_posixopens),
>> +             atomic_read(&tcon->stats.cifs_stats.num_posixmkdirs));
>> +     seq_printf(m, "\nMkdirs: %d Rmdirs: %d",
>> +             atomic_read(&tcon->stats.cifs_stats.num_mkdirs),
>> +             atomic_read(&tcon->stats.cifs_stats.num_rmdirs));
>> +     seq_printf(m, "\nRenames: %d T2 Renames %d",
>> +             atomic_read(&tcon->stats.cifs_stats.num_renames),
>> +             atomic_read(&tcon->stats.cifs_stats.num_t2renames));
>> +     seq_printf(m, "\nFindFirst: %d FNext %d FClose %d",
>> +             atomic_read(&tcon->stats.cifs_stats.num_ffirst),
>> +             atomic_read(&tcon->stats.cifs_stats.num_fnext),
>> +             atomic_read(&tcon->stats.cifs_stats.num_fclose));
>> +}
>> +
>>  static int cifs_stats_proc_show(struct seq_file *m, void *v)
>>  {
>>       int i;
>> @@ -354,44 +423,7 @@ static int cifs_stats_proc_show(struct seq_file *m, void *v)
>>                                                 tcon_list);
>>                               i++;
>>                               seq_printf(m, "\n%d) %s", i, tcon->treeName);
>> -                             if (tcon->need_reconnect)
>> -                                     seq_puts(m, "\tDISCONNECTED ");
>> -                             seq_printf(m, "\nSMBs: %d Oplock Breaks: %d",
>> -                                     atomic_read(&tcon->num_smbs_sent),
>> -                                     atomic_read(&tcon->num_oplock_brks));
>> -                             seq_printf(m, "\nReads:  %d Bytes: %lld",
>> -                                     atomic_read(&tcon->num_reads),
>> -                                     (long long)(tcon->bytes_read));
>> -                             seq_printf(m, "\nWrites: %d Bytes: %lld",
>> -                                     atomic_read(&tcon->num_writes),
>> -                                     (long long)(tcon->bytes_written));
>> -                             seq_printf(m, "\nFlushes: %d",
>> -                                     atomic_read(&tcon->num_flushes));
>> -                             seq_printf(m, "\nLocks: %d HardLinks: %d "
>> -                                           "Symlinks: %d",
>> -                                     atomic_read(&tcon->num_locks),
>> -                                     atomic_read(&tcon->num_hardlinks),
>> -                                     atomic_read(&tcon->num_symlinks));
>> -                             seq_printf(m, "\nOpens: %d Closes: %d "
>> -                                           "Deletes: %d",
>> -                                     atomic_read(&tcon->num_opens),
>> -                                     atomic_read(&tcon->num_closes),
>> -                                     atomic_read(&tcon->num_deletes));
>> -                             seq_printf(m, "\nPosix Opens: %d "
>> -                                           "Posix Mkdirs: %d",
>> -                                     atomic_read(&tcon->num_posixopens),
>> -                                     atomic_read(&tcon->num_posixmkdirs));
>> -                             seq_printf(m, "\nMkdirs: %d Rmdirs: %d",
>> -                                     atomic_read(&tcon->num_mkdirs),
>> -                                     atomic_read(&tcon->num_rmdirs));
>> -                             seq_printf(m, "\nRenames: %d T2 Renames %d",
>> -                                     atomic_read(&tcon->num_renames),
>> -                                     atomic_read(&tcon->num_t2renames));
>> -                             seq_printf(m, "\nFindFirst: %d FNext %d "
>> -                                           "FClose %d",
>> -                                     atomic_read(&tcon->num_ffirst),
>> -                                     atomic_read(&tcon->num_fnext),
>> -                                     atomic_read(&tcon->num_fclose));
>> +                             cifs_stats_print(m, tcon);
>>                       }
>>               }
>>       }
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 4c38b04..6dfc7ef 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -292,6 +292,7 @@ struct TCP_Server_Info {
>>       bool    sec_kerberos;           /* supports plain Kerberos */
>>       bool    sec_mskerberos;         /* supports legacy MS Kerberos */
>>       bool    large_buf;              /* is current buffer large? */
>> +     bool    is_smb2;                /* smb2 not cifs protocol negotiated */
>>       struct delayed_work     echo; /* echo ping workqueue job */
>>       struct kvec *iov;       /* reusable kvec array for receives */
>>       unsigned int nr_iov;    /* number of kvecs in array */
>> @@ -392,6 +393,9 @@ struct cifs_ses {
>>     negotiate one of the older LANMAN dialects */
>>  #define CIFS_SES_LANMAN 8
>>  #define CIFS_SES_SMB2 16
>> +
>> +#define NUMBER_OF_SMB2_COMMANDS 0x0013
>> +
>>  /*
>>   * there is one of these for each connection to a resource on a particular
>>   * session
>> @@ -409,27 +413,37 @@ struct cifs_tcon {
>>       enum statusEnum tidStatus;
>>  #ifdef CONFIG_CIFS_STATS
>>       atomic_t num_smbs_sent;
>> -     atomic_t num_writes;
>> -     atomic_t num_reads;
>> -     atomic_t num_flushes;
>> -     atomic_t num_oplock_brks;
>> -     atomic_t num_opens;
>> -     atomic_t num_closes;
>> -     atomic_t num_deletes;
>> -     atomic_t num_mkdirs;
>> -     atomic_t num_posixopens;
>> -     atomic_t num_posixmkdirs;
>> -     atomic_t num_rmdirs;
>> -     atomic_t num_renames;
>> -     atomic_t num_t2renames;
>> -     atomic_t num_ffirst;
>> -     atomic_t num_fnext;
>> -     atomic_t num_fclose;
>> -     atomic_t num_hardlinks;
>> -     atomic_t num_symlinks;
>> -     atomic_t num_locks;
>> -     atomic_t num_acl_get;
>> -     atomic_t num_acl_set;
>> +     union {
>> +             struct {
>> +                     atomic_t num_writes;
>> +                     atomic_t num_reads;
>> +                     atomic_t num_flushes;
>> +                     atomic_t num_oplock_brks;
>> +                     atomic_t num_opens;
>> +                     atomic_t num_closes;
>> +                     atomic_t num_deletes;
>> +                     atomic_t num_mkdirs;
>> +                     atomic_t num_posixopens;
>> +                     atomic_t num_posixmkdirs;
>> +                     atomic_t num_rmdirs;
>> +                     atomic_t num_renames;
>> +                     atomic_t num_t2renames;
>> +                     atomic_t num_ffirst;
>> +                     atomic_t num_fnext;
>> +                     atomic_t num_fclose;
>> +                     atomic_t num_hardlinks;
>> +                     atomic_t num_symlinks;
>> +                     atomic_t num_locks;
>> +                     atomic_t num_acl_get;
>> +                     atomic_t num_acl_set;
>> +             } cifs_stats;
>> +#ifdef CONFIG_CIFS_SMB2
>> +             struct {
>> +                     atomic_t smb2_com_sent[NUMBER_OF_SMB2_COMMANDS];
>> +                     atomic_t smb2_com_fail[NUMBER_OF_SMB2_COMMANDS];
>> +             } smb2_stats;
>
> Is it really necessary to do this with atomics? Those can have
> significant performance impact (TLB flushes, and we don't seem to need the
> guarantees that they provide for simple counters like this. Perhaps
> these should be switched to per-cpu variables or just plain ints?

presumably there is precedent for use of atomics rather than
wrapping updates to these in a mutex - if they are just int
would we have a case where the two overlapping updates could
badly corrupt the value?


-- 
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux