| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170823192300.GZ1724@mtr-leonro.local>
Date: Wed, 23 Aug 2017 22:23:00 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Long Li <longli@...rosoft.com>
Cc: Steve French <sfrench@...ba.org>,
"linux-cifs@...r.kernel.org" <linux-cifs@...r.kernel.org>,
"samba-technical@...ts.samba.org" <samba-technical@...ts.samba.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
Christoph Hellwig <hch@...radead.org>,
Tom Talpey <ttalpey@...rosoft.com>,
Matthew Wilcox <mawilcox@...rosoft.com>
Subject: Re: [Patch v2 13/19] CIFS: SMBD: Use registered memory RDMA read for
SMB write
On Wed, Aug 23, 2017 at 07:10:38PM +0000, Long Li wrote:
>
>
> > -----Original Message-----
> > From: Leon Romanovsky [mailto:leon@...nel.org]
> > Sent: Wednesday, August 23, 2017 12:02 PM
> > To: Long Li <longli@...rosoft.com>
> > Cc: Steve French <sfrench@...ba.org>; linux-cifs@...r.kernel.org; samba-
> > technical@...ts.samba.org; linux-kernel@...r.kernel.org; linux-
> > rdma@...r.kernel.org; Christoph Hellwig <hch@...radead.org>; Tom Talpey
> > <ttalpey@...rosoft.com>; Matthew Wilcox <mawilcox@...rosoft.com>
> > 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@...nel.org]
> > > > Sent: Wednesday, August 23, 2017 6:52 AM
> > > > To: Long Li <longli@...rosoft.com>
> > > > Cc: Steve French <sfrench@...ba.org>; linux-cifs@...r.kernel.org;
> > > > samba- technical@...ts.samba.org; linux-kernel@...r.kernel.org;
> > > > linux- rdma@...r.kernel.org; Christoph Hellwig <hch@...radead.org>;
> > > > Tom Talpey <ttalpey@...rosoft.com>; Matthew Wilcox
> > > > <mawilcox@...rosoft.com>; Long Li <longli@...rosoft.com>
> > > > 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@...rosoft.com>
> > > > >
> > > > > 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@...rosoft.com>
> > > > > ---
> > > > > 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
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists