Messages in this thread | | | From | Paulo Alcantara <> | Subject | Re: [PATCH] cifs: Fix reacquisition of volume cookie on still-live connection | Date | Wed, 17 Apr 2024 11:09:27 -0300 |
| |
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.
| |