Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180417191710.14855-3-longli@linuxonhyperv.com>
References: <20180417191710.14855-1-longli@linuxonhyperv.com> <20180417191710.14855-3-longli@linuxonhyperv.com>
From:   Steve French <smfrench@gmail.com>
Date:   Mon, 23 Apr 2018 10:31:51 -0500
Message-ID: <CAH2r5mv65CGR04f3Z20aRBufisPon5Ptvqk++zii3YT=GN8epQ@mail.gmail.com>
Subject: Re: [Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack
To:     Long Li <longli@microsoft.com>
Cc:     CIFS <linux-cifs@vger.kernel.org>,
        samba-technical <samba-technical@lists.samba.org>,
        LKML <linux-kernel@vger.kernel.org>, linux-rdma@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Didn't see any obvious problems, but can you fix the checkpatch
warnings and resend to the list (I am more concerned about the last
two warnings rather than the first one).

$ scripts/checkpatch.pl 0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch
WARNING: line over 80 characters
#60: FILE: fs/cifs/smbdirect.c:2106:
+        log_write(ERR, "expected the pdu length in 1st iov, but got
0x%lu\n", rqst->rq_iov[0].iov_len);

ERROR: Prefixing 0x with decimal output is defective
#60: FILE: fs/cifs/smbdirect.c:2106:
+        log_write(ERR, "expected the pdu length in 1st iov, but got
0x%lu\n", rqst->rq_iov[0].iov_len);

WARNING: braces {} are not necessary for single statement blocks
#69: FILE: fs/cifs/smbdirect.c:2112:
+    for (i = 0; i < rqst->rq_nvec-1; i++) {
         buflen += iov[i].iov_len;
     }

total: 1 errors, 2 warnings, 65 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch has style
problems, please review.

On Tue, Apr 17, 2018 at 2:17 PM, Long Li <longli@linuxonhyperv.com> wrote:
> From: Long Li <longli@microsoft.com>
>
> It's not necessary to allocate another iov when going through the buffers
> in smbd_send() through RDMA send.
>
> Remove it to reduce stack size.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/cifs/smbdirect.c | 36 ++++++++++++------------------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index b5c6c0d..f575e9a 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -2088,7 +2088,7 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>         int start, i, j;
>         int max_iov_size =
>                 info->max_send_size - sizeof(struct smbd_data_transfer);
> -       struct kvec iov[SMBDIRECT_MAX_SGE];
> +       struct kvec *iov;
>         int rc;
>
>         info->smbd_send_pending++;
> @@ -2099,32 +2099,20 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>         }
>
>         /*
> -        * This usually means a configuration error
> -        * We use RDMA read/write for packet size > rdma_readwrite_threshold
> -        * as long as it's properly configured we should never get into this
> -        * situation
> -        */
> -       if (rqst->rq_nvec + rqst->rq_npages > SMBDIRECT_MAX_SGE) {
> -               log_write(ERR, "maximum send segment %x exceeding %x\n",
> -                        rqst->rq_nvec + rqst->rq_npages, SMBDIRECT_MAX_SGE);
> -               rc = -EINVAL;
> -               goto done;
> -       }
> -
> -       /*
> -        * Remove the RFC1002 length defined in MS-SMB2 section 2.1
> -        * It is used only for TCP transport
> +        * Skip the RFC1002 length defined in MS-SMB2 section 2.1
> +        * It is used only for TCP transport in the iov[0]
>          * In future we may want to add a transport layer under protocol
>          * layer so this will only be issued to TCP transport
>          */
> -       iov[0].iov_base = (char *)rqst->rq_iov[0].iov_base + 4;
> -       iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
> -       buflen += iov[0].iov_len;
> +
> +       if (rqst->rq_iov[0].iov_len != 4) {
> +               log_write(ERR, "expected the pdu length in 1st iov, but got 0x%lu\n", rqst->rq_iov[0].iov_len);
> +               return -EINVAL;
> +       }
> +       iov = &rqst->rq_iov[1];
>
>         /* total up iov array first */
> -       for (i = 1; i < rqst->rq_nvec; i++) {
> -               iov[i].iov_base = rqst->rq_iov[i].iov_base;
> -               iov[i].iov_len = rqst->rq_iov[i].iov_len;
> +       for (i = 0; i < rqst->rq_nvec-1; i++) {
>                 buflen += iov[i].iov_len;
>         }
>
> @@ -2197,14 +2185,14 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>                                                 goto done;
>                                 }
>                                 i++;
> -                               if (i == rqst->rq_nvec)
> +                               if (i == rqst->rq_nvec-1)
>                                         break;
>                         }
>                         start = i;
>                         buflen = 0;
>                 } else {
>                         i++;
> -                       if (i == rqst->rq_nvec) {
> +                       if (i == rqst->rq_nvec-1) {
>                                 /* send out all remaining vecs */
>                                 remaining_data_length -= buflen;
>                                 log_write(INFO,
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Thanks,

Steve