* [PATCH 1/2] cifs: Check the lease context if we actually got a lease @ 2023-02-17 3:35 Ronnie Sahlberg 2023-02-17 3:35 ` [PATCH 2/2] cifs: return a single-use cfid if we did not get " Ronnie Sahlberg 2023-02-17 13:37 ` [PATCH 1/2] cifs: Check the lease context if we actually got " Paulo Alcantara 0 siblings, 2 replies; 6+ messages in thread From: Ronnie Sahlberg @ 2023-02-17 3:35 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French Some servers may return that we got a lease in rsp->OplockLevel but then in the lease context contradict this and say we got no lease at all. Thus we need to check the context if we have a lease. Additionally, IF we do not get a lease we need to make sure we close the handle before we return an error to the caller. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cached_dir.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c index 60399081046a..6672f1a0acd7 100644 --- a/fs/cifs/cached_dir.c +++ b/fs/cifs/cached_dir.c @@ -220,8 +220,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, } goto oshr_free; } - - atomic_inc(&tcon->num_remote_opens); + cfid->is_open = true; o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; oparms.fid->persistent_fid = o_rsp->PersistentFileId; @@ -238,7 +237,8 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, &oparms.fid->epoch, oparms.fid->lease_key, &oplock, NULL, NULL); - + if (!(oplock & SMB2_LEASE_READ_CACHING_HE)) + goto oshr_free; qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; if (le32_to_cpu(qi_rsp->OutputBufferLength) < sizeof(struct smb2_file_all_info)) goto oshr_free; @@ -261,7 +261,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, cfid->dentry = dentry; cfid->tcon = tcon; cfid->time = jiffies; - cfid->is_open = true; cfid->has_lease = true; oshr_free: @@ -281,12 +280,17 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, } spin_unlock(&cfids->cfid_list_lock); if (rc) { + if (cfid->is_open) + SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid, + cfid->fid.volatile_fid); free_cached_dir(cfid); cfid = NULL; } - if (rc == 0) + if (rc == 0) { *ret_cfid = cfid; + atomic_inc(&tcon->num_remote_opens); + } return rc; } -- 2.35.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] cifs: return a single-use cfid if we did not get a lease 2023-02-17 3:35 [PATCH 1/2] cifs: Check the lease context if we actually got a lease Ronnie Sahlberg @ 2023-02-17 3:35 ` Ronnie Sahlberg 2023-02-17 5:40 ` Steve French 2023-02-17 13:38 ` Paulo Alcantara 2023-02-17 13:37 ` [PATCH 1/2] cifs: Check the lease context if we actually got " Paulo Alcantara 1 sibling, 2 replies; 6+ messages in thread From: Ronnie Sahlberg @ 2023-02-17 3:35 UTC (permalink / raw) To: linux-cifs; +Cc: Steve French If we did not get a lease we can still return a single use cfid to the caller. The cfid will not have has_lease set and will thus not be shared with any other concurrent users and will be freed immediately when the caller drops the handle. This avoids extra roundtrips for servers that do not support directory leases where they would first fail to get a cfid with a lease and then fallback to try a normal SMB2_open() Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cached_dir.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c index 6672f1a0acd7..27ae908e4903 100644 --- a/fs/cifs/cached_dir.c +++ b/fs/cifs/cached_dir.c @@ -14,6 +14,7 @@ static struct cached_fid *init_cached_dir(const char *path); static void free_cached_dir(struct cached_fid *cfid); +static void smb2_close_cached_fid(struct kref *ref); static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids, const char *path, @@ -220,6 +221,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, } goto oshr_free; } + cfid->tcon = tcon; cfid->is_open = true; o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; @@ -232,7 +234,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) goto oshr_free; - smb2_parse_contexts(server, o_rsp, &oparms.fid->epoch, oparms.fid->lease_key, &oplock, @@ -259,7 +260,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, } } cfid->dentry = dentry; - cfid->tcon = tcon; cfid->time = jiffies; cfid->has_lease = true; @@ -270,7 +270,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); spin_lock(&cfids->cfid_list_lock); - if (!cfid->has_lease) { + if (rc && !cfid->has_lease) { if (cfid->on_list) { list_del(&cfid->entry); cfid->on_list = false; @@ -279,6 +279,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, rc = -ENOENT; } spin_unlock(&cfids->cfid_list_lock); + if (!rc && !cfid->has_lease) { + /* + * We are guaranteed to have two references at this point. + * One for the caller and one for a potential lease. + * Release the Lease-ref so that the directory will be closed + * when the caller closes the cached handle. + */ + kref_put(&cfid->refcount, smb2_close_cached_fid); + } if (rc) { if (cfid->is_open) SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid, @@ -339,6 +348,7 @@ smb2_close_cached_fid(struct kref *ref) if (cfid->is_open) { SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid, cfid->fid.volatile_fid); + atomic_dec(&cfid->tcon->num_remote_opens); } free_cached_dir(cfid); -- 2.35.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] cifs: return a single-use cfid if we did not get a lease 2023-02-17 3:35 ` [PATCH 2/2] cifs: return a single-use cfid if we did not get " Ronnie Sahlberg @ 2023-02-17 5:40 ` Steve French 2023-02-17 5:47 ` ronnie sahlberg 2023-02-17 13:38 ` Paulo Alcantara 1 sibling, 1 reply; 6+ messages in thread From: Steve French @ 2023-02-17 5:40 UTC (permalink / raw) To: Ronnie Sahlberg; +Cc: linux-cifs, Bharath S M Added Reviewed-by Bharath and merged into cifs-2.6.git for-next On Thu, Feb 16, 2023 at 9:35 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > If we did not get a lease we can still return a single use cfid to the caller. > The cfid will not have has_lease set and will thus not be shared with any > other concurrent users and will be freed immediately when the caller > drops the handle. > > This avoids extra roundtrips for servers that do not support directory leases > where they would first fail to get a cfid with a lease and then fallback > to try a normal SMB2_open() > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cached_dir.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) > > diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c > index 6672f1a0acd7..27ae908e4903 100644 > --- a/fs/cifs/cached_dir.c > +++ b/fs/cifs/cached_dir.c > @@ -14,6 +14,7 @@ > > static struct cached_fid *init_cached_dir(const char *path); > static void free_cached_dir(struct cached_fid *cfid); > +static void smb2_close_cached_fid(struct kref *ref); > > static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids, > const char *path, > @@ -220,6 +221,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > } > goto oshr_free; > } > + cfid->tcon = tcon; > cfid->is_open = true; > > o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > @@ -232,7 +234,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) > goto oshr_free; > > - > smb2_parse_contexts(server, o_rsp, > &oparms.fid->epoch, > oparms.fid->lease_key, &oplock, > @@ -259,7 +260,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > } > } > cfid->dentry = dentry; > - cfid->tcon = tcon; > cfid->time = jiffies; > cfid->has_lease = true; > > @@ -270,7 +270,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); > free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); > spin_lock(&cfids->cfid_list_lock); > - if (!cfid->has_lease) { > + if (rc && !cfid->has_lease) { > if (cfid->on_list) { > list_del(&cfid->entry); > cfid->on_list = false; > @@ -279,6 +279,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > rc = -ENOENT; > } > spin_unlock(&cfids->cfid_list_lock); > + if (!rc && !cfid->has_lease) { > + /* > + * We are guaranteed to have two references at this point. > + * One for the caller and one for a potential lease. > + * Release the Lease-ref so that the directory will be closed > + * when the caller closes the cached handle. > + */ > + kref_put(&cfid->refcount, smb2_close_cached_fid); > + } > if (rc) { > if (cfid->is_open) > SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid, > @@ -339,6 +348,7 @@ smb2_close_cached_fid(struct kref *ref) > if (cfid->is_open) { > SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid, > cfid->fid.volatile_fid); > + atomic_dec(&cfid->tcon->num_remote_opens); > } > > free_cached_dir(cfid); > -- > 2.35.3 > -- Thanks, Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] cifs: return a single-use cfid if we did not get a lease 2023-02-17 5:40 ` Steve French @ 2023-02-17 5:47 ` ronnie sahlberg 0 siblings, 0 replies; 6+ messages in thread From: ronnie sahlberg @ 2023-02-17 5:47 UTC (permalink / raw) To: Steve French; +Cc: Ronnie Sahlberg, linux-cifs, Bharath S M At a later time we can make additional changes so that we ALWAYS return a single-use cfid to all callers IFF we could SMB2_open() the file for the cfid. Right now any concurrent opens on a cfid for a path that does not have cfid->has_lease will return NULL and have the application retry a normal SMB2_open. We can change this further to also return single-use cfids for concurrent use and this would have the benefit that we can avoid having to do the "try getting a cfid if it fails try a normal smb2_open conditional" in the callers. It would simplify the code in the callers. It is not an urgent cleanup but something we should do in the future. On Fri, 17 Feb 2023 at 15:41, Steve French <smfrench@gmail.com> wrote: > > Added Reviewed-by Bharath and merged into cifs-2.6.git for-next > > On Thu, Feb 16, 2023 at 9:35 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > If we did not get a lease we can still return a single use cfid to the caller. > > The cfid will not have has_lease set and will thus not be shared with any > > other concurrent users and will be freed immediately when the caller > > drops the handle. > > > > This avoids extra roundtrips for servers that do not support directory leases > > where they would first fail to get a cfid with a lease and then fallback > > to try a normal SMB2_open() > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/cached_dir.c | 16 +++++++++++++--- > > 1 file changed, 13 insertions(+), 3 deletions(-) > > > > diff --git a/fs/cifs/cached_dir.c b/fs/cifs/cached_dir.c > > index 6672f1a0acd7..27ae908e4903 100644 > > --- a/fs/cifs/cached_dir.c > > +++ b/fs/cifs/cached_dir.c > > @@ -14,6 +14,7 @@ > > > > static struct cached_fid *init_cached_dir(const char *path); > > static void free_cached_dir(struct cached_fid *cfid); > > +static void smb2_close_cached_fid(struct kref *ref); > > > > static struct cached_fid *find_or_create_cached_dir(struct cached_fids *cfids, > > const char *path, > > @@ -220,6 +221,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > > } > > goto oshr_free; > > } > > + cfid->tcon = tcon; > > cfid->is_open = true; > > > > o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > > @@ -232,7 +234,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > > if (o_rsp->OplockLevel != SMB2_OPLOCK_LEVEL_LEASE) > > goto oshr_free; > > > > - > > smb2_parse_contexts(server, o_rsp, > > &oparms.fid->epoch, > > oparms.fid->lease_key, &oplock, > > @@ -259,7 +260,6 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > > } > > } > > cfid->dentry = dentry; > > - cfid->tcon = tcon; > > cfid->time = jiffies; > > cfid->has_lease = true; > > > > @@ -270,7 +270,7 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > > free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); > > free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); > > spin_lock(&cfids->cfid_list_lock); > > - if (!cfid->has_lease) { > > + if (rc && !cfid->has_lease) { > > if (cfid->on_list) { > > list_del(&cfid->entry); > > cfid->on_list = false; > > @@ -279,6 +279,15 @@ int open_cached_dir(unsigned int xid, struct cifs_tcon *tcon, > > rc = -ENOENT; > > } > > spin_unlock(&cfids->cfid_list_lock); > > + if (!rc && !cfid->has_lease) { > > + /* > > + * We are guaranteed to have two references at this point. > > + * One for the caller and one for a potential lease. > > + * Release the Lease-ref so that the directory will be closed > > + * when the caller closes the cached handle. > > + */ > > + kref_put(&cfid->refcount, smb2_close_cached_fid); > > + } > > if (rc) { > > if (cfid->is_open) > > SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid, > > @@ -339,6 +348,7 @@ smb2_close_cached_fid(struct kref *ref) > > if (cfid->is_open) { > > SMB2_close(0, cfid->tcon, cfid->fid.persistent_fid, > > cfid->fid.volatile_fid); > > + atomic_dec(&cfid->tcon->num_remote_opens); > > } > > > > free_cached_dir(cfid); > > -- > > 2.35.3 > > > > > -- > Thanks, > > Steve ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] cifs: return a single-use cfid if we did not get a lease 2023-02-17 3:35 ` [PATCH 2/2] cifs: return a single-use cfid if we did not get " Ronnie Sahlberg 2023-02-17 5:40 ` Steve French @ 2023-02-17 13:38 ` Paulo Alcantara 1 sibling, 0 replies; 6+ messages in thread From: Paulo Alcantara @ 2023-02-17 13:38 UTC (permalink / raw) To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French Ronnie Sahlberg <lsahlber@redhat.com> writes: > If we did not get a lease we can still return a single use cfid to the caller. > The cfid will not have has_lease set and will thus not be shared with any > other concurrent users and will be freed immediately when the caller > drops the handle. > > This avoids extra roundtrips for servers that do not support directory leases > where they would first fail to get a cfid with a lease and then fallback > to try a normal SMB2_open() > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cached_dir.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-) Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] cifs: Check the lease context if we actually got a lease 2023-02-17 3:35 [PATCH 1/2] cifs: Check the lease context if we actually got a lease Ronnie Sahlberg 2023-02-17 3:35 ` [PATCH 2/2] cifs: return a single-use cfid if we did not get " Ronnie Sahlberg @ 2023-02-17 13:37 ` Paulo Alcantara 1 sibling, 0 replies; 6+ messages in thread From: Paulo Alcantara @ 2023-02-17 13:37 UTC (permalink / raw) To: Ronnie Sahlberg, linux-cifs; +Cc: Steve French Ronnie Sahlberg <lsahlber@redhat.com> writes: > Some servers may return that we got a lease in rsp->OplockLevel > but then in the lease context contradict this and say we got no lease > at all. Thus we need to check the context if we have a lease. > Additionally, IF we do not get a lease we need to make sure we close > the handle before we return an error to the caller. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cached_dir.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-02-17 13:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-02-17 3:35 [PATCH 1/2] cifs: Check the lease context if we actually got a lease Ronnie Sahlberg 2023-02-17 3:35 ` [PATCH 2/2] cifs: return a single-use cfid if we did not get " Ronnie Sahlberg 2023-02-17 5:40 ` Steve French 2023-02-17 5:47 ` ronnie sahlberg 2023-02-17 13:38 ` Paulo Alcantara 2023-02-17 13:37 ` [PATCH 1/2] cifs: Check the lease context if we actually got " Paulo Alcantara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox