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

[Long Li]

> -----Original Message-----
> From: Leon Romanovsky [mailto:leon@xxxxxxxxxx]
> Sent: Wednesday, August 23, 2017 12:02 PM
> To: Long Li <longli@xxxxxxxxxxxxx>
> Cc: Steve French <sfrench@xxxxxxxxx>; linux-cifs@xxxxxxxxxxxxxxx; samba-
> technical@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> rdma@xxxxxxxxxxxxxxx; Christoph Hellwig <hch@xxxxxxxxxxxxx>; Tom Talpey
> <ttalpey@xxxxxxxxxxxxx>; Matthew Wilcox <mawilcox@xxxxxxxxxxxxx>
> 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:leon@xxxxxxxxxx]
> > > Sent: Wednesday, August 23, 2017 6:52 AM
> > > To: Long Li <longli@xxxxxxxxxxxxx>
> > > Cc: Steve French <sfrench@xxxxxxxxx>; linux-cifs@xxxxxxxxxxxxxxx;
> > > samba- technical@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > linux- rdma@xxxxxxxxxxxxxxx; Christoph Hellwig <hch@xxxxxxxxxxxxx>;
> > > Tom Talpey <ttalpey@xxxxxxxxxxxxx>; Matthew Wilcox
> > > <mawilcox@xxxxxxxxxxxxx>; Long Li <longli@xxxxxxxxxxxxx>
> > > 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 <longli@xxxxxxxxxxxxx>
> > > >
> > > > 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 <longli@xxxxxxxxxxxxx>
> > > > ---
> > > >  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