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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 Aug 2017 22:02:14 +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 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 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)