|
From: | Jevon Qiao |
Subject: | Re: [Qemu-devel] [PATCH 2/2] hw/9pfs: fix alignment issue when host filesystem block size is larger than client msize |
Date: | Fri, 19 Feb 2016 17:32:02 +0800 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
Hi Greg,
The feature was finished before it happened, I'm sorry I did not sync my tree upFrom: 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.
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 meanthe issue I mentioned in another thread with Aneesh. It's not related to virtio.
Yes, I think so. The stbuf.f_bsize refers to the iounit of backend filesystem,+ * 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 ?
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
[Prev in Thread] | Current Thread | [Next in Thread] |