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