* cifs: drop the lease for cached directories on rmdir and rename @ 2022-10-18 7:39 Ronnie Sahlberg 2022-10-18 7:39 ` [PATCH] cifs: drop the lease for cached directories on rmdir or rename Ronnie Sahlberg 0 siblings, 1 reply; 5+ messages in thread From: Ronnie Sahlberg @ 2022-10-18 7:39 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French Steve, This fixes early failures in the git tests on the buildbot. It was missing to drop the lease for rename and rmdir. There are still failures in the git tests but I don't think they are related to the directory caching. To test I have reverted all the recent dir caching patches from for-next but the git tests still fail. We will need to git bisect and find what broke these tests. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] cifs: drop the lease for cached directories on rmdir or rename 2022-10-18 7:39 cifs: drop the lease for cached directories on rmdir and rename Ronnie Sahlberg @ 2022-10-18 7:39 ` Ronnie Sahlberg 2022-10-18 14:22 ` Tom Talpey 0 siblings, 1 reply; 5+ messages in thread From: Ronnie Sahlberg @ 2022-10-18 7:39 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French, Ronnie Sahlberg When we delete or rename a directory we must also drop any cached lease we have on the directory. Fixes: a350d6e73f5e ("cifs: enable caching of directories for which a lease is held") Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cached_dir.c | 21 +++++++++++++++++++++ fs/cifs/cached_dir.h | 4 ++++ fs/cifs/smb2inode.c | 2 ++ 3 files changed, 27 insertions(+) diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c index ffc924296e59..6e689c4c8d1b 100644 --- a/fs/cifs/cached_dir.c +++ b/fs/cifs/cached_dir.c @@ -340,6 +340,27 @@ smb2_close_cached_fid(struct kref *ref) free_cached_dir(cfid); } +void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon, + const char *name, struct cifs_sb_info *cifs_sb) +{ + struct cached_fid *cfid = NULL; + int rc; + + rc = open_cached_dir(xid, tcon, name, cifs_sb, true, &cfid); + if (rc) { + cifs_dbg(FYI, "no cached dir found for rmdir(%s)\n", name); + return; + } + spin_lock(&cfid->cfids->cfid_list_lock); + if (cfid->has_lease) { + cfid->has_lease = false; + kref_put(&cfid->refcount, smb2_close_cached_fid); + } + spin_unlock(&cfid->cfids->cfid_list_lock); + close_cached_dir(cfid); +} + + void close_cached_dir(struct cached_fid *cfid) { kref_put(&cfid->refcount, smb2_close_cached_fid); diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h index e536304ca2ce..2f4e764c9ca9 100644 --- a/fs/cifs/cached_dir.h +++ b/fs/cifs/cached_dir.h @@ -69,6 +69,10 @@ extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon, struct dentry *dentry, struct cached_fid **cfid); extern void close_cached_dir(struct cached_fid *cfid); +extern void drop_cached_dir_by_name(const unsigned int xid, + struct cifs_tcon *tcon, + const char *name, + struct cifs_sb_info *cifs_sb); extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb); extern void invalidate_all_cached_dirs(struct cifs_tcon *tcon); extern int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16]); diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c index a6640e6ea58b..68e08c85fbb8 100644 --- a/fs/cifs/smb2inode.c +++ b/fs/cifs/smb2inode.c @@ -655,6 +655,7 @@ int smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name, struct cifs_sb_info *cifs_sb) { + drop_cached_dir_by_name(xid, tcon, name, cifs_sb); return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN, CREATE_NOT_FILE, ACL_NO_MODE, NULL, SMB2_OP_RMDIR, NULL, NULL, NULL); @@ -698,6 +699,7 @@ smb2_rename_path(const unsigned int xid, struct cifs_tcon *tcon, { struct cifsFileInfo *cfile; + drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb); cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile); return smb2_set_path_attr(xid, tcon, from_name, to_name, -- 2.35.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: drop the lease for cached directories on rmdir or rename 2022-10-18 7:39 ` [PATCH] cifs: drop the lease for cached directories on rmdir or rename Ronnie Sahlberg @ 2022-10-18 14:22 ` Tom Talpey 2022-10-18 23:47 ` ronnie sahlberg 0 siblings, 1 reply; 5+ messages in thread From: Tom Talpey @ 2022-10-18 14:22 UTC (permalink / raw) To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French On 10/18/2022 3:39 AM, Ronnie Sahlberg wrote: > When we delete or rename a directory we must also drop any cached lease we have > on the directory. Just curious, why drop the lease on rename? I guess this is related to setting ReplaceIfExists, but that would apply to a lease on the existing (replaced) directory, which would then become deleted? I'm probably undercaffeinated, if not. Tom. > Fixes: a350d6e73f5e ("cifs: enable caching of directories for which a lease > is held") > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cached_dir.c | 21 +++++++++++++++++++++ > fs/cifs/cached_dir.h | 4 ++++ > fs/cifs/smb2inode.c | 2 ++ > 3 files changed, 27 insertions(+) > > diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c > index ffc924296e59..6e689c4c8d1b 100644 > --- a/fs/cifs/cached_dir.c > +++ b/fs/cifs/cached_dir.c > @@ -340,6 +340,27 @@ smb2_close_cached_fid(struct kref *ref) > free_cached_dir(cfid); > } > > +void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon, > + const char *name, struct cifs_sb_info *cifs_sb) > +{ > + struct cached_fid *cfid = NULL; > + int rc; > + > + rc = open_cached_dir(xid, tcon, name, cifs_sb, true, &cfid); > + if (rc) { > + cifs_dbg(FYI, "no cached dir found for rmdir(%s)\n", name); > + return; > + } > + spin_lock(&cfid->cfids->cfid_list_lock); > + if (cfid->has_lease) { > + cfid->has_lease = false; > + kref_put(&cfid->refcount, smb2_close_cached_fid); > + } > + spin_unlock(&cfid->cfids->cfid_list_lock); > + close_cached_dir(cfid); > +} > + > + > void close_cached_dir(struct cached_fid *cfid) > { > kref_put(&cfid->refcount, smb2_close_cached_fid); > diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h > index e536304ca2ce..2f4e764c9ca9 100644 > --- a/fs/cifs/cached_dir.h > +++ b/fs/cifs/cached_dir.h > @@ -69,6 +69,10 @@ extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon, > struct dentry *dentry, > struct cached_fid **cfid); > extern void close_cached_dir(struct cached_fid *cfid); > +extern void drop_cached_dir_by_name(const unsigned int xid, > + struct cifs_tcon *tcon, > + const char *name, > + struct cifs_sb_info *cifs_sb); > extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb); > extern void invalidate_all_cached_dirs(struct cifs_tcon *tcon); > extern int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16]); > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > index a6640e6ea58b..68e08c85fbb8 100644 > --- a/fs/cifs/smb2inode.c > +++ b/fs/cifs/smb2inode.c > @@ -655,6 +655,7 @@ int > smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name, > struct cifs_sb_info *cifs_sb) > { > + drop_cached_dir_by_name(xid, tcon, name, cifs_sb); > return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN, > CREATE_NOT_FILE, ACL_NO_MODE, > NULL, SMB2_OP_RMDIR, NULL, NULL, NULL); > @@ -698,6 +699,7 @@ smb2_rename_path(const unsigned int xid, struct cifs_tcon *tcon, > { > struct cifsFileInfo *cfile; > > + drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb); > cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile); > > return smb2_set_path_attr(xid, tcon, from_name, to_name, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: drop the lease for cached directories on rmdir or rename 2022-10-18 14:22 ` Tom Talpey @ 2022-10-18 23:47 ` ronnie sahlberg 2022-10-19 16:02 ` Tom Talpey 0 siblings, 1 reply; 5+ messages in thread From: ronnie sahlberg @ 2022-10-18 23:47 UTC (permalink / raw) To: Tom Talpey; +Cc: Ronnie Sahlberg, linux-cifs, Steve French On Wed, 19 Oct 2022 at 00:27, Tom Talpey <tom@talpey.com> wrote: > > On 10/18/2022 3:39 AM, Ronnie Sahlberg wrote: > > When we delete or rename a directory we must also drop any cached lease we have > > on the directory. > > Just curious, why drop the lease on rename? I guess this is related > to setting ReplaceIfExists, but that would apply to a lease on the > existing (replaced) directory, which would then become deleted? You might be right in that, but I think the lease will be broken by the rename anyway so by deliberately closing it saves half a roundtrip for the rename to complete. (you get a lease break both for the directory you rename and also for the parent directory as far as I can see in traces) > > I'm probably undercaffeinated, if not. > > Tom. > > > Fixes: a350d6e73f5e ("cifs: enable caching of directories for which a lease > > is held") > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/cached_dir.c | 21 +++++++++++++++++++++ > > fs/cifs/cached_dir.h | 4 ++++ > > fs/cifs/smb2inode.c | 2 ++ > > 3 files changed, 27 insertions(+) > > > > diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c > > index ffc924296e59..6e689c4c8d1b 100644 > > --- a/fs/cifs/cached_dir.c > > +++ b/fs/cifs/cached_dir.c > > @@ -340,6 +340,27 @@ smb2_close_cached_fid(struct kref *ref) > > free_cached_dir(cfid); > > } > > > > +void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon, > > + const char *name, struct cifs_sb_info *cifs_sb) > > +{ > > + struct cached_fid *cfid = NULL; > > + int rc; > > + > > + rc = open_cached_dir(xid, tcon, name, cifs_sb, true, &cfid); > > + if (rc) { > > + cifs_dbg(FYI, "no cached dir found for rmdir(%s)\n", name); > > + return; > > + } > > + spin_lock(&cfid->cfids->cfid_list_lock); > > + if (cfid->has_lease) { > > + cfid->has_lease = false; > > + kref_put(&cfid->refcount, smb2_close_cached_fid); > > + } > > + spin_unlock(&cfid->cfids->cfid_list_lock); > > + close_cached_dir(cfid); > > +} > > + > > + > > void close_cached_dir(struct cached_fid *cfid) > > { > > kref_put(&cfid->refcount, smb2_close_cached_fid); > > diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h > > index e536304ca2ce..2f4e764c9ca9 100644 > > --- a/fs/cifs/cached_dir.h > > +++ b/fs/cifs/cached_dir.h > > @@ -69,6 +69,10 @@ extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon, > > struct dentry *dentry, > > struct cached_fid **cfid); > > extern void close_cached_dir(struct cached_fid *cfid); > > +extern void drop_cached_dir_by_name(const unsigned int xid, > > + struct cifs_tcon *tcon, > > + const char *name, > > + struct cifs_sb_info *cifs_sb); > > extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb); > > extern void invalidate_all_cached_dirs(struct cifs_tcon *tcon); > > extern int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16]); > > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > > index a6640e6ea58b..68e08c85fbb8 100644 > > --- a/fs/cifs/smb2inode.c > > +++ b/fs/cifs/smb2inode.c > > @@ -655,6 +655,7 @@ int > > smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name, > > struct cifs_sb_info *cifs_sb) > > { > > + drop_cached_dir_by_name(xid, tcon, name, cifs_sb); > > return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN, > > CREATE_NOT_FILE, ACL_NO_MODE, > > NULL, SMB2_OP_RMDIR, NULL, NULL, NULL); > > @@ -698,6 +699,7 @@ smb2_rename_path(const unsigned int xid, struct cifs_tcon *tcon, > > { > > struct cifsFileInfo *cfile; > > > > + drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb); > > cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile); > > > > return smb2_set_path_attr(xid, tcon, from_name, to_name, ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: drop the lease for cached directories on rmdir or rename 2022-10-18 23:47 ` ronnie sahlberg @ 2022-10-19 16:02 ` Tom Talpey 0 siblings, 0 replies; 5+ messages in thread From: Tom Talpey @ 2022-10-19 16:02 UTC (permalink / raw) To: ronnie sahlberg; +Cc: Ronnie Sahlberg, linux-cifs, Steve French On 10/18/2022 7:47 PM, ronnie sahlberg wrote: > On Wed, 19 Oct 2022 at 00:27, Tom Talpey <tom@talpey.com> wrote: >> >> On 10/18/2022 3:39 AM, Ronnie Sahlberg wrote: >>> When we delete or rename a directory we must also drop any cached lease we have >>> on the directory. >> >> Just curious, why drop the lease on rename? I guess this is related >> to setting ReplaceIfExists, but that would apply to a lease on the >> existing (replaced) directory, which would then become deleted? > > You might be right in that, but I think the lease will be broken by > the rename anyway > so by deliberately closing it saves half a roundtrip for the rename to complete. > (you get a lease break both for the directory you rename and also for > the parent directory as far as I can see in traces) Do both Windows and Samba servers break the lease on the renamed directory? I'd certainly expect the parent lease to break, so that much isn't surprising. Tom. >> I'm probably undercaffeinated, if not. >> >> Tom. >> >>> Fixes: a350d6e73f5e ("cifs: enable caching of directories for which a lease >>> is held") >>> Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> >>> --- >>> fs/cifs/cached_dir.c | 21 +++++++++++++++++++++ >>> fs/cifs/cached_dir.h | 4 ++++ >>> fs/cifs/smb2inode.c | 2 ++ >>> 3 files changed, 27 insertions(+) >>> >>> diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c >>> index ffc924296e59..6e689c4c8d1b 100644 >>> --- a/fs/cifs/cached_dir.c >>> +++ b/fs/cifs/cached_dir.c >>> @@ -340,6 +340,27 @@ smb2_close_cached_fid(struct kref *ref) >>> free_cached_dir(cfid); >>> } >>> >>> +void drop_cached_dir_by_name(const unsigned int xid, struct cifs_tcon *tcon, >>> + const char *name, struct cifs_sb_info *cifs_sb) >>> +{ >>> + struct cached_fid *cfid = NULL; >>> + int rc; >>> + >>> + rc = open_cached_dir(xid, tcon, name, cifs_sb, true, &cfid); >>> + if (rc) { >>> + cifs_dbg(FYI, "no cached dir found for rmdir(%s)\n", name); >>> + return; >>> + } >>> + spin_lock(&cfid->cfids->cfid_list_lock); >>> + if (cfid->has_lease) { >>> + cfid->has_lease = false; >>> + kref_put(&cfid->refcount, smb2_close_cached_fid); >>> + } >>> + spin_unlock(&cfid->cfids->cfid_list_lock); >>> + close_cached_dir(cfid); >>> +} >>> + >>> + >>> void close_cached_dir(struct cached_fid *cfid) >>> { >>> kref_put(&cfid->refcount, smb2_close_cached_fid); >>> diff --git a/fs/cifs/cached_dir.h b/fs/cifs/cached_dir.h >>> index e536304ca2ce..2f4e764c9ca9 100644 >>> --- a/fs/cifs/cached_dir.h >>> +++ b/fs/cifs/cached_dir.h >>> @@ -69,6 +69,10 @@ extern int open_cached_dir_by_dentry(struct cifs_tcon *tcon, >>> struct dentry *dentry, >>> struct cached_fid **cfid); >>> extern void close_cached_dir(struct cached_fid *cfid); >>> +extern void drop_cached_dir_by_name(const unsigned int xid, >>> + struct cifs_tcon *tcon, >>> + const char *name, >>> + struct cifs_sb_info *cifs_sb); >>> extern void close_all_cached_dirs(struct cifs_sb_info *cifs_sb); >>> extern void invalidate_all_cached_dirs(struct cifs_tcon *tcon); >>> extern int cached_dir_lease_break(struct cifs_tcon *tcon, __u8 lease_key[16]); >>> diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c >>> index a6640e6ea58b..68e08c85fbb8 100644 >>> --- a/fs/cifs/smb2inode.c >>> +++ b/fs/cifs/smb2inode.c >>> @@ -655,6 +655,7 @@ int >>> smb2_rmdir(const unsigned int xid, struct cifs_tcon *tcon, const char *name, >>> struct cifs_sb_info *cifs_sb) >>> { >>> + drop_cached_dir_by_name(xid, tcon, name, cifs_sb); >>> return smb2_compound_op(xid, tcon, cifs_sb, name, DELETE, FILE_OPEN, >>> CREATE_NOT_FILE, ACL_NO_MODE, >>> NULL, SMB2_OP_RMDIR, NULL, NULL, NULL); >>> @@ -698,6 +699,7 @@ smb2_rename_path(const unsigned int xid, struct cifs_tcon *tcon, >>> { >>> struct cifsFileInfo *cfile; >>> >>> + drop_cached_dir_by_name(xid, tcon, from_name, cifs_sb); >>> cifs_get_writable_path(tcon, from_name, FIND_WR_WITH_DELETE, &cfile); >>> >>> return smb2_set_path_attr(xid, tcon, from_name, to_name, > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-10-19 16:04 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-18 7:39 cifs: drop the lease for cached directories on rmdir and rename Ronnie Sahlberg 2022-10-18 7:39 ` [PATCH] cifs: drop the lease for cached directories on rmdir or rename Ronnie Sahlberg 2022-10-18 14:22 ` Tom Talpey 2022-10-18 23:47 ` ronnie sahlberg 2022-10-19 16:02 ` Tom Talpey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox