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

[Leon Romanovsky]

On Wed, Aug 23, 2017 at 07:10:38PM +0000, Long Li wrote:
>
>
> > -----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.

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

Attachment: signature.asc
Description: PGP signature