On 09/01, Steve French wrote:
This does look wrong since it would affect use of acdirmax/acregmax.
So, as per my question in my first follow up reply, it should be
possible to not cache handles, but still cache attributes?
TBH I agree the fix might be wrong, as it seems the problem is more
fundamental than this -- it was more of a PoC, thus sent as RFC.
But my understanding is that if we're not getting leases, we shouldn't
be caching anything at all (i.e. neither files/dirs nor their
attributes).
e.g. something like (handcrafted, untested) in connect.c:cifs_get_tcon():
- else
- nohandlecache = true;
+ else {
+ nohandlecache = true;
+ ctx->acregmax = 0;
+ ctx->acdirmax = 0;
+ }
This illustrates better what I'm asking (and also a clearer intent of
my original patch).
Would like to dig deeper into the failure and see if more intuitive
way to fix it.
It also seems like the if ... nohandlecache check applies more to
whether it is worth calling open_cached_dir_by_dentry ... not to
whether we should leverage acdirmax/acregmax cached dentries
On Sat, Aug 31, 2024 at 11:36 PM Steve French <smfrench@xxxxxxxxx> wrote:
tentatively merged to cifs-2.6.git for-next pending testing and
additional review
On Fri, Aug 30, 2024 at 7:10 PM Enzo Matsumiya <ematsumiya@xxxxxxx> wrote:
>
> Some operations return -EEXIST for a non-existing dir/file because of
> cached attributes.
>
> Fix this by forcing dentry revalidation when nohandlecache is set.
>
> Bug/reproducer:
> Azure Files share, attribute caching timeout is 30s (as
> suggested by Azure), 2 clients mounting the same share.
>
> tcon->nohandlecache is set by cifs_get_tcon() because no directory
> leasing capability is offered.
>
> # client 1 and 2
> $ mount.cifs -o ...,actimeo=30 //server/share /mnt
> $ cd /mnt
>
> # client 1
> $ mkdir dir1
>
> # client 2
> $ ls
> dir1
>
> # client 1
> $ mv dir1 dir2
>
> # client 2
> $ mkdir dir1
> mkdir: cannot create directory ‘dir1’: File exists
> $ ls
> dir2
> $ mkdir dir1
> mkdir: cannot create directory ‘dir1’: File exists
>
> "mkdir dir1" eventually works after 30s (when attribute cache
> expired).
>
> The same behaviour can be observed with files if using some
> non-overwritting operation (e.g. "rm -i").
>
> Signed-off-by: Enzo Matsumiya <ematsumiya@xxxxxxx>
> Tested-by: Henrique Carvalho <henrique.carvalho@xxxxxxxx>
> ---
> fs/smb/client/inode.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
> index dd0afa23734c..5f9c5525385f 100644
> --- a/fs/smb/client/inode.c
> +++ b/fs/smb/client/inode.c
> @@ -2427,6 +2427,9 @@ cifs_dentry_needs_reval(struct dentry *dentry)
> if (!lookupCacheEnabled)
> return true;
>
> + if (tcon->nohandlecache)
> + return true;
> +
> if (!open_cached_dir_by_dentry(tcon, dentry->d_parent, &cfid)) {
> spin_lock(&cfid->fid_lock);
> if (cfid->time && cifs_i->time > cfid->time) {
> --
> 2.46.0
Cheers,
Enzo
