Received-SPF: pass (google.com: domain of linux-kernel+bounces-148654-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <1a94a15e6863d3844f0bcb58b7b1e17a@manguebit.com>
From: Paulo Alcantara <pc@manguebit.com>
To: David Howells <dhowells@redhat.com>
Cc: dhowells@redhat.com, Steve French <sfrench@samba.org>, Shyam Prasad N
 <sprasad@microsoft.com>, linux-cifs@vger.kernel.org,
 linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] cifs: Fix reacquisition of volume cookie on still-live
 connection
In-Reply-To: <2713340.1713286722@warthog.procyon.org.uk>
References: <14e66691a65e3d05d3d8d50e74dfb366@manguebit.com>
 <3756406.1712244064@warthog.procyon.org.uk>
 <2713340.1713286722@warthog.procyon.org.uk>
Date: Wed, 17 Apr 2024 11:09:27 -0300
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

David Howells <dhowells@redhat.com> writes:

> Paulo Alcantara <pc@manguebit.com> wrote:
>
>> Can't we just move the cookie acquisition to cifs_get_tcon() before it
>> gets added to list @ses->tcon_list.  This way we'll guarantee that the
>> cookie is set only once for the new tcon.
>
> cifs_get_tcon() is used from more than one place and I'm not sure the second
> place (__cifs_construct_tcon()) actually wants a cookie.  I'm not sure what
> that path is for.

__cifs_construct_tcon() is used for creating sessions and tcons under
multiuser mounts.  Whenever an user accesses a multiuser mount and the
client can't find a credential for it, a new session and tcon will be
created for the user accessing the mount -- new accesses from same user
will end up reusing the created session and tcon.

And yes, I don't think we'll need a cookie for those tcons as the client
seems to get the fscache cookie from master tcon (the one created from
mount credentials).

> Could all the (re)setting up being done in cifs_mount_get_tcon() be
> pushed back into cifs_get_tcon()?

AFAICT, yes.  I'd need to look into it to make sure that's safe.

>> Besides, do we want to share a tcon with two different superblocks that
>> have 'fsc' and 'nofsc', respectively?  If not, it would be better to fix
>> match_tcon() as well to handle such case.
>
> Maybe?  What does a tcon *actually* represent?  I note that in
> cifs_match_super(), it's not the only criterion matched upon - so you can, at
> least in apparent theory, get different superblocks for the same tcon anyway.

tcon simply represents a tree connected SMB share.  It can be either an
IPC share (\\srv\IPC$) or the actual share (\\srv\share) we're accessing
the files from.

Consider the following example where a tcon is reused from different
CIFS superblocks:

  mount.cifs //srv/share /mnt/1 -o ${opts} # new super, new tcon
  mount.cifs //srv/share/dir /mnt/2 -o ${opts} # new super, reused tcon

So, /mnt/1/dir/foo and /mnt/2/foo will lead to different inodes.

The two mounts are accessing the same tcon (\\srv\share) but the new
superblock was created because the prefix path "\dir" didn't match in
cifs_match_super().  Trust me, that's a very common scenario.

> This suggests that the tcon might not be the best place for the fscache volume
> cookie as you can have multiple inodes wishing to use the same file cookie if
> there are multiple mounts mounting the same tcon but, say, with different
> mount parameters.

We're not supposed to allow mounts with different parameters reusing
servers, sessions or tcons, so that should be no issue.

> I'm not sure what the right way around this is.  The root of the problem is
> coherency management.  If we make a change to an inode on one mounted
> superblock and this bounces a change notification over to the server that then
> pokes an inode in another mounted superblock on the same machine and causes it
> to be invalidated, you lose your local cache if both inodes refer to the same
> fscache cookie.

Yes, that could be a problem.  Perhaps placing the fscache cookie in the
superblock would be a way to go, so we can handle the different fscache
cookies for the superblocks that contain different prefix paths and
access same tcon.

> Remember: fscache does not do this for you!  It's just a facility by which
> which data can be stored and retrieved.  The netfs is responsible for telling
> it when to invalidate and handling coherency.

ACK.

> That said, it might be possible to time-share a cookie on cifs with leases,
> but the local superblocks would have to know about each other - in which case,
> why are they separate superblocks?

See above why they could be separate superblocks.