| Message ID | 20190124061932.3970-2-lsahlber@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] cifs: print CIFSMaxBufSize as part of /proc/fs/cifs/DebugData | expand |
Merged into cifs-2.6.git for-next and now testing this with the buildbot (along the 11 other fixes in for-next, more than half from Pavel's compounding series, many of them very important and marked for stable). Making good progress on cleaning up bugs prior to the (SNIA Europe) SMB3 test event next week. The list of xfstests that pass (e.g. cifs.ko to Samba or Azure or Windows or Macs) continues to increase. On Thu, Jan 24, 2019 at 12:19 AM Ronnie Sahlberg <lsahlber@redhat.com> wrote: > > or else we might trigger a session reconnect. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/smb2ops.c | 2 +- > fs/cifs/smb2pdu.h | 2 -- > 2 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 34f621fe6dc0..32f19a97c750 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -905,7 +905,7 @@ smb2_query_eas(const unsigned int xid, struct cifs_tcon *tcon, > FILE_READ_EA, > FILE_FULL_EA_INFORMATION, > SMB2_O_INFO_FILE, > - SMB2_MAX_EA_BUF, > + CIFSMaxBufSize, > &rsp_iov, &buftype, cifs_sb); > if (rc) { > /* > diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h > index 05dea6750c33..c0abe9908014 100644 > --- a/fs/cifs/smb2pdu.h > +++ b/fs/cifs/smb2pdu.h > @@ -1398,8 +1398,6 @@ struct smb2_file_link_info { /* encoding of request for level 11 */ > char FileName[0]; /* Name to be assigned to new link */ > } __packed; /* level 11 Set */ > > -#define SMB2_MAX_EA_BUF 65536 > - > struct smb2_file_full_ea_info { /* encoding of response for level 15 */ > __le32 next_entry_offset; > __u8 flags; > -- > 2.13.6 >
ср, 23 янв. 2019 г. в 22:19, Ronnie Sahlberg <lsahlber@redhat.com>: > > or else we might trigger a session reconnect. > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > --- > fs/cifs/smb2ops.c | 2 +- > fs/cifs/smb2pdu.h | 2 -- > 2 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 34f621fe6dc0..32f19a97c750 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -905,7 +905,7 @@ smb2_query_eas(const unsigned int xid, struct cifs_tcon *tcon, > FILE_READ_EA, > FILE_FULL_EA_INFORMATION, > SMB2_O_INFO_FILE, > - SMB2_MAX_EA_BUF, > + CIFSMaxBufSize, This won't work with encryption. Today we allocate server->bigbuf of CIFSMaxBufSize (16k) + MAX_SMB2_HDR_SIZE. The MAX_SMB2_HDR_SIZE is "4 len + 52 transform hdr + 64 hdr + 56 create rsp" currently. To query EAs you need to send open + query + close. So, if you want EAs to be 16k, you need to fix MAX_SMB2_HDR_SIZE to be big enough, so it can fit transform + 2 open + all create contexts + MAX_PATH * 2 + query info + close which is 52 (transform) + 64*3 (3 headers) + 89 (open) + 520 (path) + 5 * ~30 (5 create contexts) + 9 (query info) + 60 (close) ~~ 1100. We have been already using MAX_HEADER_SIZE() + CIFSMaxBufSize in some calculations in demultiplex thread and encryption functions, let's do two fixes: 1) fix MAX_SMB2_HDR_SIZE to be 52 (transform header) + 64 (hdr) + 89 (right create rsp) - long-term minor bug. 2) fix smb2_query_eas() to pass (CIFSMaxBufSize - MAX_SMB2_OPEN_SIZE - MAX_SMB2_CLOSE_SIZE), where MAX_SMB2_OPEN_SIZE is 64 (hdr) + 89 (create rsp) + 520 (path) + 150 (contexts) and MAX_SMB2_CLOSE_SIZE is 64 + 60. -- Best regards, Pavel Shilovsky
On Fri, Jan 25, 2019 at 5:48 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > ср, 23 янв. 2019 г. в 22:19, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > or else we might trigger a session reconnect. > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > --- > > fs/cifs/smb2ops.c | 2 +- > > fs/cifs/smb2pdu.h | 2 -- > > 2 files changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > index 34f621fe6dc0..32f19a97c750 100644 > > --- a/fs/cifs/smb2ops.c > > +++ b/fs/cifs/smb2ops.c > > @@ -905,7 +905,7 @@ smb2_query_eas(const unsigned int xid, struct cifs_tcon *tcon, > > FILE_READ_EA, > > FILE_FULL_EA_INFORMATION, > > SMB2_O_INFO_FILE, > > - SMB2_MAX_EA_BUF, > > + CIFSMaxBufSize, > > This won't work with encryption. Today we allocate server->bigbuf of > CIFSMaxBufSize (16k) + MAX_SMB2_HDR_SIZE. The MAX_SMB2_HDR_SIZE is "4 > len + 52 transform hdr + 64 hdr + 56 create rsp" currently. To query > EAs you need to send open + query + close. So, if you want EAs to be > 16k, you need to fix MAX_SMB2_HDR_SIZE to be big enough, so it can fit > transform + 2 open + all create contexts + MAX_PATH * 2 + query info + > close which is 52 (transform) + 64*3 (3 headers) + 89 (open) + 520 > (path) + 5 * ~30 (5 create contexts) + 9 (query info) + 60 (close) ~~ > 1100. > > We have been already using MAX_HEADER_SIZE() + CIFSMaxBufSize in some > calculations in demultiplex thread and encryption functions, let's do > two fixes: > > 1) fix MAX_SMB2_HDR_SIZE to be 52 (transform header) + 64 (hdr) + 89 > (right create rsp) - long-term minor bug. > 2) fix smb2_query_eas() to pass (CIFSMaxBufSize - MAX_SMB2_OPEN_SIZE - > MAX_SMB2_CLOSE_SIZE), where > MAX_SMB2_OPEN_SIZE is 64 (hdr) + 89 (create rsp) + 520 (path) + 150 > (contexts) and > MAX_SMB2_CLOSE_SIZE is 64 + 60. Thanks. You are right. But s/89/88 ? We only need size of the fixed part for that constant. And also need to take into account the size of the query info response and 8 byte fixed payload as well. Lots of magic constants :-( I have resent an updated patch.
чт, 24 янв. 2019 г. в 17:05, ronnie sahlberg <ronniesahlberg@gmail.com>: > > On Fri, Jan 25, 2019 at 5:48 AM Pavel Shilovsky <piastryyy@gmail.com> wrote: > > > > ср, 23 янв. 2019 г. в 22:19, Ronnie Sahlberg <lsahlber@redhat.com>: > > > > > > or else we might trigger a session reconnect. > > > > > > Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> > > > --- > > > fs/cifs/smb2ops.c | 2 +- > > > fs/cifs/smb2pdu.h | 2 -- > > > 2 files changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > > > index 34f621fe6dc0..32f19a97c750 100644 > > > --- a/fs/cifs/smb2ops.c > > > +++ b/fs/cifs/smb2ops.c > > > @@ -905,7 +905,7 @@ smb2_query_eas(const unsigned int xid, struct cifs_tcon *tcon, > > > FILE_READ_EA, > > > FILE_FULL_EA_INFORMATION, > > > SMB2_O_INFO_FILE, > > > - SMB2_MAX_EA_BUF, > > > + CIFSMaxBufSize, > > > > This won't work with encryption. Today we allocate server->bigbuf of > > CIFSMaxBufSize (16k) + MAX_SMB2_HDR_SIZE. The MAX_SMB2_HDR_SIZE is "4 > > len + 52 transform hdr + 64 hdr + 56 create rsp" currently. To query > > EAs you need to send open + query + close. So, if you want EAs to be > > 16k, you need to fix MAX_SMB2_HDR_SIZE to be big enough, so it can fit > > transform + 2 open + all create contexts + MAX_PATH * 2 + query info + > > close which is 52 (transform) + 64*3 (3 headers) + 89 (open) + 520 > > (path) + 5 * ~30 (5 create contexts) + 9 (query info) + 60 (close) ~~ > > 1100. > > > > We have been already using MAX_HEADER_SIZE() + CIFSMaxBufSize in some > > calculations in demultiplex thread and encryption functions, let's do > > two fixes: > > > > 1) fix MAX_SMB2_HDR_SIZE to be 52 (transform header) + 64 (hdr) + 89 > > (right create rsp) - long-term minor bug. > > 2) fix smb2_query_eas() to pass (CIFSMaxBufSize - MAX_SMB2_OPEN_SIZE - > > MAX_SMB2_CLOSE_SIZE), where > > MAX_SMB2_OPEN_SIZE is 64 (hdr) + 89 (create rsp) + 520 (path) + 150 > > (contexts) and > > MAX_SMB2_CLOSE_SIZE is 64 + 60. > > Thanks. > > You are right. > But s/89/88 ? We only need size of the fixed part for that constant. > And also need to take into account the size of the query info response > and 8 byte fixed payload as well. > Lots of magic constants :-( > > I have resent an updated patch. Yes, you are right - 88. 89 is together with 1 byte of variable length buffer coming next after create response header. -- Best regards, Pavel Shilovsky
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 34f621fe6dc0..32f19a97c750 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -905,7 +905,7 @@ smb2_query_eas(const unsigned int xid, struct cifs_tcon *tcon, FILE_READ_EA, FILE_FULL_EA_INFORMATION, SMB2_O_INFO_FILE, - SMB2_MAX_EA_BUF, + CIFSMaxBufSize, &rsp_iov, &buftype, cifs_sb); if (rc) { /* diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h index 05dea6750c33..c0abe9908014 100644 --- a/fs/cifs/smb2pdu.h +++ b/fs/cifs/smb2pdu.h @@ -1398,8 +1398,6 @@ struct smb2_file_link_info { /* encoding of request for level 11 */ char FileName[0]; /* Name to be assigned to new link */ } __packed; /* level 11 Set */ -#define SMB2_MAX_EA_BUF 65536 - struct smb2_file_full_ea_info { /* encoding of response for level 15 */ __le32 next_entry_offset; __u8 flags;
or else we might trigger a session reconnect. Signed-off-by: Ronnie Sahlberg <lsahlber@redhat.com> --- fs/cifs/smb2ops.c | 2 +- fs/cifs/smb2pdu.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)