* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.