Re: [Qemu-devel] [PATCH 2/2] hw/9pfs: fix alignment issue when host filesystem block size is larger than client msize

[Aneesh Kumar K.V]

Jevon Qiao <address@hidden> writes:

> Hi Aneesh,
>
> Thank you for reviewing my code, please see my reply in-line.
> On 14/2/16 21:38, Aneesh Kumar K.V wrote:
>> Jevon Qiao <address@hidden> writes:
>>
>>> The following patch is to fix alignment issue when host filesystem block
>>> size
>>> is larger than client msize.
>>>
>>> Thanks,
>>> Jevon
>> That is not the right format to send patch. You can send them as a
>> series using git-send-email.
> Yes, you're correct. I will send the patches later after I address all 
> the technical comments.
>>> From: Jevon Qiao <address@hidden>
>>> Date: Sun, 14 Feb 2016 15:11:08 +0800
>>> Subject: [PATCH] hw/9pfs: fix alignment issue when host filesystem block
>>> size
>>>    is larger than client msize.
>>>
>>> Per the previous implementation, iounit will be assigned to be 0 after the
>>> first if statement as (s->msize - P9_IOHDRSZ)/stbuf.f_bsize will be zero
>>> when
>>> host filesystem block size is larger than msize. Finally, iounit will be
>>> equal
>>> to s->msize - P9_IOHDRSZ, which is usually not aligned.
>>>
>>> Signed-off-by: Jevon Qiao <address@hidden>
>>> ---
>>>    hw/9pfs/virtio-9p.c | 19 ++++++++++++++++---
>>>    1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
>>> index f972731..005d3a8 100644
>>> --- a/hw/9pfs/virtio-9p.c
>>> +++ b/hw/9pfs/virtio-9p.c
>>> @@ -1326,7 +1326,7 @@ out_nofid:
>>>    static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path)
>>>    {
>>>        struct statfs stbuf;
>>> -    int32_t iounit = 0;
>>> +    int32_t iounit = 0, unit = 0;
>>>        V9fsState *s = pdu->s;
>>>
>>>        /*
>>> @@ -1334,8 +1334,21 @@ static int32_t get_iounit(V9fsPDU *pdu, V9fsPath
>>> *path)
>>>         * and as well as less than (client msize - P9_IOHDRSZ))
>>>         */
>>>        if (!v9fs_co_statfs(pdu, path, &stbuf)) {
>>> -        iounit = stbuf.f_bsize;
>>> -        iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
>>> +    /*
>>> +     * If host filesystem block size is larger than client msize,
>>> +     * we will use PAGESIZE as the unit. The reason why we choose
>>> +     * PAGESIZE is because the data will be splitted in terms of
>>> +     * PAGESIZE in the virtio layer. In this case, the final
>>> +     * iounit is equal to the value of ((msize/unit) - 1) * unit.
>>> +     */
>>> +    if (stbuf.f_bsize > s->msize) {
>>> +        iounit = 4096;
>>> +        unit = 4096;
>> What page size it should be guest or host ?. Also why 4096 ?. ppc64 use
>> 64K page size.
> The data to be read or written will be divided into pieces according to the
> size of iounit and msize firstly, and then mapped to pages before being 
> added
> into virtqueue. Since all these operations happen in the guest side, so the
> page size should be guest. Please correct me if I'm wrong.

I am not sure I understand the details correctly. iounit is the size
that we use in client_read to determine the  size in which
we should request I/O from the client. But we still can't do I/O in size
larger than s->msize. If you look at the client side (kernel 9p fs), you
will find

        rsize = fid->iounit;
        if (!rsize || rsize > clnt->msize-P9_IOHDRSZ)
                rsize = clnt->msize - P9_IOHDRSZ;

if your iounit calculation ends up zero, that should be handled
correctly by

    if (!iounit) {
        iounit = s->msize - P9_IOHDRSZ;
    }
    return iounit;


So what is the issue here. ?

-aneesh