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
