| Message ID | 20230217033501.2591185-2-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] cifs: Check the lease context if we actually got a lease | expand |
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 >
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
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>
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);
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(-)