| Message ID | 20221018073910.1732992-2-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: drop the lease for cached directories on rmdir or rename | expand |
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,
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,
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, >
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,
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(+)