tcon and session refcounts, get/put
Subsystems (2)
Hi,
While working on a bug I've found something strange.
cifs_get_tcon(ses, volinfo) looks for a tcon matching volinfo in ses tcon list. If it doesn't find one it create one.
Can someone explain to me why when it finds a matching one, it *puts* the session?
static struct cifs_tcon *cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
{
int rc, xid;
struct cifs_tcon *tcon;
tcon = cifs_find_tcon(ses, volume_info);
if (tcon) {
cifs_dbg(FYI, "Found match on UNC path\n");
/* existing tcon already has a reference */
cifs_put_smb_ses(ses); <---- why?
return tcon;
}
<... code that creates the missing tcon ...>I think the "existing tcon already has a reference" comment refers to the fact that cifs_find_tcon() already increments the refcount of the tcon and so there's no need to do it a second time.
static struct cifs_tcon *cifs_find_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
{
struct list_head *tmp;
struct cifs_tcon *tcon;
spin_lock(&cifs_tcp_ses_lock);
list_for_each(tmp, &ses->tcon_list) {
tcon = list_entry(tmp, struct cifs_tcon, tcon_list);
if (!match_tcon(tcon, volume_info))
continue;
++tcon->tc_count;
spin_unlock(&cifs_tcp_ses_lock);
return tcon;
}
spin_unlock(&cifs_tcp_ses_lock);
return NULL;
}I don't understand the logic here and I have a feeling it might be a bug.
Signature · 4 lines
Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
2018-01-12 10:38 GMT-08:00 Aurélien Aptel <[email protected]>:
Quoted 1 line
Hi,
Hi Aurelien,
Quoted 25 lines
While working on a bug I've found something strange.
cifs_get_tcon(ses, volinfo)looks for a tcon matching volinfo in ses tcon list. If it doesn't find one it create one.Can someone explain to me why when it finds a matching one, it *puts* the session?
static struct
cifs_tcon*cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info){ int rc, xid; struct cifs_tcon *tcon; tcon = cifs_find_tcon(ses, volume_info); if (tcon) { cifs_dbg(FYI, "Found match on UNC path\n"); /* existing tcon already has a reference */ cifs_put_smb_ses(ses); <---- why? return tcon; } <... code that creates the missing tcon ...>
Quoted 3 lines
I think the "existing tcon already has a reference" comment refers to the fact that
cifs_find_tcon()already increments the refcount of the tcon and so there's no need to do it a second time.
Every tcon holds a reference to smb ses, every smb ses hold a reference to tcp ses. Refcount of tcp ses means a number of smb ses referencing this tcp ses. Refcount of smb ses means a number of tcon referencing this smb ses. Refcount of tcon means a number of mounts referencing the tcon.
Suppose we have 1 tcon pointing to 1 smb ses (pointing to 1 tcp ses). Before calling cifs_get_tcon() we have already obtained a reference to smb ses (which increments refcount of this smb ses to 2) and now we are trying to search for tcon that matches ours. If we found one we should increment refcount of this tcon, so, tc_count = 2.
Quoted 19 lines
static struct
cifs_tcon*cifs_find_tcon(struct cifs_ses *ses, struct smb_vol *volume_info){ struct list_head *tmp; struct cifs_tcon *tcon; spin_lock(&cifs_tcp_ses_lock); list_for_each(tmp, &ses->tcon_list) { tcon = list_entry(tmp, struct cifs_tcon, tcon_list); if (!match_tcon(tcon, volume_info)) continue; ++tcon->tc_count; spin_unlock(&cifs_tcp_ses_lock); return tcon; } spin_unlock(&cifs_tcp_ses_lock); return NULL; }
But this tcon already has its own reference to smb ses. So, our reference to smb ses is useless because we doesn't create a new tcon which should hold the reference. So, that's why we drop our reference. At the end we have 1 tcp ses referenced by 1 smb ses referenced by 1 tcon referenced by 2 mounts.
Quoted 3 lines
I don't understand the logic here and I have a feeling it might be a bug.
We probably need a comment describing this behavior, so, an appropriate patch is highly welcomed.
--
Best regards,
Pavel Shilovsky
Quoted 1 line · Pavel Shilovskiy
But this tcon already has its own reference to smb ses. So, our reference to smb ses is useless because we doesn't create a new tcon which should hold the reference. So, that's why we drop our reference. At the end we have 1 tcp ses referenced by 1 smb ses referenced by 1 tcon referenced by 2 mounts.
Much clearer now, thanks Pavel!
Quoted 1 line
We probably need a comment describing this behavior, so, an appropriate patch is highly welcomed.
I will do that.
Signature · 4 lines
Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)