tcon and session refcounts, get/put

tcon and session refcounts, get/put

[Aurélien Aptel]

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.

-- 
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)

RE: tcon and session refcounts, get/put

[Pavel Shilovskiy]

2018-01-12 10:38 GMT-08:00 Aurélien Aptel <aaptel@suse.com>:
> Hi,

Hi Aurelien,

>
> 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.

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.

>
> 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.

>
> 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

RE: tcon and session refcounts, get/put

[Aurélien Aptel]

Pavel Shilovskiy <pshilov-0li6OtcxBFHby3iVrkZq2A@public.gmane.org> writes:
> 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!

> We probably need a comment describing this behavior, so, an appropriate patch is highly welcomed.

I will do that.

-- 
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)