mbox series

[00/14] cifs: add compounding support for smb2+

Message ID 20180213044234.18364-1-lsahlber@redhat.com
Headers show
Series cifs: add compounding support for smb2+ | expand

Message

Ronnie Sahlberg Feb. 13, 2018, 4:42 a.m. UTC 
Steve, all,

Please find attached a series that adds the plumbing to do compounding
as well as, the last patch, changing smb2_queryfs() to use a
compound for the Create/Query/Close cycle.

There is still a small bug in SMB2_read() when using SMB3 encryption
in that we get the wrong buffer (offset by 4) for the data, but I will
look into that.
As the series is somewhat large, that does not preclude us from starting to
review.

Basic manual testing looks promising so far.
The only operation so far that will use compounding is smb2_queryfs()
so far. You can use 'df' to invoke this operation if you want to look
at what the PDUs for compounding looks like on the wire.

Once we get this finished and ready to merge, we can move on and convert
all other create/set|query/close operations to be compounded too.
that should be very easy.

Wooohooo


--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ronnie Sahlberg Feb. 13, 2018, 8:15 a.m. UTC | #1 
Very basic performance test.

Simple test tool to just call statfs() 1000 times in a row.
Run on one slow VM mounting a share of a different slow VM.
YMMV.


SMB2 with compounding, running loop with 1000 statfs calls.
===========================================================
[sahlberg@rawhide-2 cifs]$ time ./cmpndtst /mnt
real    0m0.296s
user    0m0.001s
sys     0m0.039s

SMB2 without compounding:
=========================
[sahlberg@rawhide-2 cifs]$ time ./cmpndtst /mnt
real    0m0.799s
user    0m0.002s
sys     0m0.104s



Decent improvement. Cutting latency to near a third of the non-compounded version.
Of course, not many apps call statfs() but this is a good indication of what will
happen when we change the other parts of cifs.ko to do compounding for the
Create/Query/Close patterns, patterns that are used a LOT in cifs.


[sahlberg@rawhide-2 cifs]$ cat cmpndtst.c
#include <stdio.h>
#include <stdlib.h>
#include <sys/vfs.h>

static void
usage() {
  fprintf(stderr, "Usage: cmpndtst <mountpoint>\n");
  exit(10);
}


int main(int argc, char *argv[])
{
  int i;
  struct statfs sfs;

  if (argc < 2)
    usage();

  for (i = 0; i < 1000; i++)
    statfs(argv[1], &sfs);

  return 0;
}


----- Original Message -----
From: "Ronnie Sahlberg" <lsahlber@redhat.com>
To: "linux-cifs" <linux-cifs@vger.kernel.org>
Cc: "Steve French" <smfrench@gmail.com>
Sent: Tuesday, 13 February, 2018 3:42:20 PM
Subject: [PATCH 00/14] cifs: add compounding support for smb2+ 

Steve, all,

Please find attached a series that adds the plumbing to do compounding
as well as, the last patch, changing smb2_queryfs() to use a
compound for the Create/Query/Close cycle.

There is still a small bug in SMB2_read() when using SMB3 encryption
in that we get the wrong buffer (offset by 4) for the data, but I will
look into that.
As the series is somewhat large, that does not preclude us from starting to
review.

Basic manual testing looks promising so far.
The only operation so far that will use compounding is smb2_queryfs()
so far. You can use 'df' to invoke this operation if you want to look
at what the PDUs for compounding looks like on the wire.

Once we get this finished and ready to merge, we can move on and convert
all other create/set|query/close operations to be compounded too.
that should be very easy.

Wooohooo


--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Long Li Feb. 28, 2018, 8:45 p.m. UTC | #2 
> Subject: Re: [PATCH 00/14] cifs: add compounding support for smb2+

> 

> Hi Ronnie, all,

> 

> I've rebasing this on current for-next since it couldn't be applied anymore.

> Beside the simple conflicts there are a couple of non-trivial changes I've

> made.


Hi Aurelien,

I tried to apply your patch to for-next, but it couldn't apply.

I was using the for-next branch of
git://git.samba.org/sfrench/cifs-2.6.git

Is this the branch this patch is rebased for?

> 

> Since some SMB DIRECT code made it in the meantime I had to handle some

> conflicts.

> 

> in smb2pdu.c, smb2_async_writev():

> 

> > 	rqst.rq_iov = iov;

> > 	rqst.rq_nvec = 2;

> > 	rqst.rq_pages = wdata->pages;

> > 	rqst.rq_npages = wdata->nr_pages;

> > 	rqst.rq_pagesz = wdata->pagesz;

> > 	rqst.rq_tailsz = wdata->tailsz;

> > #ifdef CONFIG_CIFS_SMB_DIRECT

> > 	if (wdata->mr) {

> > 		iov[0].iov_len += sizeof(struct smbd_buffer_descriptor_v1);

> > 		rqst.rq_npages = 0;

> > 	}

> > #endif

> 

> since iov is an array of size 1, I've changed the SMB_DIRECT code to use iov[0]

> instead of iov[1]. But I have no idea if it's really correct.


Your change is correct. SMBD appends the Buffer at the end of the smb2_write for memory registration. It needs to pass those data to the transport.

> 

> The thing that bothers me is iov[] has a size of 1 yet

> > 	rqst.rq_iov = iov;

> > 	rqst.rq_nvec = 2;

> 

> mhh..


This is wrong. rq_nvec should be 1. I’m wondering why it didn't panic in your test. Have you tried testing with memory debugging on?

> 

>  =========

> 

> The other thing I changed was moving the SMB311 preauth hash update to

> the new

> cifs_send_receive() wrapper like so:

> 

> > int

> > cifs_send_recv(const unsigned int xid, struct cifs_ses *ses,

> > 	       struct smb_rqst *rqst, int *resp_buf_type, const int flags,

> > 	       struct kvec *resp_iov)

> > {

> > 	int rc;

> >

> > #ifdef CONFIG_CIFS_SMB311

> > 	if (ses->status == CifsNew)

> > 		smb311_update_preauth_hash(ses, rqst->rq_iov,

> > 					   rqst->rq_nvec);

> > #endif

> >

> > 	rc = compound_send_recv(xid, ses, flags, 1, rqst, resp_buf_type,

> > 				resp_iov);

> >

> > #ifdef CONFIG_CIFS_SMB311

> > 	if (ses->status == CifsNew) {

> > 		smb311_update_preauth_hash(ses, resp_iov, 1);

> > 	}

> > #endif

> > 	return rc;

> >

> 

>  =========

> 

> Small change for a sparse warning I was getting:

> 

> > #define COMPOUND_FID 0xFFFFFFFFFFFFFFFFULL

> 

> Added the ULL suffix as the constant is too big for an int.

> 

>  =========

> 

> Now these I have left these as-is but I have some comments:

> 

>  =========

> 

> in transport.c, compound_send_receive():

> 

> >	for (i = 0; i < num_rqst; i++) {

> >		cifs_save_when_sent(midQ[i]);

> >

> >		if (rc < 0)

> >			ses->server->sequence_number -= 2;

> >		mutex_unlock(&ses->server->srv_mutex);

> 

> We potentially unlock the mutex at every loop. Is that on purpose?

> 

>  =========

> 

> In cifs_demultiplex_thread():

> 

> >                 pdu_length = get_rfc1002_length(buf);

> > -               server->total_size = pdu_length;

> >

> > -               cifs_dbg(FYI, "RFC1002 header 0x%x\n", pdu_length);

> > +               cifs_dbg(FYI, "RFC1002 header 0x%x\n",

> > + server->total_size);

> >                 if (!is_smb_response(server, buf[0]))

> >                         continue;

> > +next_pdu:

> > +               server->total_size = pdu_length;

> 

> You set server->total_size *after* printing it.

> 

>  =========

> 

> 

> After the rebase I've tried testing mounting a WS2016 share and listing the

> root. If the correct files are listed its OK anything else is ERR:

> 

>  vers=1.0                                                     ... OK

>  vers=2.0                                                     ... OK

>  vers=2.0,sec=ntlmsspi                                        ... ERR

>  vers=2.1                                                     ... OK

>  vers=2.1,sec=ntlmsspi                                        ... ERR

>  vers=3.0                                                     ... OK

>  vers=3.0,sec=ntlmsspi                                        ... ERR

>  vers=3.0,seal                                                ... OK

>  vers=3.0,seal,sec=ntlmsspi                                   ... OK

>  vers=3.11                                                    ... ERR

>  vers=3.11,sec=ntlmsspi                                       ... ERR

>  vers=3.11,seal                                               ... ERR

>  vers=3.11,seal,sec=ntlmsspi                                  ... ERR

> 

> Most of the ntlmsspi errors (thats when we require signing) are due to bad

> signing on certain packets (ones triggered by ls):

> 

>  Find Request File:  SMB2_FIND_ID_FULL_DIRECTORY_INFO Pattern: *  Find

> Response, Error: STATUS_ACCESS_DENIED

> 

> (if access is ok without signing -> error due to wrong sig)

> 

> Weirdly enough, encryption+signingon 3.0 works.

> 

> As for SMB311, mount fails and I see lengths issues

> 

> > fs/cifs/transport.c: Sending smb: smb_len=170

> > fs/cifs/connect.c: RFC1002 header 0x0

> > fs/cifs/smb2misc.c: SMB2 data length 120 offset 128

> > fs/cifs/smb2misc.c: SMB2 len 248

> > fs/cifs/smb2misc.c: Calculated size 248 length 308 mismatch mid 0

> > SMB2 server sent bad RFC1001 len 308 not 248

> 

> And it does weird things from there.

> 

> I can provide full kernel debug log and network captures each of the run.

> 

> I've attached my rebased patches (I've added my sign-off to patches I had to

> modify to apply or added changes to).

> 

> Cheers,

> 

> --

> Aurélien Aptel / SUSE Labs Samba Team

> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3 SUSE Linux

> GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany

> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG

> Nürnberg)
Ronnie Sahlberg Feb. 28, 2018, 11:20 p.m. UTC | #3 
Thanks. I will have a look and merge your changes.

I have reworked the patches quite a bit and will re-send the patch series in a few days.


----- Original Message -----
From: "Aurélien Aptel" <aaptel@suse.com>
To: "Ronnie Sahlberg" <lsahlber@redhat.com>, "linux-cifs" <linux-cifs@vger.kernel.org>, "Long Li" <longli@microsoft.com>
Cc: "Steve French" <smfrench@gmail.com>
Sent: Thursday, 1 March, 2018 1:21:43 AM
Subject: Re: [PATCH 00/14] cifs: add compounding support for smb2+

Hi Ronnie, all,

I've rebasing this on current for-next since it couldn't be applied
anymore. Beside the simple conflicts there are a couple of non-trivial
changes I've made.

Since some SMB DIRECT code made it in the meantime I had to handle some
conflicts.

in smb2pdu.c, smb2_async_writev():

> 	rqst.rq_iov = iov;
> 	rqst.rq_nvec = 2;
> 	rqst.rq_pages = wdata->pages;
> 	rqst.rq_npages = wdata->nr_pages;
> 	rqst.rq_pagesz = wdata->pagesz;
> 	rqst.rq_tailsz = wdata->tailsz;
> #ifdef CONFIG_CIFS_SMB_DIRECT
> 	if (wdata->mr) {
> 		iov[0].iov_len += sizeof(struct smbd_buffer_descriptor_v1);
> 		rqst.rq_npages = 0;
> 	}
> #endif

since iov is an array of size 1, I've changed the SMB_DIRECT code to use
iov[0] instead of iov[1]. But I have no idea if it's really correct.

The thing that bothers me is iov[] has a size of 1 yet 
> 	rqst.rq_iov = iov;
> 	rqst.rq_nvec = 2;

mhh..

 =========

The other thing I changed was moving the SMB311 preauth hash update to the new
cifs_send_receive() wrapper like so:

> int
> cifs_send_recv(const unsigned int xid, struct cifs_ses *ses,
> 	       struct smb_rqst *rqst, int *resp_buf_type, const int flags,
> 	       struct kvec *resp_iov)
> {
> 	int rc;
> 	
> #ifdef CONFIG_CIFS_SMB311
> 	if (ses->status == CifsNew)
> 		smb311_update_preauth_hash(ses, rqst->rq_iov,
> 					   rqst->rq_nvec);
> #endif
> 	
> 	rc = compound_send_recv(xid, ses, flags, 1, rqst, resp_buf_type,
> 				resp_iov);
> 
> #ifdef CONFIG_CIFS_SMB311
> 	if (ses->status == CifsNew) {
> 		smb311_update_preauth_hash(ses, resp_iov, 1);
> 	}
> #endif
> 	return rc;
>

 =========

Small change for a sparse warning I was getting:

> #define COMPOUND_FID 0xFFFFFFFFFFFFFFFFULL

Added the ULL suffix as the constant is too big for an int.

 =========

Now these I have left these as-is but I have some comments:

 =========
 
in transport.c, compound_send_receive():

>	for (i = 0; i < num_rqst; i++) {
>		cifs_save_when_sent(midQ[i]);
>
>		if (rc < 0)
>			ses->server->sequence_number -= 2;
>		mutex_unlock(&ses->server->srv_mutex);

We potentially unlock the mutex at every loop. Is that on purpose?

 =========

In cifs_demultiplex_thread():

>                 pdu_length = get_rfc1002_length(buf);
> -               server->total_size = pdu_length;
> 
> -               cifs_dbg(FYI, "RFC1002 header 0x%x\n", pdu_length);
> +               cifs_dbg(FYI, "RFC1002 header 0x%x\n", server->total_size);
>                 if (!is_smb_response(server, buf[0]))
>                         continue;
> +next_pdu:
> +               server->total_size = pdu_length;

You set server->total_size *after* printing it.

 =========


After the rebase I've tried testing mounting a WS2016 share and listing
the root. If the correct files are listed its OK anything else is ERR:

 vers=1.0                                                     ... OK
 vers=2.0                                                     ... OK
 vers=2.0,sec=ntlmsspi                                        ... ERR
 vers=2.1                                                     ... OK
 vers=2.1,sec=ntlmsspi                                        ... ERR
 vers=3.0                                                     ... OK
 vers=3.0,sec=ntlmsspi                                        ... ERR
 vers=3.0,seal                                                ... OK
 vers=3.0,seal,sec=ntlmsspi                                   ... OK
 vers=3.11                                                    ... ERR
 vers=3.11,sec=ntlmsspi                                       ... ERR
 vers=3.11,seal                                               ... ERR
 vers=3.11,seal,sec=ntlmsspi                                  ... ERR

Most of the ntlmsspi errors (thats when we require signing) are due to
bad signing on certain packets (ones triggered by ls):

 Find Request File:  SMB2_FIND_ID_FULL_DIRECTORY_INFO Pattern: *
 Find Response, Error: STATUS_ACCESS_DENIED

(if access is ok without signing -> error due to wrong sig)

Weirdly enough, encryption+signingon 3.0 works.

As for SMB311, mount fails and I see lengths issues

> fs/cifs/transport.c: Sending smb: smb_len=170
> fs/cifs/connect.c: RFC1002 header 0x0
> fs/cifs/smb2misc.c: SMB2 data length 120 offset 128
> fs/cifs/smb2misc.c: SMB2 len 248
> fs/cifs/smb2misc.c: Calculated size 248 length 308 mismatch mid 0
> SMB2 server sent bad RFC1001 len 308 not 248

And it does weird things from there. 

I can provide full kernel debug log and network captures each of the
run.

I've attached my rebased patches (I've added my sign-off to patches I
had to modify to apply or added changes to).

Cheers,
Ronnie Sahlberg March 1, 2018, 12:25 a.m. UTC | #4 
----- Original Message -----
> From: "Aurélien Aptel" <aaptel@suse.com>
> To: "Ronnie Sahlberg" <lsahlber@redhat.com>, "linux-cifs" <linux-cifs@vger.kernel.org>, "Long Li"
> <longli@microsoft.com>
> Cc: "Steve French" <smfrench@gmail.com>
> Sent: Thursday, 1 March, 2018 1:21:43 AM
> Subject: Re: [PATCH 00/14] cifs: add compounding support for smb2+
> 
> Hi Ronnie, all,
> 
> I've rebasing this on current for-next since it couldn't be applied
> anymore. Beside the simple conflicts there are a couple of non-trivial
> changes I've made.
> 
> Since some SMB DIRECT code made it in the meantime I had to handle some
> conflicts.
> 
> in smb2pdu.c, smb2_async_writev():
> 
> > 	rqst.rq_iov = iov;
> > 	rqst.rq_nvec = 2;
> > 	rqst.rq_pages = wdata->pages;
> > 	rqst.rq_npages = wdata->nr_pages;
> > 	rqst.rq_pagesz = wdata->pagesz;
> > 	rqst.rq_tailsz = wdata->tailsz;
> > #ifdef CONFIG_CIFS_SMB_DIRECT
> > 	if (wdata->mr) {
> > 		iov[0].iov_len += sizeof(struct smbd_buffer_descriptor_v1);
> > 		rqst.rq_npages = 0;
> > 	}
> > #endif
> 
> since iov is an array of size 1, I've changed the SMB_DIRECT code to use
> iov[0] instead of iov[1]. But I have no idea if it's really correct.
> 
> The thing that bothers me is iov[] has a size of 1 yet
> > 	rqst.rq_iov = iov;
> > 	rqst.rq_nvec = 2;
> 
> mhh..
> 
>  =========
> 
> The other thing I changed was moving the SMB311 preauth hash update to the
> new
> cifs_send_receive() wrapper like so:
> 
> > int
> > cifs_send_recv(const unsigned int xid, struct cifs_ses *ses,
> > 	       struct smb_rqst *rqst, int *resp_buf_type, const int flags,
> > 	       struct kvec *resp_iov)
> > {
> > 	int rc;
> > 	
> > #ifdef CONFIG_CIFS_SMB311
> > 	if (ses->status == CifsNew)
> > 		smb311_update_preauth_hash(ses, rqst->rq_iov,
> > 					   rqst->rq_nvec);
> > #endif
> > 	
> > 	rc = compound_send_recv(xid, ses, flags, 1, rqst, resp_buf_type,
> > 				resp_iov);
> > 
> > #ifdef CONFIG_CIFS_SMB311
> > 	if (ses->status == CifsNew) {
> > 		smb311_update_preauth_hash(ses, resp_iov, 1);
> > 	}
> > #endif
> > 	return rc;
> >
> 
>  =========
> 
> Small change for a sparse warning I was getting:
> 
> > #define COMPOUND_FID 0xFFFFFFFFFFFFFFFFULL
> 
> Added the ULL suffix as the constant is too big for an int.
> 
>  =========
> 
> Now these I have left these as-is but I have some comments:
> 
>  =========
>  
> in transport.c, compound_send_receive():
> 
> >	for (i = 0; i < num_rqst; i++) {
> >		cifs_save_when_sent(midQ[i]);
> >
> >		if (rc < 0)
> >			ses->server->sequence_number -= 2;
> >		mutex_unlock(&ses->server->srv_mutex);
> 
> We potentially unlock the mutex at every loop. Is that on purpose?

No that is a bug I have fixed.

> 
>  =========
> 
> In cifs_demultiplex_thread():
> 
> >                 pdu_length = get_rfc1002_length(buf);
> > -               server->total_size = pdu_length;
> > 
> > -               cifs_dbg(FYI, "RFC1002 header 0x%x\n", pdu_length);
> > +               cifs_dbg(FYI, "RFC1002 header 0x%x\n", server->total_size);
> >                 if (!is_smb_response(server, buf[0]))
> >                         continue;
> > +next_pdu:
> > +               server->total_size = pdu_length;
> 
> You set server->total_size *after* printing it.
> 
>  =========
> 
> 
> After the rebase I've tried testing mounting a WS2016 share and listing
> the root. If the correct files are listed its OK anything else is ERR:
> 
>  vers=1.0                                                     ... OK
>  vers=2.0                                                     ... OK
>  vers=2.0,sec=ntlmsspi                                        ... ERR
>  vers=2.1                                                     ... OK
>  vers=2.1,sec=ntlmsspi                                        ... ERR
>  vers=3.0                                                     ... OK
>  vers=3.0,sec=ntlmsspi                                        ... ERR
>  vers=3.0,seal                                                ... OK
>  vers=3.0,seal,sec=ntlmsspi                                   ... OK
>  vers=3.11                                                    ... ERR
>  vers=3.11,sec=ntlmsspi                                       ... ERR
>  vers=3.11,seal                                               ... ERR
>  vers=3.11,seal,sec=ntlmsspi                                  ... ERR
> 
> Most of the ntlmsspi errors (thats when we require signing) are due to
> bad signing on certain packets (ones triggered by ls):
> 
>  Find Request File:  SMB2_FIND_ID_FULL_DIRECTORY_INFO Pattern: *
>  Find Response, Error: STATUS_ACCESS_DENIED
> 

The signing is a bug in __cifs_calc_signature() where I did not fix the check for too short iovs :
                if (... && iov[1].iov_len <= 4)
                        break; /* nothing to sign or corrupt header */

I have fixed that.

> (if access is ok without signing -> error due to wrong sig)
> 
> Weirdly enough, encryption+signingon 3.0 works.
> 
> As for SMB311, mount fails and I see lengths issues
> 
> > fs/cifs/transport.c: Sending smb: smb_len=170
> > fs/cifs/connect.c: RFC1002 header 0x0
> > fs/cifs/smb2misc.c: SMB2 data length 120 offset 128
> > fs/cifs/smb2misc.c: SMB2 len 248
> > fs/cifs/smb2misc.c: Calculated size 248 length 308 mismatch mid 0
> > SMB2 server sent bad RFC1001 len 308 not 248
> 
> And it does weird things from there.
> 
> I can provide full kernel debug log and network captures each of the
> run.
> 
> I've attached my rebased patches (I've added my sign-off to patches I
> had to modify to apply or added changes to).
> 
> Cheers,
> 
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
> GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aurélien Aptel March 1, 2018, 9:46 a.m. UTC | #5 
Long Li <longli@microsoft.com> writes:
> I tried to apply your patch to for-next, but it couldn't apply.
> 
> I was using the for-next branch of
> git://git.samba.org/sfrench/cifs-2.6.git
> 
> Is this the branch this patch is rebased for?

It should be yes, strange. I've just tried again and it worked.

  $ git branch -vvv | grep '*'
  * for-next                             13b8cb522ed3 [sfrench/for-next] [SMB3] fix smb3-encryption breakage when CONFIG_DEBUG_SG=y
  $ git remote -v | grep sfrench
  sfrench git://git.samba.org/sfrench/cifs-2.6.git (fetch)
  sfrench git://git.samba.org/sfrench/cifs-2.6.git (push)

You should probably wait for Ronnie's next version anyway.

> This is wrong. rq_nvec should be 1. I’m wondering why it didn't panic
> in your test. Have you tried testing with memory debugging on?

I supose it didn't panic because I only used plain tcp and no async
io. It was just a simple mount+ls test.

Cheers,