Re: [V9fs-developer] [PATCH] net/9p: avoid request size exceed to the virtqueue number in the zero copy

jiangyiwen
2018-08-03T08:18:35Z

On 2018/8/3 15:32, Dominique Martinet wrote:
> jiangyiwen wrote on Fri, Aug 03, 2018:
>> Unfortunately, when the address(input and response headers) are not
>> at page boundary, it will need two extra entry in the zero copy, or
>> else it will cause sg array out of bounds.
>>
>> To avoid the problem, we should subtract two pages for maxsize.
>
> Good catch, that must have been painful to figure.
>
> Given we know how big the headers are (something like 11 or 23 bytes
> depending on the op/direction, it's capped by P9_IOHDRSZ at 24),
> couldn't we just cheat and not use the start of the buffer if we detect
> it's overlapping?
>

Actually, generally the P9_IOHDRSZ will not cause the problem, because
24 bytes is too small, but P9_ZC_HDR_SZ(4096 bytes) often cause two pages.

So I have a question about why we need to use P9_ZC_HDR_SZ, actually we
may use P9_IOHDRSZ instead.

> It's probably faster to memcpy a few bytes than to use two pages for the
> sg list...
> It's definitely ugly though, just taking more margin here is probably
> just as good.
>
>
>
> I'm going to be picky about English again, sorry, please bear with me.
>
>> Subject: net/9p: avoid request size exceed to the virtqueue number in
>> the zero copy
>
> This is >72 characters so a little bit too long, if possible to shorten
> it.
> I'm also not sure 'exceed' is a noun so I probably wouldn't have
> understood this sentence without the rest of the message...
>
> The balance is difficult but it doesn't need to contain too much details
> either something like "9p/virtio: reduce transport maxsize" is simple
> but probably enough as it describes what is done: someone can look at
> the rest of the message for the justification.
>

Thanks, I will resend the patch later.

>
>
>> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
>> ---
>> net/9p/trans_virtio.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
>> index 6265d1d..63591b2 100644
>> --- a/net/9p/trans_virtio.c
>> +++ b/net/9p/trans_virtio.c
>> @@ -754,11 +754,12 @@ static void p9_virtio_remove(struct virtio_device *vdev)
>> .cancel = p9_virtio_cancel,
>> /*
>> * We leave one entry for input and one entry for response
>> - * headers. We also skip one more entry to accomodate, address
>> - * that are not at page boundary, that can result in an extra
>> - * page in zero copy.
>> + * headers. We also skip three more entrys to accomodate
>
> "entry"'s plural is "entries", this word is in checkpatch's dictionary
> as a common typo

Thanks, I will modify it.

>
>> + * (input + response headers + data pages), address
>> + * that are not at page boundary, that can result in
>> + * an extra page in zero copy.
>> */
>> - .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 3),
>> + .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 5),
>> .def = 1,
>> .owner = THIS_MODULE,
>> };
>
> Thanks,
>