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

[Jevon Qiao]

Hi Greg,
> > > > 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 ++++++++++++++++---
> Hmmm I just realize your tree is not up-to-date since hw/9pfs/virtio-9p.c got
> renamed with this recent commit:
>
> commit 60ce86c7140d5ca33d5fd87ce821681165d06b2a
> Author: Wei Liu <address@hidden>
> Date:   Thu Jan 7 18:42:20 2016 +0000
>
>      9pfs: rename virtio-9p.c to 9p.c
>
> Also 9p.c only contains generic code now, not related to virtio... see below.
The feature was finished before it happened, I'm sorry I did not sync my 
tree up
with master.
> > > >     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
> ... and here you mention virtio. Does this code really belong here ?
Sorry for confusing you, the comment might not be very clear. Here I mean
the issue I mentioned in another thread with Aneesh. It's not related to 
virtio.
> > > > +     * iounit is equal to the value of ((msize/unit) - 1) * unit.
> > > > +     */
> > > > +    if (stbuf.f_bsize > s->msize) {
> > > > +        iounit = 4096;
> > > > +        unit = 4096;
> This looks weird when reading the initial comment in get_iounit()... is
> iounit a multiple of stbuf.f_bsize in this case ?
Yes, I think so. The stbuf.f_bsize refers to the iounit of backend 
filesystem,
and to comply with the backend is a right way to go always.
> > > 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?
> getpagesize() will return the host page size. If you need the guest page size,
> you should use TARGET_PAGE_SIZE.
> And then you will hit another problem: the 9p.c file is in common-obj and
> cannot contain target specific code...
Well, good to know this, thank you for sharing this.
> Along with the other remark, I'm beginning to think you may need to move this
> to virtio-9p-device.c.
I'll think of this, thank you for the option.

Thanks,
Jevon
> > 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
> > >