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

[Jevon Qiao <scaleqiao@gmail.com>]

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 <scaleqiao@gmail.com> 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 <scaleqiao@gmail.com>
>> 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 <scaleqiao@gmail.com>
>> ---
>>    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.

As for the number 4096, It's the default value in Linux OS. I did not take
other platforms into account, it's my fault. To make it suitable for all 
platforms,
shall I use the function getpagesize() here?

Thanks,
Jevon
>> +    } else {
>> +            iounit = stbuf.f_bsize;
>> +        unit = stbuf.f_bsize;
>> +    }
>> +        iounit *= (s->msize - P9_IOHDRSZ)/unit;
>>        }
>>        if (!iounit) {
>>            iounit = s->msize - P9_IOHDRSZ;
>> -- 
> -aneesh
>