[v2,13/19] CIFS: SMBD: Use registered memory RDMA read for SMB write

Commit Message

Long Li Aug. 20, 2017, 7:04 p.m. UTC

From: Long Li <[e-mail]>

When sending I/O, if size is larger than rdma_readwrite_threshold we prepare to send SMB WRITE packet for a RDMA read via memory registration. The actual I/O is done out-of-the-band, so modify the relevant fields in the packet accordingly.

Signed-off-by: Long Li <[e-mail]>
---
 fs/cifs/smb2pdu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

Comments

Leon Romanovsky Aug. 23, 2017, 1:52 p.m. UTC | #1

On Sun, Aug 20, 2017 at 12:04:37PM -0700, Long Li wrote:
> From: Long Li <[e-mail]>
>
> When sending I/O, if size is larger than rdma_readwrite_threshold we prepare to send SMB WRITE packet for a RDMA read via memory registration. The actual I/O is done out-of-the-band, so modify the relevant fields in the packet accordingly.
>
> Signed-off-by: Long Li <[e-mail]>
> ---
>  fs/cifs/smb2pdu.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 5cc5f6c..5581afd 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -48,6 +48,7 @@
>  #include "smb2glob.h"
>  #include "cifspdu.h"
>  #include "cifs_spnego.h"
> +#include "smbdirect.h"
>
>  /*
>   *  The following table defines the expected "StructureSize" of SMB2 requests
> @@ -2716,6 +2717,41 @@ smb2_async_writev(struct cifs_writedata *wdata,
>  				offsetof(struct smb2_write_req, Buffer) - 4);
>  	req->RemainingBytes = 0;
>
> +	/*
> +	 * If we want to do a server RDMA read, fill in and append
> +	 * smbd_buffer_descriptor_v1 to the end of write request
> +	 */
> +	if (server->rdma && wdata->bytes >
> +		server->smbd_conn->rdma_readwrite_threshold) {
> +
> +		struct smbd_buffer_descriptor_v1 *v1;
> +		bool need_invalidate = server->dialect == SMB30_PROT_ID;
> +
> +		wdata->mr = smbd_register_mr(
> +				server->smbd_conn, wdata->pages,
> +				wdata->nr_pages, wdata->tailsz,
> +				false, need_invalidate);
> +		if (!wdata->mr) {
> +			rc = -ENOBUFS;
> +			goto async_writev_out;
> +		}
> +		req->Length = 0;
> +		req->DataOffset = 0;
> +		req->RemainingBytes =

Wow, we have CamelCase variables in linux kernel. It will help if you
start your patchset with small cleanup to convert those variables from
CamelCase to normal names.

Thanks

> +			(wdata->nr_pages-1)*PAGE_SIZE + wdata->tailsz;
> +		req->Channel = SMB2_CHANNEL_RDMA_V1_INVALIDATE;
> +		if (need_invalidate)
> +			req->Channel = SMB2_CHANNEL_RDMA_V1;
> +		req->WriteChannelInfoOffset =
> +			offsetof(struct smb2_write_req, Buffer) - 4;
> +		req->WriteChannelInfoLength =
> +			sizeof(struct smbd_buffer_descriptor_v1);
> +		v1 = (struct smbd_buffer_descriptor_v1 *) &req->Buffer[0];
> +		v1->offset = wdata->mr->mr->iova;
> +		v1->token = wdata->mr->mr->rkey;
> +		v1->length = wdata->mr->mr->length;
> +	}
> +
>  	/* 4 for rfc1002 length field and 1 for Buffer */
>  	iov[0].iov_len = 4;
>  	iov[0].iov_base = req;
> @@ -2729,10 +2765,17 @@ smb2_async_writev(struct cifs_writedata *wdata,
>  	rqst.rq_pagesz = wdata->pagesz;
>  	rqst.rq_tailsz = wdata->tailsz;
>
> +	if (wdata->mr) {
> +		iov[1].iov_len += sizeof(struct smbd_buffer_descriptor_v1);
> +		rqst.rq_npages = 0;
> +	}
> +
>  	cifs_dbg(FYI, "async write at %llu %u bytes\n",
>  		 wdata->offset, wdata->bytes);
>
> -	req->Length = cpu_to_le32(wdata->bytes);
> +	/* For RDMA read, I/O size is in RemainingBytes not in Length */
> +	if (!wdata->mr)
> +		req->Length = cpu_to_le32(wdata->bytes);
>
>  	inc_rfc1001_len(&req->hdr, wdata->bytes - 1 /* Buffer */);
>
> --
> 2.7.4
>

Long Li Aug. 23, 2017, 6:09 p.m. UTC | #2

> -----Original Message-----
> From: Leon Romanovsky [mailto:[e-mail]]
> Sent: Wednesday, August 23, 2017 6:52 AM
> To: Long Li <[e-mail]>
> Cc: Steve French <[e-mail]>; [e-mail]; samba-
> [e-mail]; [e-mail]; linux-
> [e-mail]; Christoph Hellwig <[e-mail]>; Tom Talpey
> <[e-mail]>; Matthew Wilcox <[e-mail]>;
> Long Li <[e-mail]>
> Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA
> read for SMB write
> 
> On Sun, Aug 20, 2017 at 12:04:37PM -0700, Long Li wrote:
> > From: Long Li <[e-mail]>
> >
> > When sending I/O, if size is larger than rdma_readwrite_threshold we
> prepare to send SMB WRITE packet for a RDMA read via memory registration.
> The actual I/O is done out-of-the-band, so modify the relevant fields in the
> packet accordingly.
> >
> > Signed-off-by: Long Li <[e-mail]>
> > ---
> >  fs/cifs/smb2pdu.c | 45
> ++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
> > 5cc5f6c..5581afd 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -48,6 +48,7 @@
> >  #include "smb2glob.h"
> >  #include "cifspdu.h"
> >  #include "cifs_spnego.h"
> > +#include "smbdirect.h"
> >
> >  /*
> >   *  The following table defines the expected "StructureSize" of SMB2
> > requests @@ -2716,6 +2717,41 @@ smb2_async_writev(struct
> cifs_writedata *wdata,
> >  				offsetof(struct smb2_write_req, Buffer) - 4);
> >  	req->RemainingBytes = 0;
> >
> > +	/*
> > +	 * If we want to do a server RDMA read, fill in and append
> > +	 * smbd_buffer_descriptor_v1 to the end of write request
> > +	 */
> > +	if (server->rdma && wdata->bytes >
> > +		server->smbd_conn->rdma_readwrite_threshold) {
> > +
> > +		struct smbd_buffer_descriptor_v1 *v1;
> > +		bool need_invalidate = server->dialect == SMB30_PROT_ID;
> > +
> > +		wdata->mr = smbd_register_mr(
> > +				server->smbd_conn, wdata->pages,
> > +				wdata->nr_pages, wdata->tailsz,
> > +				false, need_invalidate);
> > +		if (!wdata->mr) {
> > +			rc = -ENOBUFS;
> > +			goto async_writev_out;
> > +		}
> > +		req->Length = 0;
> > +		req->DataOffset = 0;
> > +		req->RemainingBytes =
> 
> Wow, we have CamelCase variables in linux kernel. It will help if you start
> your patchset with small cleanup to convert those variables from CamelCase
> to normal names.

They are used everywhere in the upper layer code for packet definitions, written a long time ago. (most in fs/cifs/smb2pdu.h and fs/cifs/cifspdu.h)

I suggest we do another cleanup patch to clean things up.

> 
> Thanks
> 
> > +			(wdata->nr_pages-1)*PAGE_SIZE + wdata->tailsz;
> > +		req->Channel = SMB2_CHANNEL_RDMA_V1_INVALIDATE;
> > +		if (need_invalidate)
> > +			req->Channel = SMB2_CHANNEL_RDMA_V1;
> > +		req->WriteChannelInfoOffset =
> > +			offsetof(struct smb2_write_req, Buffer) - 4;
> > +		req->WriteChannelInfoLength =
> > +			sizeof(struct smbd_buffer_descriptor_v1);
> > +		v1 = (struct smbd_buffer_descriptor_v1 *) &req->Buffer[0];
> > +		v1->offset = wdata->mr->mr->iova;
> > +		v1->token = wdata->mr->mr->rkey;
> > +		v1->length = wdata->mr->mr->length;
> > +	}
> > +
> >  	/* 4 for rfc1002 length field and 1 for Buffer */
> >  	iov[0].iov_len = 4;
> >  	iov[0].iov_base = req;
> > @@ -2729,10 +2765,17 @@ smb2_async_writev(struct cifs_writedata
> *wdata,
> >  	rqst.rq_pagesz = wdata->pagesz;
> >  	rqst.rq_tailsz = wdata->tailsz;
> >
> > +	if (wdata->mr) {
> > +		iov[1].iov_len += sizeof(struct smbd_buffer_descriptor_v1);
> > +		rqst.rq_npages = 0;
> > +	}
> > +
> >  	cifs_dbg(FYI, "async write at %llu %u bytes\n",
> >  		 wdata->offset, wdata->bytes);
> >
> > -	req->Length = cpu_to_le32(wdata->bytes);
> > +	/* For RDMA read, I/O size is in RemainingBytes not in Length */
> > +	if (!wdata->mr)
> > +		req->Length = cpu_to_le32(wdata->bytes);
> >
> >  	inc_rfc1001_len(&req->hdr, wdata->bytes - 1 /* Buffer */);
> >
> > --
> > 2.7.4
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky Aug. 23, 2017, 7:02 p.m. UTC | #3

On Wed, Aug 23, 2017 at 06:09:11PM +0000, Long Li wrote:
>
>
> > -----Original Message-----
> > From: Leon Romanovsky [mailto:[e-mail]]
> > Sent: Wednesday, August 23, 2017 6:52 AM
> > To: Long Li <[e-mail]>
> > Cc: Steve French <[e-mail]>; [e-mail]; samba-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; Christoph Hellwig <[e-mail]>; Tom Talpey
> > <[e-mail]>; Matthew Wilcox <[e-mail]>;
> > Long Li <[e-mail]>
> > Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA
> > read for SMB write
> >
> > On Sun, Aug 20, 2017 at 12:04:37PM -0700, Long Li wrote:
> > > From: Long Li <[e-mail]>
> > >
> > > When sending I/O, if size is larger than rdma_readwrite_threshold we
> > prepare to send SMB WRITE packet for a RDMA read via memory registration.
> > The actual I/O is done out-of-the-band, so modify the relevant fields in the
> > packet accordingly.
> > >
> > > Signed-off-by: Long Li <[e-mail]>
> > > ---
> > >  fs/cifs/smb2pdu.c | 45
> > ++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
> > > 5cc5f6c..5581afd 100644
> > > --- a/fs/cifs/smb2pdu.c
> > > +++ b/fs/cifs/smb2pdu.c
> > > @@ -48,6 +48,7 @@
> > >  #include "smb2glob.h"
> > >  #include "cifspdu.h"
> > >  #include "cifs_spnego.h"
> > > +#include "smbdirect.h"
> > >
> > >  /*
> > >   *  The following table defines the expected "StructureSize" of SMB2
> > > requests @@ -2716,6 +2717,41 @@ smb2_async_writev(struct
> > cifs_writedata *wdata,
> > >  				offsetof(struct smb2_write_req, Buffer) - 4);
> > >  	req->RemainingBytes = 0;
> > >
> > > +	/*
> > > +	 * If we want to do a server RDMA read, fill in and append
> > > +	 * smbd_buffer_descriptor_v1 to the end of write request
> > > +	 */
> > > +	if (server->rdma && wdata->bytes >
> > > +		server->smbd_conn->rdma_readwrite_threshold) {
> > > +
> > > +		struct smbd_buffer_descriptor_v1 *v1;
> > > +		bool need_invalidate = server->dialect == SMB30_PROT_ID;
> > > +
> > > +		wdata->mr = smbd_register_mr(
> > > +				server->smbd_conn, wdata->pages,
> > > +				wdata->nr_pages, wdata->tailsz,
> > > +				false, need_invalidate);
> > > +		if (!wdata->mr) {
> > > +			rc = -ENOBUFS;
> > > +			goto async_writev_out;
> > > +		}
> > > +		req->Length = 0;
> > > +		req->DataOffset = 0;
> > > +		req->RemainingBytes =
> >
> > Wow, we have CamelCase variables in linux kernel. It will help if you start
> > your patchset with small cleanup to convert those variables from CamelCase
> > to normal names.
>
> They are used everywhere in the upper layer code for packet definitions, written a long time ago. (most in fs/cifs/smb2pdu.h and fs/cifs/cifspdu.h)

"everywhere" is a little bit over estimated in this case.
➜  linux-rdma git:(master) git grep RemainingBytes
fs/cifs/smb2pdu.c:              req->RemainingBytes = cpu_to_le32(remaining_bytes);
fs/cifs/smb2pdu.c:              req->RemainingBytes = 0;
fs/cifs/smb2pdu.c:      req->RemainingBytes = 0;
fs/cifs/smb2pdu.c:      req->RemainingBytes = 0;
fs/cifs/smb2pdu.h:      __le32 RemainingBytes;
fs/cifs/smb2pdu.h:      __le32 RemainingBytes;

One simple "sed -i" will replace all them in one shot and it doesn't
look like undoable task.

>
> I suggest we do another cleanup patch to clean things up.

Yes, another cleanup patch is needed before your patches. You are adding
your code in 2017 and you are expected to follow present coding standards
like everyone else in the kernel.

Thanks

Long Li Aug. 23, 2017, 7:10 p.m. UTC | #4

> -----Original Message-----

> From: Leon Romanovsky [mailto:[e-mail]]

> Sent: Wednesday, August 23, 2017 12:02 PM

> To: Long Li <[e-mail]>

> Cc: Steve French <[e-mail]>; [e-mail]; samba-

> [e-mail]; [e-mail]; linux-

> [e-mail]; Christoph Hellwig <[e-mail]>; Tom Talpey

> <[e-mail]>; Matthew Wilcox <[e-mail]>

> Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA

> read for SMB write

> 

> On Wed, Aug 23, 2017 at 06:09:11PM +0000, Long Li wrote:

> >

> >

> > > -----Original Message-----

> > > From: Leon Romanovsky [mailto:[e-mail]]

> > > Sent: Wednesday, August 23, 2017 6:52 AM

> > > To: Long Li <[e-mail]>

> > > Cc: Steve French <[e-mail]>; [e-mail];

> > > samba- [e-mail]; [e-mail];

> > > linux- [e-mail]; Christoph Hellwig <[e-mail]>;

> > > Tom Talpey <[e-mail]>; Matthew Wilcox

> > > <[e-mail]>; Long Li <[e-mail]>

> > > Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA

> > > read for SMB write

> > >

> > > On Sun, Aug 20, 2017 at 12:04:37PM -0700, Long Li wrote:

> > > > From: Long Li <[e-mail]>

> > > >

> > > > When sending I/O, if size is larger than rdma_readwrite_threshold

> > > > we

> > > prepare to send SMB WRITE packet for a RDMA read via memory

> registration.

> > > The actual I/O is done out-of-the-band, so modify the relevant

> > > fields in the packet accordingly.

> > > >

> > > > Signed-off-by: Long Li <[e-mail]>

> > > > ---

> > > >  fs/cifs/smb2pdu.c | 45

> > > ++++++++++++++++++++++++++++++++++++++++++++-

> > > >  1 file changed, 44 insertions(+), 1 deletion(-)

> > > >

> > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index

> > > > 5cc5f6c..5581afd 100644

> > > > --- a/fs/cifs/smb2pdu.c

> > > > +++ b/fs/cifs/smb2pdu.c

> > > > @@ -48,6 +48,7 @@

> > > >  #include "smb2glob.h"

> > > >  #include "cifspdu.h"

> > > >  #include "cifs_spnego.h"

> > > > +#include "smbdirect.h"

> > > >

> > > >  /*

> > > >   *  The following table defines the expected "StructureSize" of

> > > > SMB2 requests @@ -2716,6 +2717,41 @@ smb2_async_writev(struct

> > > cifs_writedata *wdata,

> > > >  				offsetof(struct smb2_write_req, Buffer) - 4);

> > > >  	req->RemainingBytes = 0;

> > > >

> > > > +	/*

> > > > +	 * If we want to do a server RDMA read, fill in and append

> > > > +	 * smbd_buffer_descriptor_v1 to the end of write request

> > > > +	 */

> > > > +	if (server->rdma && wdata->bytes >

> > > > +		server->smbd_conn->rdma_readwrite_threshold) {

> > > > +

> > > > +		struct smbd_buffer_descriptor_v1 *v1;

> > > > +		bool need_invalidate = server->dialect == SMB30_PROT_ID;

> > > > +

> > > > +		wdata->mr = smbd_register_mr(

> > > > +				server->smbd_conn, wdata->pages,

> > > > +				wdata->nr_pages, wdata->tailsz,

> > > > +				false, need_invalidate);

> > > > +		if (!wdata->mr) {

> > > > +			rc = -ENOBUFS;

> > > > +			goto async_writev_out;

> > > > +		}

> > > > +		req->Length = 0;

> > > > +		req->DataOffset = 0;

> > > > +		req->RemainingBytes =

> > >

> > > Wow, we have CamelCase variables in linux kernel. It will help if

> > > you start your patchset with small cleanup to convert those

> > > variables from CamelCase to normal names.

> >

> > They are used everywhere in the upper layer code for packet

> > definitions, written a long time ago. (most in fs/cifs/smb2pdu.h and

> > fs/cifs/cifspdu.h)

> 

> "everywhere" is a little bit over estimated in this case.

> ➜  linux-rdma git:(master) git grep RemainingBytes

> fs/cifs/smb2pdu.c:              req->RemainingBytes =

> cpu_to_le32(remaining_bytes);

> fs/cifs/smb2pdu.c:              req->RemainingBytes = 0;

> fs/cifs/smb2pdu.c:      req->RemainingBytes = 0;

> fs/cifs/smb2pdu.c:      req->RemainingBytes = 0;

> fs/cifs/smb2pdu.h:      __le32 RemainingBytes;

> fs/cifs/smb2pdu.h:      __le32 RemainingBytes;

> 

> One simple "sed -i" will replace all them in one shot and it doesn't look like

> undoable task.


I mean cifspdu.h and smb2pdu.h. use CamelCase for all packet definitions. For example another one in smb2pdu.h:
struct smb2_negotiate_rsp {
        struct smb2_hdr hdr;
        __le16 StructureSize;   /* Must be 65 */
        __le16 SecurityMode;
        __le16 DialectRevision;
        __le16 NegotiateContextCount;   /* Prior to SMB3.1.1 was Reserved & MBZ */
        __u8   ServerGUID[16];
        __le32 Capabilities;
        __le32 MaxTransactSize;
        __le32 MaxReadSize;
        __le32 MaxWriteSize;
        __le64 SystemTime;      /* MBZ */
        __le64 ServerStartTime;
        __le16 SecurityBufferOffset;
        __le16 SecurityBufferLength;
        __le32 NegotiateContextOffset;  /* Pre:SMB3.1.1 was reserved/ignored */
        __u8   Buffer[1];       /* variable length GSS security buffer */
} __packed;

We may want to change them all together to keep naming consistent, that's a lot of changes.

> 

> >

> > I suggest we do another cleanup patch to clean things up.

> 

> Yes, another cleanup patch is needed before your patches. You are adding

> your code in 2017 and you are expected to follow present coding standards

> like everyone else in the kernel.

> 

> Thanks

Leon Romanovsky Aug. 23, 2017, 7:23 p.m. UTC | #5

On Wed, Aug 23, 2017 at 07:10:38PM +0000, Long Li wrote:
>
>
> > -----Original Message-----
> > From: Leon Romanovsky [mailto:[e-mail]]
> > Sent: Wednesday, August 23, 2017 12:02 PM
> > To: Long Li <[e-mail]>
> > Cc: Steve French <[e-mail]>; [e-mail]; samba-
> > [e-mail]; [e-mail]; linux-
> > [e-mail]; Christoph Hellwig <[e-mail]>; Tom Talpey
> > <[e-mail]>; Matthew Wilcox <[e-mail]>
> > Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA
> > read for SMB write
> >
> > On Wed, Aug 23, 2017 at 06:09:11PM +0000, Long Li wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Leon Romanovsky [mailto:[e-mail]]
> > > > Sent: Wednesday, August 23, 2017 6:52 AM
> > > > To: Long Li <[e-mail]>
> > > > Cc: Steve French <[e-mail]>; [e-mail];
> > > > samba- [e-mail]; [e-mail];
> > > > linux- [e-mail]; Christoph Hellwig <[e-mail]>;
> > > > Tom Talpey <[e-mail]>; Matthew Wilcox
> > > > <[e-mail]>; Long Li <[e-mail]>
> > > > Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA
> > > > read for SMB write
> > > >
> > > > On Sun, Aug 20, 2017 at 12:04:37PM -0700, Long Li wrote:
> > > > > From: Long Li <[e-mail]>
> > > > >
> > > > > When sending I/O, if size is larger than rdma_readwrite_threshold
> > > > > we
> > > > prepare to send SMB WRITE packet for a RDMA read via memory
> > registration.
> > > > The actual I/O is done out-of-the-band, so modify the relevant
> > > > fields in the packet accordingly.
> > > > >
> > > > > Signed-off-by: Long Li <[e-mail]>
> > > > > ---
> > > > >  fs/cifs/smb2pdu.c | 45
> > > > ++++++++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 44 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
> > > > > 5cc5f6c..5581afd 100644
> > > > > --- a/fs/cifs/smb2pdu.c
> > > > > +++ b/fs/cifs/smb2pdu.c
> > > > > @@ -48,6 +48,7 @@
> > > > >  #include "smb2glob.h"
> > > > >  #include "cifspdu.h"
> > > > >  #include "cifs_spnego.h"
> > > > > +#include "smbdirect.h"
> > > > >
> > > > >  /*
> > > > >   *  The following table defines the expected "StructureSize" of
> > > > > SMB2 requests @@ -2716,6 +2717,41 @@ smb2_async_writev(struct
> > > > cifs_writedata *wdata,
> > > > >  				offsetof(struct smb2_write_req, Buffer) - 4);
> > > > >  	req->RemainingBytes = 0;
> > > > >
> > > > > +	/*
> > > > > +	 * If we want to do a server RDMA read, fill in and append
> > > > > +	 * smbd_buffer_descriptor_v1 to the end of write request
> > > > > +	 */
> > > > > +	if (server->rdma && wdata->bytes >
> > > > > +		server->smbd_conn->rdma_readwrite_threshold) {
> > > > > +
> > > > > +		struct smbd_buffer_descriptor_v1 *v1;
> > > > > +		bool need_invalidate = server->dialect == SMB30_PROT_ID;
> > > > > +
> > > > > +		wdata->mr = smbd_register_mr(
> > > > > +				server->smbd_conn, wdata->pages,
> > > > > +				wdata->nr_pages, wdata->tailsz,
> > > > > +				false, need_invalidate);
> > > > > +		if (!wdata->mr) {
> > > > > +			rc = -ENOBUFS;
> > > > > +			goto async_writev_out;
> > > > > +		}
> > > > > +		req->Length = 0;
> > > > > +		req->DataOffset = 0;
> > > > > +		req->RemainingBytes =
> > > >
> > > > Wow, we have CamelCase variables in linux kernel. It will help if
> > > > you start your patchset with small cleanup to convert those
> > > > variables from CamelCase to normal names.
> > >
> > > They are used everywhere in the upper layer code for packet
> > > definitions, written a long time ago. (most in fs/cifs/smb2pdu.h and
> > > fs/cifs/cifspdu.h)
> >
> > "everywhere" is a little bit over estimated in this case.
> > ➜  linux-rdma git:(master) git grep RemainingBytes
> > fs/cifs/smb2pdu.c:              req->RemainingBytes =
> > cpu_to_le32(remaining_bytes);
> > fs/cifs/smb2pdu.c:              req->RemainingBytes = 0;
> > fs/cifs/smb2pdu.c:      req->RemainingBytes = 0;
> > fs/cifs/smb2pdu.c:      req->RemainingBytes = 0;
> > fs/cifs/smb2pdu.h:      __le32 RemainingBytes;
> > fs/cifs/smb2pdu.h:      __le32 RemainingBytes;
> >
> > One simple "sed -i" will replace all them in one shot and it doesn't look like
> > undoable task.
>
> I mean cifspdu.h and smb2pdu.h. use CamelCase for all packet definitions. For example another one in smb2pdu.h:
> struct smb2_negotiate_rsp {
>         struct smb2_hdr hdr;
>         __le16 StructureSize;   /* Must be 65 */
>         __le16 SecurityMode;
>         __le16 DialectRevision;
>         __le16 NegotiateContextCount;   /* Prior to SMB3.1.1 was Reserved & MBZ */
>         __u8   ServerGUID[16];
>         __le32 Capabilities;
>         __le32 MaxTransactSize;
>         __le32 MaxReadSize;
>         __le32 MaxWriteSize;
>         __le64 SystemTime;      /* MBZ */
>         __le64 ServerStartTime;
>         __le16 SecurityBufferOffset;
>         __le16 SecurityBufferLength;
>         __le32 NegotiateContextOffset;  /* Pre:SMB3.1.1 was reserved/ignored */
>         __u8   Buffer[1];       /* variable length GSS security buffer */
> } __packed;
>
> We may want to change them all together to keep naming consistent, that's a lot of changes.

Yes, but I'm not asking to change the structures which you are not
touching/using, concentrate on the ones used in your series.

It is great to have all these files in one coding style, but I agree with you that
it is not needed. I hope that your first patch with CC to kernel-janiators will
bring other people who will clean the rest.

Thanks

>
> >
> > >
> > > I suggest we do another cleanup patch to clean things up.
> >
> > Yes, another cleanup patch is needed before your patches. You are adding
> > your code in 2017 and you are expected to follow present coding standards
> > like everyone else in the kernel.
> >
> > Thanks

Steve French Aug. 23, 2017, 7:39 p.m. UTC | #6

Note that Camel Case in cifs.ko source is largely used for protocol
definitions to match the official protocol documentation.  So we
expect Camel Case only where it is meant to express the **exact** name
of a field in the official specification of the wire protocol but
there may be some legacy code (unrelated to protocol wire format) that
still uses camel case - and that could be cleaned up with other
patches outside this series but is lower priority.

On Wed, Aug 23, 2017 at 2:10 PM, Long Li via samba-technical
<[e-mail]> wrote:
>
>
>> -----Original Message-----
>> From: Leon Romanovsky [mailto:[e-mail]]
>> Sent: Wednesday, August 23, 2017 12:02 PM
>> To: Long Li <[e-mail]>
>> Cc: Steve French <[e-mail]>; [e-mail]; samba-
>> [e-mail]; [e-mail]; linux-
>> [e-mail]; Christoph Hellwig <[e-mail]>; Tom Talpey
>> <[e-mail]>; Matthew Wilcox <[e-mail]>
>> Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA
>> read for SMB write
>>
>> On Wed, Aug 23, 2017 at 06:09:11PM +0000, Long Li wrote:
>> >
>> >
>> > > -----Original Message-----
>> > > From: Leon Romanovsky [mailto:[e-mail]]
>> > > Sent: Wednesday, August 23, 2017 6:52 AM
>> > > To: Long Li <[e-mail]>
>> > > Cc: Steve French <[e-mail]>; [e-mail];
>> > > samba- [e-mail]; [e-mail];
>> > > linux- [e-mail]; Christoph Hellwig <[e-mail]>;
>> > > Tom Talpey <[e-mail]>; Matthew Wilcox
>> > > <[e-mail]>; Long Li <[e-mail]>
>> > > Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA
>> > > read for SMB write
>> > >
>> > > On Sun, Aug 20, 2017 at 12:04:37PM -0700, Long Li wrote:
>> > > > From: Long Li <[e-mail]>
>> > > >
>> > > > When sending I/O, if size is larger than rdma_readwrite_threshold
>> > > > we
>> > > prepare to send SMB WRITE packet for a RDMA read via memory
>> registration.
>> > > The actual I/O is done out-of-the-band, so modify the relevant
>> > > fields in the packet accordingly.
>> > > >
>> > > > Signed-off-by: Long Li <[e-mail]>
>> > > > ---
>> > > >  fs/cifs/smb2pdu.c | 45
>> > > ++++++++++++++++++++++++++++++++++++++++++++-
>> > > >  1 file changed, 44 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
>> > > > 5cc5f6c..5581afd 100644
>> > > > --- a/fs/cifs/smb2pdu.c
>> > > > +++ b/fs/cifs/smb2pdu.c
>> > > > @@ -48,6 +48,7 @@
>> > > >  #include "smb2glob.h"
>> > > >  #include "cifspdu.h"
>> > > >  #include "cifs_spnego.h"
>> > > > +#include "smbdirect.h"
>> > > >
>> > > >  /*
>> > > >   *  The following table defines the expected "StructureSize" of
>> > > > SMB2 requests @@ -2716,6 +2717,41 @@ smb2_async_writev(struct
>> > > cifs_writedata *wdata,
>> > > >                                 offsetof(struct smb2_write_req, Buffer) - 4);
>> > > >         req->RemainingBytes = 0;
>> > > >
>> > > > +       /*
>> > > > +        * If we want to do a server RDMA read, fill in and append
>> > > > +        * smbd_buffer_descriptor_v1 to the end of write request
>> > > > +        */
>> > > > +       if (server->rdma && wdata->bytes >
>> > > > +               server->smbd_conn->rdma_readwrite_threshold) {
>> > > > +
>> > > > +               struct smbd_buffer_descriptor_v1 *v1;
>> > > > +               bool need_invalidate = server->dialect == SMB30_PROT_ID;
>> > > > +
>> > > > +               wdata->mr = smbd_register_mr(
>> > > > +                               server->smbd_conn, wdata->pages,
>> > > > +                               wdata->nr_pages, wdata->tailsz,
>> > > > +                               false, need_invalidate);
>> > > > +               if (!wdata->mr) {
>> > > > +                       rc = -ENOBUFS;
>> > > > +                       goto async_writev_out;
>> > > > +               }
>> > > > +               req->Length = 0;
>> > > > +               req->DataOffset = 0;
>> > > > +               req->RemainingBytes =
>> > >
>> > > Wow, we have CamelCase variables in linux kernel. It will help if
>> > > you start your patchset with small cleanup to convert those
>> > > variables from CamelCase to normal names.
>> >
>> > They are used everywhere in the upper layer code for packet
>> > definitions, written a long time ago. (most in fs/cifs/smb2pdu.h and
>> > fs/cifs/cifspdu.h)
>>
>> "everywhere" is a little bit over estimated in this case.
>> ➜  linux-rdma git:(master) git grep RemainingBytes
>> fs/cifs/smb2pdu.c:              req->RemainingBytes =
>> cpu_to_le32(remaining_bytes);
>> fs/cifs/smb2pdu.c:              req->RemainingBytes = 0;
>> fs/cifs/smb2pdu.c:      req->RemainingBytes = 0;
>> fs/cifs/smb2pdu.c:      req->RemainingBytes = 0;
>> fs/cifs/smb2pdu.h:      __le32 RemainingBytes;
>> fs/cifs/smb2pdu.h:      __le32 RemainingBytes;
>>
>> One simple "sed -i" will replace all them in one shot and it doesn't look like
>> undoable task.
>
> I mean cifspdu.h and smb2pdu.h. use CamelCase for all packet definitions. For example another one in smb2pdu.h:
> struct smb2_negotiate_rsp {
>         struct smb2_hdr hdr;
>         __le16 StructureSize;   /* Must be 65 */
>         __le16 SecurityMode;
>         __le16 DialectRevision;
>         __le16 NegotiateContextCount;   /* Prior to SMB3.1.1 was Reserved & MBZ */
>         __u8   ServerGUID[16];
>         __le32 Capabilities;
>         __le32 MaxTransactSize;
>         __le32 MaxReadSize;
>         __le32 MaxWriteSize;
>         __le64 SystemTime;      /* MBZ */
>         __le64 ServerStartTime;
>         __le16 SecurityBufferOffset;
>         __le16 SecurityBufferLength;
>         __le32 NegotiateContextOffset;  /* Pre:SMB3.1.1 was reserved/ignored */
>         __u8   Buffer[1];       /* variable length GSS security buffer */
> } __packed;
>
> We may want to change them all together to keep naming consistent, that's a lot of changes.
>
>>
>> >
>> > I suggest we do another cleanup patch to clean things up.
>>
>> Yes, another cleanup patch is needed before your patches. You are adding
>> your code in 2017 and you are expected to follow present coding standards
>> like everyone else in the kernel.
>>
>> Thanks

Patch

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 5cc5f6c..5581afd 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -48,6 +48,7 @@ 
 #include "smb2glob.h"
 #include "cifspdu.h"
 #include "cifs_spnego.h"
+#include "smbdirect.h"
 
 /*
  *  The following table defines the expected "StructureSize" of SMB2 requests
@@ -2716,6 +2717,41 @@  smb2_async_writev(struct cifs_writedata *wdata,
 				offsetof(struct smb2_write_req, Buffer) - 4);
 	req->RemainingBytes = 0;
 
+	/*
+	 * If we want to do a server RDMA read, fill in and append
+	 * smbd_buffer_descriptor_v1 to the end of write request
+	 */
+	if (server->rdma && wdata->bytes >
+		server->smbd_conn->rdma_readwrite_threshold) {
+
+		struct smbd_buffer_descriptor_v1 *v1;
+		bool need_invalidate = server->dialect == SMB30_PROT_ID;
+
+		wdata->mr = smbd_register_mr(
+				server->smbd_conn, wdata->pages,
+				wdata->nr_pages, wdata->tailsz,
+				false, need_invalidate);
+		if (!wdata->mr) {
+			rc = -ENOBUFS;
+			goto async_writev_out;
+		}
+		req->Length = 0;
+		req->DataOffset = 0;
+		req->RemainingBytes =
+			(wdata->nr_pages-1)*PAGE_SIZE + wdata->tailsz;
+		req->Channel = SMB2_CHANNEL_RDMA_V1_INVALIDATE;
+		if (need_invalidate)
+			req->Channel = SMB2_CHANNEL_RDMA_V1;
+		req->WriteChannelInfoOffset =
+			offsetof(struct smb2_write_req, Buffer) - 4;
+		req->WriteChannelInfoLength =
+			sizeof(struct smbd_buffer_descriptor_v1);
+		v1 = (struct smbd_buffer_descriptor_v1 *) &req->Buffer[0];
+		v1->offset = wdata->mr->mr->iova;
+		v1->token = wdata->mr->mr->rkey;
+		v1->length = wdata->mr->mr->length;
+	}
+
 	/* 4 for rfc1002 length field and 1 for Buffer */
 	iov[0].iov_len = 4;
 	iov[0].iov_base = req;
@@ -2729,10 +2765,17 @@  smb2_async_writev(struct cifs_writedata *wdata,
 	rqst.rq_pagesz = wdata->pagesz;
 	rqst.rq_tailsz = wdata->tailsz;
 
+	if (wdata->mr) {
+		iov[1].iov_len += sizeof(struct smbd_buffer_descriptor_v1);
+		rqst.rq_npages = 0;
+	}
+
 	cifs_dbg(FYI, "async write at %llu %u bytes\n",
 		 wdata->offset, wdata->bytes);
 
-	req->Length = cpu_to_le32(wdata->bytes);
+	/* For RDMA read, I/O size is in RemainingBytes not in Length */
+	if (!wdata->mr)
+		req->Length = cpu_to_le32(wdata->bytes);
 
 	inc_rfc1001_len(&req->hdr, wdata->bytes - 1 /* Buffer */);