diff mbox series

[RFC] smb: client: force dentry revalidation if nohandlecache is set

Message ID 20240831000937.8103-1-ematsumiya@suse.de (mailing list archive)
State New, archived
Headers show
Series [RFC] smb: client: force dentry revalidation if nohandlecache is set | expand

Commit Message

Enzo Matsumiya Aug. 31, 2024, 12:09 a.m. UTC
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@suse.de>
Tested-by: Henrique Carvalho <henrique.carvalho@suse.com>
---
 fs/smb/client/inode.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Enzo Matsumiya Aug. 31, 2024, 12:16 a.m. UTC | #1
On 08/30, Enzo Matsumiya 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.
> 
> ...
> 
>---
> 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) {

Simplest fix for a bug we hit while debugging something else.

Sending as RFC because I wasn't sure if/why attributes caching
shouldn't be in sync with handles caching.

Appreciate any thoughts, corrections, explanations.


Cheers,

Enzo
Steve French Sept. 1, 2024, 4:36 a.m. UTC | #2
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@suse.de> 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@suse.de>
> Tested-by: Henrique Carvalho <henrique.carvalho@suse.com>
> ---
>  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
>
Steve French Sept. 1, 2024, 8:15 p.m. UTC | #3
This does look wrong since it would affect use of acdirmax/acregmax.

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@gmail.com> 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@suse.de> 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@suse.de>
> > Tested-by: Henrique Carvalho <henrique.carvalho@suse.com>
> > ---
> >  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
> >
>
>
> --
> Thanks,
>
> Steve
Enzo Matsumiya Sept. 2, 2024, 12:11 p.m. UTC | #4
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@gmail.com> 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@suse.de> 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@suse.de>
>> > Tested-by: Henrique Carvalho <henrique.carvalho@suse.com>
>> > ---
>> >  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
Steve French Sept. 3, 2024, 4:58 a.m. UTC | #5
> 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@suse.de> 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@gmail.com> 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@suse.de> 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@suse.de>
> >> > Tested-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >> > ---
> >> >  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
Enzo Matsumiya Sept. 3, 2024, 3:21 p.m. UTC | #6
On 09/02, Steve French wrote:
>> 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)

Okay, thanks, I get that.

Performance is a reasonable explanation iff the files/dirs exists, which
is an assumption of cifs here.

I'm still researching ideas how to better handle this, but the fact that
dentry lookups are done by names, so -EEXIST is returned by the mkdir
syscall way before hitting any cifs code, makes really hard to come
up with an alternative fix that preserves this behaviour you mention.


Cheers,

>On Mon, Sep 2, 2024 at 7:12 AM Enzo Matsumiya <ematsumiya@suse.de> 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@gmail.com> 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@suse.de> 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@suse.de>
>> >> > Tested-by: Henrique Carvalho <henrique.carvalho@suse.com>
>> >> > ---
>> >> >  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
>
Steve French Sept. 3, 2024, 3:45 p.m. UTC | #7
And as expected, the intel bot showed performance degradation with the
RFC version of the patch (your earlier version) due to extra network
traffic (revalidate)

On Tue, Sep 3, 2024 at 10:22 AM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>
> On 09/02, Steve French wrote:
> >> 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)
>
> Okay, thanks, I get that.
>
> Performance is a reasonable explanation iff the files/dirs exists, which
> is an assumption of cifs here.
>
> I'm still researching ideas how to better handle this, but the fact that
> dentry lookups are done by names, so -EEXIST is returned by the mkdir
> syscall way before hitting any cifs code, makes really hard to come
> up with an alternative fix that preserves this behaviour you mention.
>
>
> Cheers,
>
> >On Mon, Sep 2, 2024 at 7:12 AM Enzo Matsumiya <ematsumiya@suse.de> 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@gmail.com> 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@suse.de> 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@suse.de>
> >> >> > Tested-by: Henrique Carvalho <henrique.carvalho@suse.com>
> >> >> > ---
> >> >> >  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
> >
diff mbox series

Patch

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) {