2018-01-12 10:38 GMT-08:00 Aurélien Aptel <aaptel@xxxxxxxx>:
> 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
��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
