The main debugging benefit I get from the xid is simply being able to
track in logs which operations I am looking at when running something
like dbench with lots of processes in parallel or even when one
process but log entries got lost. Pid isn't always as helpful since
log entries get dropped - it is not always clear which call is failing
when pid does multiple operations. Quick modifications of other log
entries to add xid is pretty easy when the xid in a particular
important cFYI or cERROR is not clear in a complex trace.
On Tue, Jun 19, 2012 at 5:59 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Sat, 9 Jun 2012 10:26:04 +0400
> Pavel Shilovsky <pshilovsky@xxxxxxxxx> wrote:
>
>> and rename variables around the code changes.
>>
>> Signed-off-by: Pavel Shilovsky <pshilovsky@xxxxxxxxx>
>> ---
>> fs/cifs/cifsglob.h | 6 ++++
>> fs/cifs/cifsproto.h | 5 +--
>> fs/cifs/connect.c | 71 ++++++++++++++++++++++++++++++---------------------
>> fs/cifs/smb1ops.c | 2 +
>> 4 files changed, 52 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index 812f27c..1266e2e 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -160,6 +160,7 @@ struct mid_q_entry;
>> struct TCP_Server_Info;
>> struct cifsFileInfo;
>> struct cifs_ses;
>> +struct cifs_tcon;
>>
>> struct smb_version_operations {
>> int (*send_cancel)(struct TCP_Server_Info *, void *,
>> @@ -201,6 +202,11 @@ struct smb_version_operations {
>> const struct nls_table *);
>> /* close smb session */
>> int (*logoff)(const int, struct cifs_ses *);
>> + /* connect to a server share */
>> + int (*tree_connect)(const int, struct cifs_ses *, const char *,
>> + struct cifs_tcon *, const struct nls_table *);
>> + /* close tree connecion */
>> + int (*tree_disconnect)(const int, struct cifs_tcon *);
>> };
>>
>> struct smb_version_values {
>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
>> index 8e6cd38..2f4f661 100644
>> --- a/fs/cifs/cifsproto.h
>> +++ b/fs/cifs/cifsproto.h
>> @@ -183,9 +183,8 @@ extern int cifs_setup_session(const int xid, struct cifs_ses *ses,
>> struct nls_table *nls_info);
>> extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses);
>>
>> -extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses,
>> - const char *tree, struct cifs_tcon *tcon,
>> - const struct nls_table *);
>> +extern int CIFSTCon(const int xid, struct cifs_ses *ses, const char *tree,
>> + struct cifs_tcon *tcon, const struct nls_table *);
>>
>> extern int CIFSFindFirst(const int xid, struct cifs_tcon *tcon,
>> const char *searchName, const struct nls_table *nls_codepage,
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 9679352..d14bd45 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -2552,7 +2552,8 @@ cifs_put_tcon(struct cifs_tcon *tcon)
>> spin_unlock(&cifs_tcp_ses_lock);
>>
>> xid = GetXid();
>> - CIFSSMBTDis(xid, tcon);
>> + if (ses->server->ops->tree_disconnect)
>> + ses->server->ops->tree_disconnect(xid, tcon);
>> _FreeXid(xid);
>>
>> cifs_fscache_release_super_cookie(tcon);
>> @@ -2577,6 +2578,11 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
>> return tcon;
>> }
>>
>> + if (!ses->server->ops->tree_connect) {
>> + rc = -ENOSYS;
>> + goto out_fail;
>> + }
>> +
>> tcon = tconInfoAlloc();
>> if (tcon == NULL) {
>> rc = -ENOMEM;
>> @@ -2599,13 +2605,15 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
>> goto out_fail;
>> }
>>
>> - /* BB Do we need to wrap session_mutex around
>> - * this TCon call and Unix SetFS as
>> - * we do on SessSetup and reconnect? */
>> + /*
>> + * BB Do we need to wrap session_mutex around this TCon call and Unix
>> + * SetFS as we do on SessSetup and reconnect?
>> + */
>> xid = GetXid();
>> - rc = CIFSTCon(xid, ses, volume_info->UNC, tcon, volume_info->local_nls);
>> + rc = ses->server->ops->tree_connect(xid, ses, volume_info->UNC, tcon,
>> + volume_info->local_nls);
>> FreeXid(xid);
>> - cFYI(1, "CIFS Tcon rc = %d", rc);
>> + cFYI(1, "Tcon rc = %d", rc);
>> if (rc)
>> goto out_fail;
>>
>> @@ -2614,10 +2622,11 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
>> cFYI(1, "DFS disabled (%d)", tcon->Flags);
>> }
>> tcon->seal = volume_info->seal;
>> - /* we can have only one retry value for a connection
>> - to a share so for resources mounted more than once
>> - to the same server share the last value passed in
>> - for the retry flag is used */
>> + /*
>> + * We can have only one retry value for a connection to a share so for
>> + * resources mounted more than once to the same server share the last
>> + * value passed in for the retry flag is used.
>> + */
>> tcon->retry = volume_info->retry;
>> tcon->nocase = volume_info->nocase;
>> tcon->local_lease = volume_info->local_lease;
>> @@ -2751,37 +2760,41 @@ out:
>> }
>>
>> int
>> -get_dfs_path(int xid, struct cifs_ses *pSesInfo, const char *old_path,
>> - const struct nls_table *nls_codepage, unsigned int *pnum_referrals,
>> - struct dfs_info3_param **preferrals, int remap)
>> +get_dfs_path(int xid, struct cifs_ses *ses, const char *old_path,
>> + const struct nls_table *nls_codepage, unsigned int *num_referrals,
>> + struct dfs_info3_param **referrals, int remap)
>> {
>> char *temp_unc;
>> int rc = 0;
>>
>> - *pnum_referrals = 0;
>> - *preferrals = NULL;
>> + if (!ses->server->ops->tree_connect)
>> + return -ENOSYS;
>> +
>> + *num_referrals = 0;
>> + *referrals = NULL;
>>
>> - if (pSesInfo->ipc_tid == 0) {
>> + if (ses->ipc_tid == 0) {
>> temp_unc = kmalloc(2 /* for slashes */ +
>> - strnlen(pSesInfo->serverName,
>> - SERVER_NAME_LEN_WITH_NULL * 2)
>> - + 1 + 4 /* slash IPC$ */ + 2,
>> - GFP_KERNEL);
>> + strnlen(ses->serverName, SERVER_NAME_LEN_WITH_NULL * 2)
>> + + 1 + 4 /* slash IPC$ */ + 2, GFP_KERNEL);
>> if (temp_unc == NULL)
>> return -ENOMEM;
>> temp_unc[0] = '\\';
>> temp_unc[1] = '\\';
>> - strcpy(temp_unc + 2, pSesInfo->serverName);
>> - strcpy(temp_unc + 2 + strlen(pSesInfo->serverName), "\\IPC$");
>> - rc = CIFSTCon(xid, pSesInfo, temp_unc, NULL, nls_codepage);
>> - cFYI(1, "CIFS Tcon rc = %d ipc_tid = %d", rc, pSesInfo->ipc_tid);
>> + strcpy(temp_unc + 2, ses->serverName);
>> + strcpy(temp_unc + 2 + strlen(ses->serverName), "\\IPC$");
>> + rc = ses->server->ops->tree_connect(xid, ses, temp_unc, NULL,
>> + nls_codepage);
>> + cFYI(1, "Tcon rc = %d ipc_tid = %d", rc, ses->ipc_tid);
>> kfree(temp_unc);
>> }
>> if (rc == 0)
>> - rc = CIFSGetDFSRefer(xid, pSesInfo, old_path, preferrals,
>> - pnum_referrals, nls_codepage, remap);
>> - /* BB map targetUNCs to dfs_info3 structures, here or
>> - in CIFSGetDFSRefer BB */
>> + rc = CIFSGetDFSRefer(xid, ses, old_path, referrals,
>> + num_referrals, nls_codepage, remap);
>> + /*
>> + * BB - map targetUNCs to dfs_info3 structures, here or in
>> + * CIFSGetDFSRefer.
>> + */
>>
>> return rc;
>> }
>> @@ -3758,7 +3771,7 @@ out:
>> * pointer may be NULL.
>> */
>> int
>> -CIFSTCon(unsigned int xid, struct cifs_ses *ses,
>> +CIFSTCon(const int xid, struct cifs_ses *ses,
>> const char *tree, struct cifs_tcon *tcon,
>> const struct nls_table *nls_codepage)
>> {
>> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
>> index ba3071f..1a3f08c 100644
>> --- a/fs/cifs/smb1ops.c
>> +++ b/fs/cifs/smb1ops.c
>> @@ -432,6 +432,8 @@ struct smb_version_operations smb1_operations = {
>> .negotiate = cifs_negotiate,
>> .sess_setup = CIFS_SessSetup,
>> .logoff = CIFSSMBLogoff,
>> + .tree_connect = CIFSTCon,
>> + .tree_disconnect = CIFSSMBTDis,
>> };
>>
>> struct smb_version_values smb1_values = {
>
> Looks fine other than the type change on the xid. Note too, that if you
> wanted to just remove all of that xid stuff, I'd be even happier. Steve
> seems to like it for some reason though, so we'll need to convince him.
>
> IMO, it's just unnecessary clutter that gets in the way of the code
> clarity. It's also very unclear that FreeXid() has a side effect (a
> debug printk).
>
> A better scheme would be to turn all of those FreeXid calls into an
> explict cFYI that prints the name of the function and maybe the return
> code. The xid could be replaced with current->pid, which would
> eliminate GetXid.
>
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
> --
> 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
--
Thanks,
Steve
--
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
