> So, as per my question in my first follow up reply, it should be
possible to not cache handles, but still cache attributes?
Yes - there are two types of caching.
Safe caching of file attributes (size and mtime e.g.) and existence
with file leases (for open files, or files whose close has been
deferred) or file or directory existence with directory leases on the
parent.
"actimeo" attribute caching (less safe): Using actimeo sets all
acregmax and acdirmax to the same value. If this option is not
specified, the cifs (and similarly for NFS) client uses the defaults.
"acregmax" controls how long mtime/size and existence of all are
cached, while acdirmax does the same thing for directories (note that
for directories, caching their existence can save much time in open of
files in deep directory trees, it is less common that apps care about
the mtime or ctime of a directory - just that the path is still valid
- ie that the directory exists).
cifs.ko sets a much lower value for actimeo by default than nfs, but
it is still technically "unsafe" so applications that need 100%
accuracy on timestamps or size of files being updated by other clients
should set acregmax smaller (or to 0) - it is usually safe to keep
acdirmax to a higher value.
By default even if we do not have a lease - we will cache attributes
for a file (so two stats on the same file, less than a second apart,
will be benefit from the cached attributes and only require 1/2 as
many SMB3.1.1 queryinfo calls to be sent over the wire)
On Mon, Sep 2, 2024 at 7:12 AM Enzo Matsumiya <ematsumiya@xxxxxxx> wrote:
>
> 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
--
Thanks,
Steve
