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