| Message ID | 20190311060025.9750-2-lsahlber@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: cache FILE_ALL_INFO for the shared root handle | expand |
As a followup discussion. We do tracing in SMB2_open() but not in SMB2_open_init(). Perhaps we should change this to do the actual tracing in SMB2_open_init() instead and catch ALL places where we try to open a file. Though, we don't actually have the result of the operation in _open_init() which means we may need to reconsider how we trace these things. As we will likely switch more and more things to be compounds, maybe we should not have tracing in SMB2_open() and have it trace trace_smb3_open_err() Maybe we should trace "smb2_init : constructing compound" and have trace_smb3_compound_error() We restructure the tracing to be focused around compounds instead of individual PDUs. I.e. tracing in all the _init() functions, then tracing in compound_send_recv() for success/error. Tracing success/error on pdu/compound level instead of individual open/close/read/... operations. For individual non-compounded commands we can still treat these the same. They are just trivial compounds of one single SMB2 PDU. On Mon, Mar 11, 2019 at 4:01 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > When we open the shared root handle also ask for FILE_ALL_INFORMATION since > we can do this at zero cost as part of a compound. > Cache this information as long as the lease is return and serve any > future requests from cache. > > This allows us to serve "stat /<mountpoint>" directly from cache and avoid > a network roundtrip. Since clients ofthen need to do this quite a lot > this improve performance slightly. > > As an example: xfstest generic/533 performs 43 stat operations on the root > of the share while it is run. Which are eliminated with this patch. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cifsglob.h | 3 ++ > fs/cifs/smb2inode.c | 15 ++++--- > fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++--------- > fs/cifs/smb2pdu.c | 12 +++--- > fs/cifs/smb2proto.h | 3 ++ > 5 files changed, 116 insertions(+), 28 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index f293e052e351..b8360ca221eb 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses) > > struct cached_fid { > bool is_valid:1; /* Do we have a useable root fid */ > + bool file_all_info_is_valid:1; > + > struct kref refcount; > struct cifs_fid *fid; > struct mutex fid_mutex; > struct cifs_tcon *tcon; > struct work_struct lease_break; > + struct smb2_file_all_info file_all_info; > }; > > /* > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > index 01a76bccdb8d..b6e07e2eed10 100644 > --- a/fs/cifs/smb2inode.c > +++ b/fs/cifs/smb2inode.c > @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > rc = open_shroot(xid, tcon, &fid); > if (rc) > goto out; > - rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > - fid.volatile_fid, smb2_data); > + > + if (tcon->crfid.file_all_info_is_valid) { > + move_smb2_info_to_cifs(data, > + &tcon->crfid.file_all_info); > + } else { > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > + fid.volatile_fid, smb2_data); > + if (!rc) > + move_smb2_info_to_cifs(data, smb2_data); > + } > close_shroot(&tcon->crfid); > - if (rc) > - goto out; > - move_smb2_info_to_cifs(data, smb2_data); > goto out; > } > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 085e91436da7..0d8bf87592ff 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref) > SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid, > cfid->fid->volatile_fid); > cfid->is_valid = false; > + cfid->file_all_info_is_valid = false; > } > } > > @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work) > */ > int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > { > - struct cifs_open_parms oparams; > - int rc; > - __le16 srch_path = 0; /* Null - since an open of top of share */ > + struct cifs_ses *ses = tcon->ses; > + struct TCP_Server_Info *server = ses->server; > + struct cifs_open_parms oparms; > + struct smb2_create_rsp *o_rsp = NULL; > + struct smb2_query_info_rsp *qi_rsp = NULL; > + int resp_buftype[2]; > + struct smb_rqst rqst[2]; > + struct kvec rsp_iov[2]; > + struct kvec open_iov[SMB2_CREATE_IOV_SIZE]; > + struct kvec qi_iov[1]; > + int rc, flags = 0; > + __le16 utf16_path = 0; /* Null - since an open of top of share */ > u8 oplock = SMB2_OPLOCK_LEVEL_II; > > mutex_lock(&tcon->crfid.fid_mutex); > @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > return 0; > } > > - oparams.tcon = tcon; > - oparams.create_options = 0; > - oparams.desired_access = FILE_READ_ATTRIBUTES; > - oparams.disposition = FILE_OPEN; > - oparams.fid = pfid; > - oparams.reconnect = false; > - > - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL); > - if (rc == 0) { > - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > - tcon->crfid.tcon = tcon; > - tcon->crfid.is_valid = true; > - kref_init(&tcon->crfid.refcount); > - kref_get(&tcon->crfid.refcount); > - } > + if (smb3_encryption_required(tcon)) > + flags |= CIFS_TRANSFORM_REQ; > + > + memset(rqst, 0, sizeof(rqst)); > + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER; > + memset(rsp_iov, 0, sizeof(rsp_iov)); > + > + /* Open */ > + memset(&open_iov, 0, sizeof(open_iov)); > + rqst[0].rq_iov = open_iov; > + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE; > + > + oparms.tcon = tcon; > + oparms.create_options = 0; > + oparms.desired_access = FILE_READ_ATTRIBUTES; > + oparms.disposition = FILE_OPEN; > + oparms.fid = pfid; > + oparms.reconnect = false; > + > + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path); > + if (rc) > + goto oshr_exit; > + smb2_set_next_command(tcon, &rqst[0]); > + > + memset(&qi_iov, 0, sizeof(qi_iov)); > + rqst[1].rq_iov = qi_iov; > + rqst[1].rq_nvec = 1; > + > + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID, > + COMPOUND_FID, FILE_ALL_INFORMATION, > + SMB2_O_INFO_FILE, 0, > + sizeof(struct smb2_file_all_info) + > + PATH_MAX * 2, 0, NULL); > + if (rc) > + goto oshr_exit; > + > + smb2_set_related(&rqst[1]); > + > + rc = compound_send_recv(xid, ses, flags, 2, rqst, > + resp_buftype, rsp_iov); > + if (rc) > + goto oshr_exit; > + > + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > + oparms.fid->persistent_fid = o_rsp->PersistentFileId; > + oparms.fid->volatile_fid = o_rsp->VolatileFileId; > +#ifdef CONFIG_CIFS_DEBUG2 > + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); > +#endif /* CIFS_DEBUG2 */ > + > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > + oplock = smb2_parse_lease_state(server, o_rsp, > + &oparms.fid->epoch, > + oparms.fid->lease_key); > + else > + goto oshr_exit; > + > + > + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > + tcon->crfid.tcon = tcon; > + tcon->crfid.is_valid = true; > + kref_init(&tcon->crfid.refcount); > + kref_get(&tcon->crfid.refcount); > + > + > + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; > + rc = smb2_validate_and_copy_iov( > + le16_to_cpu(qi_rsp->OutputBufferOffset), > + le32_to_cpu(qi_rsp->OutputBufferLength), > + &rsp_iov[1], sizeof(struct smb2_file_all_info), > + (char *)&tcon->crfid.file_all_info); > + if (rc) > + goto oshr_exit; > + tcon->crfid.file_all_info_is_valid = 1; > + > + oshr_exit: > mutex_unlock(&tcon->crfid.fid_mutex); > + SMB2_open_free(&rqst[0]); > + SMB2_query_info_free(&rqst[1]); > + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); > + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); > return rc; > } > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 60fbe306f604..cfe9fe41ccf5 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid) > return buf; > } > > -static __u8 > -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp, > - unsigned int *epoch, char *lease_key) > +__u8 > +smb2_parse_lease_state(struct TCP_Server_Info *server, > + struct smb2_create_rsp *rsp, > + unsigned int *epoch, char *lease_key) > { > char *data_offset; > struct create_context *cc; > @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path, > } > > if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch, > - oparms->fid->lease_key); > + *oplock = smb2_parse_lease_state(server, rsp, > + &oparms->fid->epoch, > + oparms->fid->lease_key); > else > *oplock = rsp->OplockLevel; > creat_exit: > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > index 87733b27a65f..72cc563c32fe 100644 > --- a/fs/cifs/smb2proto.h > +++ b/fs/cifs/smb2proto.h > @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); > > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, > enum securityEnum); > +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server, > + struct smb2_create_rsp *rsp, > + unsigned int *epoch, char *lease_key); > extern int smb3_encryption_required(const struct cifs_tcon *tcon); > extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length, > struct kvec *iov, unsigned int min_buf_size); > -- > 2.13.6 >
Pavel and I had talked about adding tracing in smb2_compound_op for enter/exit since it allows more granularity (and with dynamic tracing, overhead is small). Thoughts? On Mon, Mar 11, 2019 at 7:02 AM ronnie sahlberg <ronniesahlberg@gmail.com> wrote: > > As a followup discussion. > > We do tracing in SMB2_open() but not in SMB2_open_init(). Perhaps we > should change this to do the actual tracing in SMB2_open_init() > instead and catch ALL places where we try to open a file. > Though, we don't actually have the result of the operation in > _open_init() which means we may need to reconsider how we trace these > things. > > As we will likely switch more and more things to be compounds, maybe > we should not have tracing in SMB2_open() and have it trace > trace_smb3_open_err() > > Maybe we should trace "smb2_init : constructing compound" > and have trace_smb3_compound_error() > > > We restructure the tracing to be focused around compounds instead of > individual PDUs. > I.e. tracing in all the _init() functions, then tracing in > compound_send_recv() for success/error. > > Tracing success/error on pdu/compound level instead of individual > open/close/read/... operations. > For individual non-compounded commands we can still treat these the > same. They are just trivial compounds of one single SMB2 PDU. > > > On Mon, Mar 11, 2019 at 4:01 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > When we open the shared root handle also ask for FILE_ALL_INFORMATION since > > we can do this at zero cost as part of a compound. > > Cache this information as long as the lease is return and serve any > > future requests from cache. > > > > This allows us to serve "stat /<mountpoint>" directly from cache and avoid > > a network roundtrip. Since clients ofthen need to do this quite a lot > > this improve performance slightly. > > > > As an example: xfstest generic/533 performs 43 stat operations on the root > > of the share while it is run. Which are eliminated with this patch. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/cifsglob.h | 3 ++ > > fs/cifs/smb2inode.c | 15 ++++--- > > fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++--------- > > fs/cifs/smb2pdu.c | 12 +++--- > > fs/cifs/smb2proto.h | 3 ++ > > 5 files changed, 116 insertions(+), 28 deletions(-) > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > index f293e052e351..b8360ca221eb 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses) > > > > struct cached_fid { > > bool is_valid:1; /* Do we have a useable root fid */ > > + bool file_all_info_is_valid:1; > > + > > struct kref refcount; > > struct cifs_fid *fid; > > struct mutex fid_mutex; > > struct cifs_tcon *tcon; > > struct work_struct lease_break; > > + struct smb2_file_all_info file_all_info; > > }; > > > > /* > > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > > index 01a76bccdb8d..b6e07e2eed10 100644 > > --- a/fs/cifs/smb2inode.c > > +++ b/fs/cifs/smb2inode.c > > @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > > rc = open_shroot(xid, tcon, &fid); > > if (rc) > > goto out; > > - rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > > - fid.volatile_fid, smb2_data); > > + > > + if (tcon->crfid.file_all_info_is_valid) { > > + move_smb2_info_to_cifs(data, > > + &tcon->crfid.file_all_info); > > + } else { > > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > > + fid.volatile_fid, smb2_data); > > + if (!rc) > > + move_smb2_info_to_cifs(data, smb2_data); > > + } > > close_shroot(&tcon->crfid); > > - if (rc) > > - goto out; > > - move_smb2_info_to_cifs(data, smb2_data); > > goto out; > > } > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > index 085e91436da7..0d8bf87592ff 100644 > > --- a/fs/cifs/smb2ops.c > > +++ b/fs/cifs/smb2ops.c > > @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref) > > SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid, > > cfid->fid->volatile_fid); > > cfid->is_valid = false; > > + cfid->file_all_info_is_valid = false; > > } > > } > > > > @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work) > > */ > > int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > > { > > - struct cifs_open_parms oparams; > > - int rc; > > - __le16 srch_path = 0; /* Null - since an open of top of share */ > > + struct cifs_ses *ses = tcon->ses; > > + struct TCP_Server_Info *server = ses->server; > > + struct cifs_open_parms oparms; > > + struct smb2_create_rsp *o_rsp = NULL; > > + struct smb2_query_info_rsp *qi_rsp = NULL; > > + int resp_buftype[2]; > > + struct smb_rqst rqst[2]; > > + struct kvec rsp_iov[2]; > > + struct kvec open_iov[SMB2_CREATE_IOV_SIZE]; > > + struct kvec qi_iov[1]; > > + int rc, flags = 0; > > + __le16 utf16_path = 0; /* Null - since an open of top of share */ > > u8 oplock = SMB2_OPLOCK_LEVEL_II; > > > > mutex_lock(&tcon->crfid.fid_mutex); > > @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > > return 0; > > } > > > > - oparams.tcon = tcon; > > - oparams.create_options = 0; > > - oparams.desired_access = FILE_READ_ATTRIBUTES; > > - oparams.disposition = FILE_OPEN; > > - oparams.fid = pfid; > > - oparams.reconnect = false; > > - > > - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL); > > - if (rc == 0) { > > - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > > - tcon->crfid.tcon = tcon; > > - tcon->crfid.is_valid = true; > > - kref_init(&tcon->crfid.refcount); > > - kref_get(&tcon->crfid.refcount); > > - } > > + if (smb3_encryption_required(tcon)) > > + flags |= CIFS_TRANSFORM_REQ; > > + > > + memset(rqst, 0, sizeof(rqst)); > > + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER; > > + memset(rsp_iov, 0, sizeof(rsp_iov)); > > + > > + /* Open */ > > + memset(&open_iov, 0, sizeof(open_iov)); > > + rqst[0].rq_iov = open_iov; > > + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE; > > + > > + oparms.tcon = tcon; > > + oparms.create_options = 0; > > + oparms.desired_access = FILE_READ_ATTRIBUTES; > > + oparms.disposition = FILE_OPEN; > > + oparms.fid = pfid; > > + oparms.reconnect = false; > > + > > + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path); > > + if (rc) > > + goto oshr_exit; > > + smb2_set_next_command(tcon, &rqst[0]); > > + > > + memset(&qi_iov, 0, sizeof(qi_iov)); > > + rqst[1].rq_iov = qi_iov; > > + rqst[1].rq_nvec = 1; > > + > > + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID, > > + COMPOUND_FID, FILE_ALL_INFORMATION, > > + SMB2_O_INFO_FILE, 0, > > + sizeof(struct smb2_file_all_info) + > > + PATH_MAX * 2, 0, NULL); > > + if (rc) > > + goto oshr_exit; > > + > > + smb2_set_related(&rqst[1]); > > + > > + rc = compound_send_recv(xid, ses, flags, 2, rqst, > > + resp_buftype, rsp_iov); > > + if (rc) > > + goto oshr_exit; > > + > > + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > > + oparms.fid->persistent_fid = o_rsp->PersistentFileId; > > + oparms.fid->volatile_fid = o_rsp->VolatileFileId; > > +#ifdef CONFIG_CIFS_DEBUG2 > > + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); > > +#endif /* CIFS_DEBUG2 */ > > + > > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > > + oplock = smb2_parse_lease_state(server, o_rsp, > > + &oparms.fid->epoch, > > + oparms.fid->lease_key); > > + else > > + goto oshr_exit; > > + > > + > > + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > > + tcon->crfid.tcon = tcon; > > + tcon->crfid.is_valid = true; > > + kref_init(&tcon->crfid.refcount); > > + kref_get(&tcon->crfid.refcount); > > + > > + > > + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; > > + rc = smb2_validate_and_copy_iov( > > + le16_to_cpu(qi_rsp->OutputBufferOffset), > > + le32_to_cpu(qi_rsp->OutputBufferLength), > > + &rsp_iov[1], sizeof(struct smb2_file_all_info), > > + (char *)&tcon->crfid.file_all_info); > > + if (rc) > > + goto oshr_exit; > > + tcon->crfid.file_all_info_is_valid = 1; > > + > > + oshr_exit: > > mutex_unlock(&tcon->crfid.fid_mutex); > > + SMB2_open_free(&rqst[0]); > > + SMB2_query_info_free(&rqst[1]); > > + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); > > + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); > > return rc; > > } > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > > index 60fbe306f604..cfe9fe41ccf5 100644 > > --- a/fs/cifs/smb2pdu.c > > +++ b/fs/cifs/smb2pdu.c > > @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid) > > return buf; > > } > > > > -static __u8 > > -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp, > > - unsigned int *epoch, char *lease_key) > > +__u8 > > +smb2_parse_lease_state(struct TCP_Server_Info *server, > > + struct smb2_create_rsp *rsp, > > + unsigned int *epoch, char *lease_key) > > { > > char *data_offset; > > struct create_context *cc; > > @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path, > > } > > > > if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > > - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch, > > - oparms->fid->lease_key); > > + *oplock = smb2_parse_lease_state(server, rsp, > > + &oparms->fid->epoch, > > + oparms->fid->lease_key); > > else > > *oplock = rsp->OplockLevel; > > creat_exit: > > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > > index 87733b27a65f..72cc563c32fe 100644 > > --- a/fs/cifs/smb2proto.h > > +++ b/fs/cifs/smb2proto.h > > @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); > > > > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, > > enum securityEnum); > > +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server, > > + struct smb2_create_rsp *rsp, > > + unsigned int *epoch, char *lease_key); > > extern int smb3_encryption_required(const struct cifs_tcon *tcon); > > extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length, > > struct kvec *iov, unsigned int min_buf_size); > > -- > > 2.13.6 > >
I like the idea of tracing compound chain as one logical operation because it correlates with a particular action in the user space. Individual operations like SMB2_open() should also be traced as one logical operation for the same reason. With tracing we need to be able to match a particular system call with logical operations (open, remove, setinfo) and PDU(s) (sid, tid, mid(s)). -- Best regards, Pavel Shilovsky пн, 11 мар. 2019 г. в 08:39, Steve French <smfrench@gmail.com>: > > Pavel and I had talked about adding tracing in smb2_compound_op for > enter/exit since it allows more granularity (and with dynamic tracing, > overhead is small). Thoughts? > > On Mon, Mar 11, 2019 at 7:02 AM ronnie sahlberg > <ronniesahlberg@gmail.com> wrote: > > > > As a followup discussion. > > > > We do tracing in SMB2_open() but not in SMB2_open_init(). Perhaps we > > should change this to do the actual tracing in SMB2_open_init() > > instead and catch ALL places where we try to open a file. > > Though, we don't actually have the result of the operation in > > _open_init() which means we may need to reconsider how we trace these > > things. > > > > As we will likely switch more and more things to be compounds, maybe > > we should not have tracing in SMB2_open() and have it trace > > trace_smb3_open_err() > > > > Maybe we should trace "smb2_init : constructing compound" > > and have trace_smb3_compound_error() > > > > > > We restructure the tracing to be focused around compounds instead of > > individual PDUs. > > I.e. tracing in all the _init() functions, then tracing in > > compound_send_recv() for success/error. > > > > Tracing success/error on pdu/compound level instead of individual > > open/close/read/... operations. > > For individual non-compounded commands we can still treat these the > > same. They are just trivial compounds of one single SMB2 PDU. > > > > > > On Mon, Mar 11, 2019 at 4:01 PM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > > > > > When we open the shared root handle also ask for FILE_ALL_INFORMATION since > > > we can do this at zero cost as part of a compound. > > > Cache this information as long as the lease is return and serve any > > > future requests from cache. > > > > > > This allows us to serve "stat /<mountpoint>" directly from cache and avoid > > > a network roundtrip. Since clients ofthen need to do this quite a lot > > > this improve performance slightly. > > > > > > As an example: xfstest generic/533 performs 43 stat operations on the root > > > of the share while it is run. Which are eliminated with this patch. > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > --- > > > fs/cifs/cifsglob.h | 3 ++ > > > fs/cifs/smb2inode.c | 15 ++++--- > > > fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++--------- > > > fs/cifs/smb2pdu.c | 12 +++--- > > > fs/cifs/smb2proto.h | 3 ++ > > > 5 files changed, 116 insertions(+), 28 deletions(-) > > > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > > index f293e052e351..b8360ca221eb 100644 > > > --- a/fs/cifs/cifsglob.h > > > +++ b/fs/cifs/cifsglob.h > > > @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses) > > > > > > struct cached_fid { > > > bool is_valid:1; /* Do we have a useable root fid */ > > > + bool file_all_info_is_valid:1; > > > + > > > struct kref refcount; > > > struct cifs_fid *fid; > > > struct mutex fid_mutex; > > > struct cifs_tcon *tcon; > > > struct work_struct lease_break; > > > + struct smb2_file_all_info file_all_info; > > > }; > > > > > > /* > > > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > > > index 01a76bccdb8d..b6e07e2eed10 100644 > > > --- a/fs/cifs/smb2inode.c > > > +++ b/fs/cifs/smb2inode.c > > > @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > > > rc = open_shroot(xid, tcon, &fid); > > > if (rc) > > > goto out; > > > - rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > > > - fid.volatile_fid, smb2_data); > > > + > > > + if (tcon->crfid.file_all_info_is_valid) { > > > + move_smb2_info_to_cifs(data, > > > + &tcon->crfid.file_all_info); > > > + } else { > > > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > > > + fid.volatile_fid, smb2_data); > > > + if (!rc) > > > + move_smb2_info_to_cifs(data, smb2_data); > > > + } > > > close_shroot(&tcon->crfid); > > > - if (rc) > > > - goto out; > > > - move_smb2_info_to_cifs(data, smb2_data); > > > goto out; > > > } > > > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > > index 085e91436da7..0d8bf87592ff 100644 > > > --- a/fs/cifs/smb2ops.c > > > +++ b/fs/cifs/smb2ops.c > > > @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref) > > > SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid, > > > cfid->fid->volatile_fid); > > > cfid->is_valid = false; > > > + cfid->file_all_info_is_valid = false; > > > } > > > } > > > > > > @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work) > > > */ > > > int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > > > { > > > - struct cifs_open_parms oparams; > > > - int rc; > > > - __le16 srch_path = 0; /* Null - since an open of top of share */ > > > + struct cifs_ses *ses = tcon->ses; > > > + struct TCP_Server_Info *server = ses->server; > > > + struct cifs_open_parms oparms; > > > + struct smb2_create_rsp *o_rsp = NULL; > > > + struct smb2_query_info_rsp *qi_rsp = NULL; > > > + int resp_buftype[2]; > > > + struct smb_rqst rqst[2]; > > > + struct kvec rsp_iov[2]; > > > + struct kvec open_iov[SMB2_CREATE_IOV_SIZE]; > > > + struct kvec qi_iov[1]; > > > + int rc, flags = 0; > > > + __le16 utf16_path = 0; /* Null - since an open of top of share */ > > > u8 oplock = SMB2_OPLOCK_LEVEL_II; > > > > > > mutex_lock(&tcon->crfid.fid_mutex); > > > @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > > > return 0; > > > } > > > > > > - oparams.tcon = tcon; > > > - oparams.create_options = 0; > > > - oparams.desired_access = FILE_READ_ATTRIBUTES; > > > - oparams.disposition = FILE_OPEN; > > > - oparams.fid = pfid; > > > - oparams.reconnect = false; > > > - > > > - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL); > > > - if (rc == 0) { > > > - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > > > - tcon->crfid.tcon = tcon; > > > - tcon->crfid.is_valid = true; > > > - kref_init(&tcon->crfid.refcount); > > > - kref_get(&tcon->crfid.refcount); > > > - } > > > + if (smb3_encryption_required(tcon)) > > > + flags |= CIFS_TRANSFORM_REQ; > > > + > > > + memset(rqst, 0, sizeof(rqst)); > > > + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER; > > > + memset(rsp_iov, 0, sizeof(rsp_iov)); > > > + > > > + /* Open */ > > > + memset(&open_iov, 0, sizeof(open_iov)); > > > + rqst[0].rq_iov = open_iov; > > > + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE; > > > + > > > + oparms.tcon = tcon; > > > + oparms.create_options = 0; > > > + oparms.desired_access = FILE_READ_ATTRIBUTES; > > > + oparms.disposition = FILE_OPEN; > > > + oparms.fid = pfid; > > > + oparms.reconnect = false; > > > + > > > + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path); > > > + if (rc) > > > + goto oshr_exit; > > > + smb2_set_next_command(tcon, &rqst[0]); > > > + > > > + memset(&qi_iov, 0, sizeof(qi_iov)); > > > + rqst[1].rq_iov = qi_iov; > > > + rqst[1].rq_nvec = 1; > > > + > > > + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID, > > > + COMPOUND_FID, FILE_ALL_INFORMATION, > > > + SMB2_O_INFO_FILE, 0, > > > + sizeof(struct smb2_file_all_info) + > > > + PATH_MAX * 2, 0, NULL); > > > + if (rc) > > > + goto oshr_exit; > > > + > > > + smb2_set_related(&rqst[1]); > > > + > > > + rc = compound_send_recv(xid, ses, flags, 2, rqst, > > > + resp_buftype, rsp_iov); > > > + if (rc) > > > + goto oshr_exit; > > > + > > > + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > > > + oparms.fid->persistent_fid = o_rsp->PersistentFileId; > > > + oparms.fid->volatile_fid = o_rsp->VolatileFileId; > > > +#ifdef CONFIG_CIFS_DEBUG2 > > > + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); > > > +#endif /* CIFS_DEBUG2 */ > > > + > > > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > > > + oplock = smb2_parse_lease_state(server, o_rsp, > > > + &oparms.fid->epoch, > > > + oparms.fid->lease_key); > > > + else > > > + goto oshr_exit; > > > + > > > + > > > + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > > > + tcon->crfid.tcon = tcon; > > > + tcon->crfid.is_valid = true; > > > + kref_init(&tcon->crfid.refcount); > > > + kref_get(&tcon->crfid.refcount); > > > + > > > + > > > + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; > > > + rc = smb2_validate_and_copy_iov( > > > + le16_to_cpu(qi_rsp->OutputBufferOffset), > > > + le32_to_cpu(qi_rsp->OutputBufferLength), > > > + &rsp_iov[1], sizeof(struct smb2_file_all_info), > > > + (char *)&tcon->crfid.file_all_info); > > > + if (rc) > > > + goto oshr_exit; > > > + tcon->crfid.file_all_info_is_valid = 1; > > > + > > > + oshr_exit: > > > mutex_unlock(&tcon->crfid.fid_mutex); > > > + SMB2_open_free(&rqst[0]); > > > + SMB2_query_info_free(&rqst[1]); > > > + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); > > > + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); > > > return rc; > > > } > > > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > > > index 60fbe306f604..cfe9fe41ccf5 100644 > > > --- a/fs/cifs/smb2pdu.c > > > +++ b/fs/cifs/smb2pdu.c > > > @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid) > > > return buf; > > > } > > > > > > -static __u8 > > > -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp, > > > - unsigned int *epoch, char *lease_key) > > > +__u8 > > > +smb2_parse_lease_state(struct TCP_Server_Info *server, > > > + struct smb2_create_rsp *rsp, > > > + unsigned int *epoch, char *lease_key) > > > { > > > char *data_offset; > > > struct create_context *cc; > > > @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path, > > > } > > > > > > if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > > > - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch, > > > - oparms->fid->lease_key); > > > + *oplock = smb2_parse_lease_state(server, rsp, > > > + &oparms->fid->epoch, > > > + oparms->fid->lease_key); > > > else > > > *oplock = rsp->OplockLevel; > > > creat_exit: > > > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > > > index 87733b27a65f..72cc563c32fe 100644 > > > --- a/fs/cifs/smb2proto.h > > > +++ b/fs/cifs/smb2proto.h > > > @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); > > > > > > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, > > > enum securityEnum); > > > +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server, > > > + struct smb2_create_rsp *rsp, > > > + unsigned int *epoch, char *lease_key); > > > extern int smb3_encryption_required(const struct cifs_tcon *tcon); > > > extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length, > > > struct kvec *iov, unsigned int min_buf_size); > > > -- > > > 2.13.6 > > > > > > > -- > Thanks, > > Steve
вс, 10 мар. 2019 г. в 23:01, Ronnie Sahlberg <lsahlber@redhat.com>: > > When we open the shared root handle also ask for FILE_ALL_INFORMATION since > we can do this at zero cost as part of a compound. > Cache this information as long as the lease is return and serve any > future requests from cache. > > This allows us to serve "stat /<mountpoint>" directly from cache and avoid > a network roundtrip. Since clients ofthen need to do this quite a lot > this improve performance slightly. > > As an example: xfstest generic/533 performs 43 stat operations on the root > of the share while it is run. Which are eliminated with this patch. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/cifsglob.h | 3 ++ > fs/cifs/smb2inode.c | 15 ++++--- > fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++--------- > fs/cifs/smb2pdu.c | 12 +++--- > fs/cifs/smb2proto.h | 3 ++ > 5 files changed, 116 insertions(+), 28 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index f293e052e351..b8360ca221eb 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses) > > struct cached_fid { > bool is_valid:1; /* Do we have a useable root fid */ > + bool file_all_info_is_valid:1; > + > struct kref refcount; > struct cifs_fid *fid; > struct mutex fid_mutex; > struct cifs_tcon *tcon; > struct work_struct lease_break; > + struct smb2_file_all_info file_all_info; The structure contains Name[1] - length of 1 byte... > }; > > /* > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > index 01a76bccdb8d..b6e07e2eed10 100644 > --- a/fs/cifs/smb2inode.c > +++ b/fs/cifs/smb2inode.c > @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > rc = open_shroot(xid, tcon, &fid); > if (rc) > goto out; > - rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > - fid.volatile_fid, smb2_data); > + > + if (tcon->crfid.file_all_info_is_valid) { > + move_smb2_info_to_cifs(data, > + &tcon->crfid.file_all_info); > + } else { > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > + fid.volatile_fid, smb2_data); > + if (!rc) > + move_smb2_info_to_cifs(data, smb2_data); > + } > close_shroot(&tcon->crfid); > - if (rc) > - goto out; > - move_smb2_info_to_cifs(data, smb2_data); > goto out; > } > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 085e91436da7..0d8bf87592ff 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref) > SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid, > cfid->fid->volatile_fid); > cfid->is_valid = false; > + cfid->file_all_info_is_valid = false; > } > } > > @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work) > */ > int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > { > - struct cifs_open_parms oparams; > - int rc; > - __le16 srch_path = 0; /* Null - since an open of top of share */ > + struct cifs_ses *ses = tcon->ses; > + struct TCP_Server_Info *server = ses->server; > + struct cifs_open_parms oparms; > + struct smb2_create_rsp *o_rsp = NULL; > + struct smb2_query_info_rsp *qi_rsp = NULL; > + int resp_buftype[2]; > + struct smb_rqst rqst[2]; > + struct kvec rsp_iov[2]; > + struct kvec open_iov[SMB2_CREATE_IOV_SIZE]; > + struct kvec qi_iov[1]; > + int rc, flags = 0; > + __le16 utf16_path = 0; /* Null - since an open of top of share */ > u8 oplock = SMB2_OPLOCK_LEVEL_II; > > mutex_lock(&tcon->crfid.fid_mutex); > @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > return 0; > } > > - oparams.tcon = tcon; > - oparams.create_options = 0; > - oparams.desired_access = FILE_READ_ATTRIBUTES; > - oparams.disposition = FILE_OPEN; > - oparams.fid = pfid; > - oparams.reconnect = false; > - > - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL); > - if (rc == 0) { > - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > - tcon->crfid.tcon = tcon; > - tcon->crfid.is_valid = true; > - kref_init(&tcon->crfid.refcount); > - kref_get(&tcon->crfid.refcount); > - } > + if (smb3_encryption_required(tcon)) > + flags |= CIFS_TRANSFORM_REQ; > + > + memset(rqst, 0, sizeof(rqst)); > + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER; > + memset(rsp_iov, 0, sizeof(rsp_iov)); > + > + /* Open */ > + memset(&open_iov, 0, sizeof(open_iov)); > + rqst[0].rq_iov = open_iov; > + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE; > + > + oparms.tcon = tcon; > + oparms.create_options = 0; > + oparms.desired_access = FILE_READ_ATTRIBUTES; > + oparms.disposition = FILE_OPEN; > + oparms.fid = pfid; > + oparms.reconnect = false; > + > + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path); > + if (rc) > + goto oshr_exit; > + smb2_set_next_command(tcon, &rqst[0]); > + > + memset(&qi_iov, 0, sizeof(qi_iov)); > + rqst[1].rq_iov = qi_iov; > + rqst[1].rq_nvec = 1; > + > + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID, > + COMPOUND_FID, FILE_ALL_INFORMATION, > + SMB2_O_INFO_FILE, 0, > + sizeof(struct smb2_file_all_info) + > + PATH_MAX * 2, 0, NULL); ...but OutputLenght is sizeof(struct smb2_file_all_info) + PATH_MAX * 2. > + if (rc) > + goto oshr_exit; > + > + smb2_set_related(&rqst[1]); > + > + rc = compound_send_recv(xid, ses, flags, 2, rqst, > + resp_buftype, rsp_iov); > + if (rc) > + goto oshr_exit; > + > + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > + oparms.fid->persistent_fid = o_rsp->PersistentFileId; > + oparms.fid->volatile_fid = o_rsp->VolatileFileId; > +#ifdef CONFIG_CIFS_DEBUG2 > + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); > +#endif /* CIFS_DEBUG2 */ > + > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > + oplock = smb2_parse_lease_state(server, o_rsp, > + &oparms.fid->epoch, > + oparms.fid->lease_key); > + else > + goto oshr_exit; > + > + > + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > + tcon->crfid.tcon = tcon; > + tcon->crfid.is_valid = true; > + kref_init(&tcon->crfid.refcount); > + kref_get(&tcon->crfid.refcount); > + > + > + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; > + rc = smb2_validate_and_copy_iov( > + le16_to_cpu(qi_rsp->OutputBufferOffset), > + le32_to_cpu(qi_rsp->OutputBufferLength), > + &rsp_iov[1], sizeof(struct smb2_file_all_info), > + (char *)&tcon->crfid.file_all_info); so, the above call will cause buffer overflow. Please include PATH_MAX * 2 bytes into the structure to hold the name or even make a pointer for the info. > + if (rc) > + goto oshr_exit; > + tcon->crfid.file_all_info_is_valid = 1; > + > + oshr_exit: > mutex_unlock(&tcon->crfid.fid_mutex); > + SMB2_open_free(&rqst[0]); > + SMB2_query_info_free(&rqst[1]); > + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); > + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); > return rc; > } > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 60fbe306f604..cfe9fe41ccf5 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid) > return buf; > } > > -static __u8 > -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp, > - unsigned int *epoch, char *lease_key) > +__u8 > +smb2_parse_lease_state(struct TCP_Server_Info *server, > + struct smb2_create_rsp *rsp, > + unsigned int *epoch, char *lease_key) > { > char *data_offset; > struct create_context *cc; > @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path, > } > > if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch, > - oparms->fid->lease_key); > + *oplock = smb2_parse_lease_state(server, rsp, > + &oparms->fid->epoch, > + oparms->fid->lease_key); > else > *oplock = rsp->OplockLevel; > creat_exit: > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > index 87733b27a65f..72cc563c32fe 100644 > --- a/fs/cifs/smb2proto.h > +++ b/fs/cifs/smb2proto.h > @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); > > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, > enum securityEnum); > +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server, > + struct smb2_create_rsp *rsp, > + unsigned int *epoch, char *lease_key); > extern int smb3_encryption_required(const struct cifs_tcon *tcon); > extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length, > struct kvec *iov, unsigned int min_buf_size); > -- > 2.13.6 > In general we should probably need to set oplock/lease level on the inode of the root, so the existing caching mechanism (see cifs_inode_need_reval in inode.c) can handle stat calls, so they don't reach to query_path_info(). But it seems to be out of scope of this patch and might be done separately. -- Best regards, Pavel Shilovsky
On Tue, Mar 12, 2019 at 9:19 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > вс, 10 мар. 2019 г. в 23:01, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > When we open the shared root handle also ask for FILE_ALL_INFORMATION since > > we can do this at zero cost as part of a compound. > > Cache this information as long as the lease is return and serve any > > future requests from cache. > > > > This allows us to serve "stat /<mountpoint>" directly from cache and avoid > > a network roundtrip. Since clients ofthen need to do this quite a lot > > this improve performance slightly. > > > > As an example: xfstest generic/533 performs 43 stat operations on the root > > of the share while it is run. Which are eliminated with this patch. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/cifsglob.h | 3 ++ > > fs/cifs/smb2inode.c | 15 ++++--- > > fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++--------- > > fs/cifs/smb2pdu.c | 12 +++--- > > fs/cifs/smb2proto.h | 3 ++ > > 5 files changed, 116 insertions(+), 28 deletions(-) > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > index f293e052e351..b8360ca221eb 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses) > > > > struct cached_fid { > > bool is_valid:1; /* Do we have a useable root fid */ > > + bool file_all_info_is_valid:1; > > + > > struct kref refcount; > > struct cifs_fid *fid; > > struct mutex fid_mutex; > > struct cifs_tcon *tcon; > > struct work_struct lease_break; > > + struct smb2_file_all_info file_all_info; > > The structure contains Name[1] - length of 1 byte... > > > }; > > > > /* > > diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c > > index 01a76bccdb8d..b6e07e2eed10 100644 > > --- a/fs/cifs/smb2inode.c > > +++ b/fs/cifs/smb2inode.c > > @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, > > rc = open_shroot(xid, tcon, &fid); > > if (rc) > > goto out; > > - rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > > - fid.volatile_fid, smb2_data); > > + > > + if (tcon->crfid.file_all_info_is_valid) { > > + move_smb2_info_to_cifs(data, > > + &tcon->crfid.file_all_info); > > + } else { > > + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, > > + fid.volatile_fid, smb2_data); > > + if (!rc) > > + move_smb2_info_to_cifs(data, smb2_data); > > + } > > close_shroot(&tcon->crfid); > > - if (rc) > > - goto out; > > - move_smb2_info_to_cifs(data, smb2_data); > > goto out; > > } > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > index 085e91436da7..0d8bf87592ff 100644 > > --- a/fs/cifs/smb2ops.c > > +++ b/fs/cifs/smb2ops.c > > @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref) > > SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid, > > cfid->fid->volatile_fid); > > cfid->is_valid = false; > > + cfid->file_all_info_is_valid = false; > > } > > } > > > > @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work) > > */ > > int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > > { > > - struct cifs_open_parms oparams; > > - int rc; > > - __le16 srch_path = 0; /* Null - since an open of top of share */ > > + struct cifs_ses *ses = tcon->ses; > > + struct TCP_Server_Info *server = ses->server; > > + struct cifs_open_parms oparms; > > + struct smb2_create_rsp *o_rsp = NULL; > > + struct smb2_query_info_rsp *qi_rsp = NULL; > > + int resp_buftype[2]; > > + struct smb_rqst rqst[2]; > > + struct kvec rsp_iov[2]; > > + struct kvec open_iov[SMB2_CREATE_IOV_SIZE]; > > + struct kvec qi_iov[1]; > > + int rc, flags = 0; > > + __le16 utf16_path = 0; /* Null - since an open of top of share */ > > u8 oplock = SMB2_OPLOCK_LEVEL_II; > > > > mutex_lock(&tcon->crfid.fid_mutex); > > @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) > > return 0; > > } > > > > - oparams.tcon = tcon; > > - oparams.create_options = 0; > > - oparams.desired_access = FILE_READ_ATTRIBUTES; > > - oparams.disposition = FILE_OPEN; > > - oparams.fid = pfid; > > - oparams.reconnect = false; > > - > > - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL); > > - if (rc == 0) { > > - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > > - tcon->crfid.tcon = tcon; > > - tcon->crfid.is_valid = true; > > - kref_init(&tcon->crfid.refcount); > > - kref_get(&tcon->crfid.refcount); > > - } > > + if (smb3_encryption_required(tcon)) > > + flags |= CIFS_TRANSFORM_REQ; > > + > > + memset(rqst, 0, sizeof(rqst)); > > + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER; > > + memset(rsp_iov, 0, sizeof(rsp_iov)); > > + > > + /* Open */ > > + memset(&open_iov, 0, sizeof(open_iov)); > > + rqst[0].rq_iov = open_iov; > > + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE; > > + > > + oparms.tcon = tcon; > > + oparms.create_options = 0; > > + oparms.desired_access = FILE_READ_ATTRIBUTES; > > + oparms.disposition = FILE_OPEN; > > + oparms.fid = pfid; > > + oparms.reconnect = false; > > + > > + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path); > > + if (rc) > > + goto oshr_exit; > > + smb2_set_next_command(tcon, &rqst[0]); > > + > > + memset(&qi_iov, 0, sizeof(qi_iov)); > > + rqst[1].rq_iov = qi_iov; > > + rqst[1].rq_nvec = 1; > > + > > + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID, > > + COMPOUND_FID, FILE_ALL_INFORMATION, > > + SMB2_O_INFO_FILE, 0, > > + sizeof(struct smb2_file_all_info) + > > + PATH_MAX * 2, 0, NULL); > > ...but OutputLenght is sizeof(struct smb2_file_all_info) + PATH_MAX * 2. > > > + if (rc) > > + goto oshr_exit; > > + > > + smb2_set_related(&rqst[1]); > > + > > + rc = compound_send_recv(xid, ses, flags, 2, rqst, > > + resp_buftype, rsp_iov); > > + if (rc) > > + goto oshr_exit; > > + > > + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > > + oparms.fid->persistent_fid = o_rsp->PersistentFileId; > > + oparms.fid->volatile_fid = o_rsp->VolatileFileId; > > +#ifdef CONFIG_CIFS_DEBUG2 > > + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); > > +#endif /* CIFS_DEBUG2 */ > > + > > + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > > + oplock = smb2_parse_lease_state(server, o_rsp, > > + &oparms.fid->epoch, > > + oparms.fid->lease_key); > > + else > > + goto oshr_exit; > > + > > + > > + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); > > + tcon->crfid.tcon = tcon; > > + tcon->crfid.is_valid = true; > > + kref_init(&tcon->crfid.refcount); > > + kref_get(&tcon->crfid.refcount); > > + > > + > > + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; > > + rc = smb2_validate_and_copy_iov( > > + le16_to_cpu(qi_rsp->OutputBufferOffset), > > + le32_to_cpu(qi_rsp->OutputBufferLength), > > + &rsp_iov[1], sizeof(struct smb2_file_all_info), > > + (char *)&tcon->crfid.file_all_info); > > so, the above call will cause buffer overflow. Please include PATH_MAX > * 2 bytes into the structure to hold the name or even make a pointer > for the info. Thanks. I changed it to clamp the length to sizeof(struct smb2_file_all_info) since we never actually need or use the name. > > > + if (rc) > > + goto oshr_exit; > > + tcon->crfid.file_all_info_is_valid = 1; > > + > > + oshr_exit: > > mutex_unlock(&tcon->crfid.fid_mutex); > > + SMB2_open_free(&rqst[0]); > > + SMB2_query_info_free(&rqst[1]); > > + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); > > + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); > > return rc; > > } > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > > index 60fbe306f604..cfe9fe41ccf5 100644 > > --- a/fs/cifs/smb2pdu.c > > +++ b/fs/cifs/smb2pdu.c > > @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid) > > return buf; > > } > > > > -static __u8 > > -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp, > > - unsigned int *epoch, char *lease_key) > > +__u8 > > +smb2_parse_lease_state(struct TCP_Server_Info *server, > > + struct smb2_create_rsp *rsp, > > + unsigned int *epoch, char *lease_key) > > { > > char *data_offset; > > struct create_context *cc; > > @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path, > > } > > > > if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) > > - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch, > > - oparms->fid->lease_key); > > + *oplock = smb2_parse_lease_state(server, rsp, > > + &oparms->fid->epoch, > > + oparms->fid->lease_key); > > else > > *oplock = rsp->OplockLevel; > > creat_exit: > > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > > index 87733b27a65f..72cc563c32fe 100644 > > --- a/fs/cifs/smb2proto.h > > +++ b/fs/cifs/smb2proto.h > > @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); > > > > extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, > > enum securityEnum); > > +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server, > > + struct smb2_create_rsp *rsp, > > + unsigned int *epoch, char *lease_key); > > extern int smb3_encryption_required(const struct cifs_tcon *tcon); > > extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length, > > struct kvec *iov, unsigned int min_buf_size); > > -- > > 2.13.6 > > > > In general we should probably need to set oplock/lease level on the > inode of the root, so the existing caching mechanism (see > cifs_inode_need_reval in inode.c) can handle stat calls, so they don't > reach to query_path_info(). But it seems to be out of scope of this > patch and might be done separately. > > -- > Best regards, > Pavel Shilovsky
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index f293e052e351..b8360ca221eb 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -962,11 +962,14 @@ cap_unix(struct cifs_ses *ses) struct cached_fid { bool is_valid:1; /* Do we have a useable root fid */ + bool file_all_info_is_valid:1; + struct kref refcount; struct cifs_fid *fid; struct mutex fid_mutex; struct cifs_tcon *tcon; struct work_struct lease_break; + struct smb2_file_all_info file_all_info; }; /* diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c index 01a76bccdb8d..b6e07e2eed10 100644 --- a/fs/cifs/smb2inode.c +++ b/fs/cifs/smb2inode.c @@ -309,12 +309,17 @@ smb2_query_path_info(const unsigned int xid, struct cifs_tcon *tcon, rc = open_shroot(xid, tcon, &fid); if (rc) goto out; - rc = SMB2_query_info(xid, tcon, fid.persistent_fid, - fid.volatile_fid, smb2_data); + + if (tcon->crfid.file_all_info_is_valid) { + move_smb2_info_to_cifs(data, + &tcon->crfid.file_all_info); + } else { + rc = SMB2_query_info(xid, tcon, fid.persistent_fid, + fid.volatile_fid, smb2_data); + if (!rc) + move_smb2_info_to_cifs(data, smb2_data); + } close_shroot(&tcon->crfid); - if (rc) - goto out; - move_smb2_info_to_cifs(data, smb2_data); goto out; } diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 085e91436da7..0d8bf87592ff 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -619,6 +619,7 @@ smb2_close_cached_fid(struct kref *ref) SMB2_close(0, cfid->tcon, cfid->fid->persistent_fid, cfid->fid->volatile_fid); cfid->is_valid = false; + cfid->file_all_info_is_valid = false; } } @@ -643,9 +644,18 @@ smb2_cached_lease_break(struct work_struct *work) */ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) { - struct cifs_open_parms oparams; - int rc; - __le16 srch_path = 0; /* Null - since an open of top of share */ + struct cifs_ses *ses = tcon->ses; + struct TCP_Server_Info *server = ses->server; + struct cifs_open_parms oparms; + struct smb2_create_rsp *o_rsp = NULL; + struct smb2_query_info_rsp *qi_rsp = NULL; + int resp_buftype[2]; + struct smb_rqst rqst[2]; + struct kvec rsp_iov[2]; + struct kvec open_iov[SMB2_CREATE_IOV_SIZE]; + struct kvec qi_iov[1]; + int rc, flags = 0; + __le16 utf16_path = 0; /* Null - since an open of top of share */ u8 oplock = SMB2_OPLOCK_LEVEL_II; mutex_lock(&tcon->crfid.fid_mutex); @@ -657,22 +667,87 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid) return 0; } - oparams.tcon = tcon; - oparams.create_options = 0; - oparams.desired_access = FILE_READ_ATTRIBUTES; - oparams.disposition = FILE_OPEN; - oparams.fid = pfid; - oparams.reconnect = false; - - rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL); - if (rc == 0) { - memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); - tcon->crfid.tcon = tcon; - tcon->crfid.is_valid = true; - kref_init(&tcon->crfid.refcount); - kref_get(&tcon->crfid.refcount); - } + if (smb3_encryption_required(tcon)) + flags |= CIFS_TRANSFORM_REQ; + + memset(rqst, 0, sizeof(rqst)); + resp_buftype[0] = resp_buftype[1] = CIFS_NO_BUFFER; + memset(rsp_iov, 0, sizeof(rsp_iov)); + + /* Open */ + memset(&open_iov, 0, sizeof(open_iov)); + rqst[0].rq_iov = open_iov; + rqst[0].rq_nvec = SMB2_CREATE_IOV_SIZE; + + oparms.tcon = tcon; + oparms.create_options = 0; + oparms.desired_access = FILE_READ_ATTRIBUTES; + oparms.disposition = FILE_OPEN; + oparms.fid = pfid; + oparms.reconnect = false; + + rc = SMB2_open_init(tcon, &rqst[0], &oplock, &oparms, &utf16_path); + if (rc) + goto oshr_exit; + smb2_set_next_command(tcon, &rqst[0]); + + memset(&qi_iov, 0, sizeof(qi_iov)); + rqst[1].rq_iov = qi_iov; + rqst[1].rq_nvec = 1; + + rc = SMB2_query_info_init(tcon, &rqst[1], COMPOUND_FID, + COMPOUND_FID, FILE_ALL_INFORMATION, + SMB2_O_INFO_FILE, 0, + sizeof(struct smb2_file_all_info) + + PATH_MAX * 2, 0, NULL); + if (rc) + goto oshr_exit; + + smb2_set_related(&rqst[1]); + + rc = compound_send_recv(xid, ses, flags, 2, rqst, + resp_buftype, rsp_iov); + if (rc) + goto oshr_exit; + + o_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; + oparms.fid->persistent_fid = o_rsp->PersistentFileId; + oparms.fid->volatile_fid = o_rsp->VolatileFileId; +#ifdef CONFIG_CIFS_DEBUG2 + oparms.fid->mid = le64_to_cpu(o_rsp->sync_hdr.MessageId); +#endif /* CIFS_DEBUG2 */ + + if (o_rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) + oplock = smb2_parse_lease_state(server, o_rsp, + &oparms.fid->epoch, + oparms.fid->lease_key); + else + goto oshr_exit; + + + memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid)); + tcon->crfid.tcon = tcon; + tcon->crfid.is_valid = true; + kref_init(&tcon->crfid.refcount); + kref_get(&tcon->crfid.refcount); + + + qi_rsp = (struct smb2_query_info_rsp *)rsp_iov[1].iov_base; + rc = smb2_validate_and_copy_iov( + le16_to_cpu(qi_rsp->OutputBufferOffset), + le32_to_cpu(qi_rsp->OutputBufferLength), + &rsp_iov[1], sizeof(struct smb2_file_all_info), + (char *)&tcon->crfid.file_all_info); + if (rc) + goto oshr_exit; + tcon->crfid.file_all_info_is_valid = 1; + + oshr_exit: mutex_unlock(&tcon->crfid.fid_mutex); + SMB2_open_free(&rqst[0]); + SMB2_query_info_free(&rqst[1]); + free_rsp_buf(resp_buftype[0], rsp_iov[0].iov_base); + free_rsp_buf(resp_buftype[1], rsp_iov[1].iov_base); return rc; } diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 60fbe306f604..cfe9fe41ccf5 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -1797,9 +1797,10 @@ create_reconnect_durable_buf(struct cifs_fid *fid) return buf; } -static __u8 -parse_lease_state(struct TCP_Server_Info *server, struct smb2_create_rsp *rsp, - unsigned int *epoch, char *lease_key) +__u8 +smb2_parse_lease_state(struct TCP_Server_Info *server, + struct smb2_create_rsp *rsp, + unsigned int *epoch, char *lease_key) { char *data_offset; struct create_context *cc; @@ -2456,8 +2457,9 @@ SMB2_open(const unsigned int xid, struct cifs_open_parms *oparms, __le16 *path, } if (rsp->OplockLevel == SMB2_OPLOCK_LEVEL_LEASE) - *oplock = parse_lease_state(server, rsp, &oparms->fid->epoch, - oparms->fid->lease_key); + *oplock = smb2_parse_lease_state(server, rsp, + &oparms->fid->epoch, + oparms->fid->lease_key); else *oplock = rsp->OplockLevel; creat_exit: diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index 87733b27a65f..72cc563c32fe 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -223,6 +223,9 @@ extern int smb3_validate_negotiate(const unsigned int, struct cifs_tcon *); extern enum securityEnum smb2_select_sectype(struct TCP_Server_Info *, enum securityEnum); +extern __u8 smb2_parse_lease_state(struct TCP_Server_Info *server, + struct smb2_create_rsp *rsp, + unsigned int *epoch, char *lease_key); extern int smb3_encryption_required(const struct cifs_tcon *tcon); extern int smb2_validate_iov(unsigned int offset, unsigned int buffer_length, struct kvec *iov, unsigned int min_buf_size);
When we open the shared root handle also ask for FILE_ALL_INFORMATION since we can do this at zero cost as part of a compound. Cache this information as long as the lease is return and serve any future requests from cache. This allows us to serve "stat /<mountpoint>" directly from cache and avoid a network roundtrip. Since clients ofthen need to do this quite a lot this improve performance slightly. As an example: xfstest generic/533 performs 43 stat operations on the root of the share while it is run. Which are eliminated with this patch. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/cifsglob.h | 3 ++ fs/cifs/smb2inode.c | 15 ++++--- fs/cifs/smb2ops.c | 111 +++++++++++++++++++++++++++++++++++++++++++--------- fs/cifs/smb2pdu.c | 12 +++--- fs/cifs/smb2proto.h | 3 ++ 5 files changed, 116 insertions(+), 28 deletions(-)