On Sat, 29 Oct 2011 00:12:45 -0500
Steve French <smfrench@xxxxxxxxx> wrote:
> 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?
>
There's no need for any locking here. I think what you want is a per-cpu
counter for these.
--
Jeff Layton <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
