Skip to contentSkip to conversation

hw/9pfs: fix alignment issue when host filesystem block size is larger than client msize

PATCH In review Jevon Qiao 3 participants 9 messages Feb 14, 2016 – Mar 3, 2016

lore
Share & tools
lore

Jevon Qiao

author
#0 lore raw

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

The following patch is to fix alignment issue when host filesystem block size is larger than client msize.

Thanks,
Jevon

From: Jevon Qiao <[email protected]>
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 <[email protected]>
---
 hw/9pfs/virtio-9p.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)
hw/9pfs/virtio-9p.c +16 -3
index f972731..005d3a8 100644
@@ -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;
+    } else {
+            iounit = stbuf.f_bsize;
+        unit = stbuf.f_bsize;
+    }
+        iounit *= (s->msize - P9_IOHDRSZ)/unit;
     }
     if (!iounit) {
         iounit = s->msize - P9_IOHDRSZ;
#1 lore raw
Quoted 6 lines · Jevon Qiao

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.

Quoted 39 lines

From: Jevon Qiao <[email protected]>
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 <[email protected]>
---
  hw/9pfs/virtio-9p.c | 19 ++++++++++++++++---
  1 file changed, 16 insertions(+), 3 deletions(-)
hw/9pfs/virtio-9p.c +1 -3
index f972731..005d3a8 100644
@@ -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;

hw/9pfs/virtio-9p.c static int32_t get_iounit(V9fsPDU *pdu, V9fsPath · line 1347

Quoted 10 lines
hw/9pfs/virtio-9p.c +10 -0
+    /*
+     * 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.

Quoted 9 lines
hw/9pfs/virtio-9p.c +5 -0
+    } else {
+            iounit = stbuf.f_bsize;
+        unit = stbuf.f_bsize;
+    }
+        iounit *= (s->msize - P9_IOHDRSZ)/unit;
      }
      if (!iounit) {

          iounit = s->msize - P9_IOHDRSZ;

-aneesh

author
#2 lore raw

Hi Aneesh,

Thank you for reviewing my code, please see my reply in-line.

Quoted 10 lines · Aneesh Kumar K.V
Quoted 6 lines · Jevon Qiao

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.

Quoted 42 lines
Quoted 42 lines

From: Jevon Qiao <[email protected]>
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 <[email protected]>
---
   hw/9pfs/virtio-9p.c | 19 ++++++++++++++++---
   1 file changed, 16 insertions(+), 3 deletions(-)
hw/9pfs/virtio-9p.c +5 -3
index f972731..005d3a8 100644
@@ -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
Quoted 8 lines

hw/9pfs/virtio-9p.c static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path) · line 1346

Quoted 6 lines
hw/9pfs/virtio-9p.c +6 -0
+     * 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

Quoted 11 lines
Quoted 9 lines
hw/9pfs/virtio-9p.c +5 -0
+    } else {
+            iounit = stbuf.f_bsize;
+        unit = stbuf.f_bsize;
+    }
+        iounit *= (s->msize - P9_IOHDRSZ)/unit;
       }
       if (!iounit) {

           iounit = s->msize - P9_IOHDRSZ;

-aneesh

#3 lore raw
Quoted 3 lines · On Wed, 17 Feb 2016 15:14:48 +0800 Jevon Qiao <[email protected]> wrote:

Hi Aneesh,

Thank you for reviewing my code, please see my reply in-line.

Jevon,

Please read comments below.

Quoted 7 lines · Aneesh Kumar K.V
Quoted 6 lines
Quoted 4 lines · Jevon Qiao

The following patch is to fix alignment issue when host filesystem block size
is larger than client msize.

Quoted 22 lines
Quoted 4 lines
Quoted 2 lines

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.

Quoted 16 lines
Quoted 16 lines

From: Jevon Qiao <[email protected]>
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 <[email protected]>
---
   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 <[email protected]>
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.

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
   1 file changed, 16 insertions(+), 3 deletions(-)
hw/9pfs/virtio-9p.c +1 -1
index f972731..005d3a8 100644
@@ -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;
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
hw/9pfs/virtio-9p.c +0 -0
       /*

@@ -1334,8 +1334,21 @@ static int32_t get_iounit(V9fsPDU *pdu, V9fsPath
 *path)
        * and as well as less than (client msize - P9_IOHDRSZ))
        */
Quoted 8 lines

hw/9pfs/virtio-9p.c static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path) · line 1341

Quoted 8 lines
hw/9pfs/virtio-9p.c +5 -2
       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 ?

Quoted 5 lines
Quoted 5 lines

hw/9pfs/virtio-9p.c static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path) · line 1346

Quoted 5 lines
hw/9pfs/virtio-9p.c +5 -0
+     * 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 ?

Quoted 8 lines
Quoted 2 lines

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.

Quoted 5 lines

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...

Along with the other remark, I'm beginning to think you may need to move this to virtio-9p-device.c.

Quoted 14 lines

Thanks,
Jevon

Quoted 11 lines
Quoted 9 lines
hw/9pfs/virtio-9p.c +5 -0
+    } else {
+            iounit = stbuf.f_bsize;
+        unit = stbuf.f_bsize;
+    }
+        iounit *= (s->msize - P9_IOHDRSZ)/unit;
       }
       if (!iounit) {

           iounit = s->msize - P9_IOHDRSZ;

-aneesh

#5 lore raw
In reply to Jevon Qiao
Quoted 66 lines · Jevon Qiao

Hi Aneesh,

Thank you for reviewing my code, please see my reply in-line.

Quoted 10 lines · Aneesh Kumar K.V
Quoted 6 lines · Jevon Qiao

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.

Quoted 42 lines
Quoted 42 lines

From: Jevon Qiao <[email protected]>
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 <[email protected]>
---
   hw/9pfs/virtio-9p.c | 19 ++++++++++++++++---
   1 file changed, 16 insertions(+), 3 deletions(-)
hw/9pfs/virtio-9p.c +5 -3
index f972731..005d3a8 100644
@@ -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
Quoted 8 lines

hw/9pfs/virtio-9p.c static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path) · line 1346

Quoted 6 lines
hw/9pfs/virtio-9p.c +6 -0
+     * 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.

Quoted 5 lines

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

author
#6 lore raw

Hi Aneesh,

Quoted 9 lines

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;

Yes, I know this.

Quoted 10 lines

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. ?

This will result in an alignment issue while mapping the I/O requested by client into pages in the function of p9_nr_pages().

   int p9_nr_pages(char *data, int len)
   {
            unsigned long start_page, end_page;
            start_page =  (unsigned long)data >> PAGE_SHIFT;
            end_page = ((unsigned long)data + len + PAGE_SIZE - 1) >>
   PAGE_SHIFT;
            return end_page - start_page;
   }

Please see the following experiment I did without the fix.

1) Start qemu with cephfs,

   $ qemu-system-x86_64 /root/CentOS---6.6-64bit---2015-03-06-a.qcow2
   -smp 4 -m 4096 -fsdev
   cephfs,security_model=passthrough,id=fsdev0,path=/ -device
   virtio-9p-pci,id=fs0,fsdev=fsdev0,mount_tag=cephfs --enable-kvm
   -nographic -net nic -net tap,ifname=tap0,script=no,downscript=no

2) Mount the fs in the guest.

[root@localhost ~]# mount -t 9p -o trans=virtio,version=9p2000.L cephfs /mnt
[root@localhost ~]# ls -lah /mnt/8kfile
-rw-r--r-- 1 root root 8.0K 2016-02-19 09:37 /mnt/8kfile

In this case, I used the default msize which is 8192(in Byte). Since cephfs is using 4M as the f_bsize, the iounit will be 8168 as P9_IOHDRSZ is equal to 24.

3) Run the following systemtap script to trace the paging result,

   [root@localhost ~]# cat p9_read.stp
   probe kernel.function("p9_virtio_zc_request").call
   {
        printf("p9_virtio_zc_request: inlen size is %d\n", int_arg(5));
   }

   probe kernel.function("p9_nr_pages").call
   {
        printf("p9_nr_pages: start_page = %ld\n", int_arg(1) >> 12);
        printf("p9_nr_pages: end_age = %ld\n", (int_arg(1) + 8168 +
   4096 -1) >> 12);
   }

4) The output I got when I copied out the file /mnt/8kfile to /tmp/ directory,

p9_virtio_zc_request: inlen size is 8168
p9_nr_pages: start_page = 34293757815
p9_nr_pages: end_age = 34293757818

Per the text in red(start_page = 34293757815, end_page = 34293757818), it turns out 8k data will be mapped into three pages. This could hurt the performance.

Actually, I enabled the cephfs debug functionality added by me to see how the data is distributed in this case, the result is as follows,

CEPHFS_DEBUG: cephfs_preadv iov_len=4096
CEPHFS_DEBUG: cephfs_preadv iov_len=4072
CEPHFS_DEBUG: cephfs_preadv iov_len=24

This patch aims to fix this. And the result turns out it works quite well, all the data is well aligned.

p9_virtio_zc_request: inlen size is 4096
p9_nr_pages: start_page = 34203171814
p9_nr_pages: end_age = 34203171815
p9_virtio_zc_request: inlen size is 4096
p9_nr_pages: start_page = 34203171815
p9_nr_pages: end_age = 34203171816

CEPHFS_DEBUG: cephfs_preadv iov_len=4096
CEPHFS_DEBUG: cephfs_preadv iov_len=4096

Thanks,
Jevon

Quoted 2 lines

-aneesh

author
#4 lore raw
In reply to Greg Kurz

Hi Greg,

Quoted 19 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines

From: Jevon Qiao <[email protected]>
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.

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Signed-off-by Jevon Qiao <[email protected]>
---
    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:

Quoted 7 lines

commit 60ce86c7140d5ca33d5fd87ce821681165d06b2a

Author: Wei Liu <[email protected]>
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.

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
    1 file changed, 16 insertions(+), 3 deletions(-)
hw/9pfs/virtio-9p.c +1 -1
index f972731..005d3a8 100644
@@ -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;
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
hw/9pfs/virtio-9p.c +0 -0
        /*

@@ -1334,8 +1334,21 @@ static int32_t get_iounit(V9fsPDU *pdu, V9fsPath
 *path)
         * and as well as less than (client msize - P9_IOHDRSZ))
         */
Quoted 8 lines
Quoted 8 lines

hw/9pfs/virtio-9p.c static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path) · line 1341

Quoted 8 lines
hw/9pfs/virtio-9p.c +5 -2
        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.

Quoted 7 lines
Quoted 5 lines
Quoted 5 lines

hw/9pfs/virtio-9p.c static int32_t get_iounit(V9fsPDU *pdu, V9fsPath *path) · line 1346

Quoted 5 lines
hw/9pfs/virtio-9p.c +5 -0
+     * 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.

Quoted 13 lines
Quoted 8 lines
Quoted 2 lines

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.

Quoted 5 lines

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?

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 13 lines
Quoted 13 lines

Thanks,
Jevon

Quoted 11 lines
Quoted 9 lines
hw/9pfs/virtio-9p.c +5 -0
+    } else {
+            iounit = stbuf.f_bsize;
+        unit = stbuf.f_bsize;
+    }
+        iounit *= (s->msize - P9_IOHDRSZ)/unit;
        }
        if (!iounit) {

            iounit = s->msize - P9_IOHDRSZ;

--

-aneesh

author
#7 lore raw
In reply to Jevon Qiao

[Removing ceph-devel alias]

Hi Aneesh,

Any further comment on my reply below?

Thanks,
Jevon

Quoted 106 lines · Jevon Qiao

Hi Aneesh,

Quoted 9 lines

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;

Yes, I know this.

Quoted 10 lines

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. ?

This will result in an alignment issue while mapping the I/O requested by client into pages in the function of p9_nr_pages().

   int p9_nr_pages(char *data, int len)
   {
            unsigned long start_page, end_page;
            start_page =  (unsigned long)data >> PAGE_SHIFT;
            end_page = ((unsigned long)data + len + PAGE_SIZE - 1) >>
   PAGE_SHIFT;
            return end_page - start_page;
   }

Please see the following experiment I did without the fix.

1) Start qemu with cephfs,

   $ qemu-system-x86_64 /root/CentOS---6.6-64bit---2015-03-06-a.qcow2
   -smp 4 -m 4096 -fsdev
   cephfs,security_model=passthrough,id=fsdev0,path=/ -device
   virtio-9p-pci,id=fs0,fsdev=fsdev0,mount_tag=cephfs --enable-kvm
   -nographic -net nic -net tap,ifname=tap0,script=no,downscript=no

2) Mount the fs in the guest.

   [root@localhost ~]# mount -t 9p -o trans=virtio,version=9p2000.L
   cephfs /mnt
   [root@localhost ~]# ls -lah /mnt/8kfile
   -rw-r--r-- 1 root root 8.0K 2016-02-19 09:37 /mnt/8kfile

In this case, I used the default msize which is 8192(in Byte). Since cephfs is using 4M as the f_bsize, the iounit will be 8168 as P9_IOHDRSZ is equal to 24.

3) Run the following systemtap script to trace the paging result,

   [root@localhost ~]# cat p9_read.stp
   probe kernel.function("p9_virtio_zc_request").call
   {
        printf("p9_virtio_zc_request: inlen size is %d\n", int_arg(5));
   }

   probe kernel.function("p9_nr_pages").call
   {
        printf("p9_nr_pages: start_page = %ld\n", int_arg(1) >> 12);
        printf("p9_nr_pages: end_age = %ld\n", (int_arg(1) + 8168 +
   4096 -1) >> 12);
   }

4) The output I got when I copied out the file /mnt/8kfile to /tmp/ directory,

   p9_virtio_zc_request: inlen size is 8168
   p9_nr_pages: start_page = 34293757815
   p9_nr_pages: end_age = 34293757818

Per the text in red(start_page = 34293757815, end_page = 34293757818), it turns out 8k data will be mapped into three pages. This could hurt the performance.

Actually, I enabled the cephfs debug functionality added by me to see how the data is distributed in this case, the result is as follows,

CEPHFS_DEBUG: cephfs_preadv iov_len=4096
CEPHFS_DEBUG: cephfs_preadv iov_len=4072
CEPHFS_DEBUG: cephfs_preadv iov_len=24

This patch aims to fix this. And the result turns out it works quite well, all the data is well aligned.

p9_virtio_zc_request: inlen size is 4096
p9_nr_pages: start_page = 34203171814
p9_nr_pages: end_age = 34203171815
p9_virtio_zc_request: inlen size is 4096
p9_nr_pages: start_page = 34203171815
p9_nr_pages: end_age = 34203171816

CEPHFS_DEBUG: cephfs_preadv iov_len=4096
CEPHFS_DEBUG: cephfs_preadv iov_len=4096

Thanks,
Jevon

Quoted 2 lines

-aneesh

author
#8 lore raw

Any further question/comment on this patch?

Thanks,
Jevon

Quoted 116 lines · Jevon Qiao

[Removing ceph-devel alias]

Hi Aneesh,

Any further comment on my reply below?

Thanks,
Jevon

Quoted 106 lines · Jevon Qiao

Hi Aneesh,

Quoted 9 lines

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;

Yes, I know this.

Quoted 10 lines

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. ?

This will result in an alignment issue while mapping the I/O requested by client into pages in the function of p9_nr_pages().

   int p9_nr_pages(char *data, int len)
   {
            unsigned long start_page, end_page;
            start_page =  (unsigned long)data >> PAGE_SHIFT;
            end_page = ((unsigned long)data + len + PAGE_SIZE - 1) >>
   PAGE_SHIFT;
            return end_page - start_page;
   }

Please see the following experiment I did without the fix.

1) Start qemu with cephfs,

   $ qemu-system-x86_64 /root/CentOS---6.6-64bit---2015-03-06-a.qcow2
   -smp 4 -m 4096 -fsdev
   cephfs,security_model=passthrough,id=fsdev0,path=/ -device
   virtio-9p-pci,id=fs0,fsdev=fsdev0,mount_tag=cephfs --enable-kvm
   -nographic -net nic -net tap,ifname=tap0,script=no,downscript=no

2) Mount the fs in the guest.

   [root@localhost ~]# mount -t 9p -o trans=virtio,version=9p2000.L
   cephfs /mnt
   [root@localhost ~]# ls -lah /mnt/8kfile
   -rw-r--r-- 1 root root 8.0K 2016-02-19 09:37 /mnt/8kfile

In this case, I used the default msize which is 8192(in Byte). Since cephfs is using 4M as the f_bsize, the iounit will be 8168 as P9_IOHDRSZ is equal to 24.

3) Run the following systemtap script to trace the paging result,

   [root@localhost ~]# cat p9_read.stp
   probe kernel.function("p9_virtio_zc_request").call
   {
        printf("p9_virtio_zc_request: inlen size is %d\n", int_arg(5));
   }

   probe kernel.function("p9_nr_pages").call
   {
        printf("p9_nr_pages: start_page = %ld\n", int_arg(1) >> 12);
        printf("p9_nr_pages: end_age = %ld\n", (int_arg(1) + 8168 +
   4096 -1) >> 12);
   }

4) The output I got when I copied out the file /mnt/8kfile to /tmp/ directory,

   p9_virtio_zc_request: inlen size is 8168
   p9_nr_pages: start_page = 34293757815
   p9_nr_pages: end_age = 34293757818

Per the text in red(start_page = 34293757815, end_page = 34293757818), it turns out 8k data will be mapped into three pages. This could hurt the performance.

Actually, I enabled the cephfs debug functionality added by me to see how the data is distributed in this case, the result is as follows,

CEPHFS_DEBUG: cephfs_preadv iov_len=4096
CEPHFS_DEBUG: cephfs_preadv iov_len=4072
CEPHFS_DEBUG: cephfs_preadv iov_len=24

This patch aims to fix this. And the result turns out it works quite well, all the data is well aligned.

p9_virtio_zc_request: inlen size is 4096
p9_nr_pages: start_page = 34203171814
p9_nr_pages: end_age = 34203171815
p9_virtio_zc_request: inlen size is 4096
p9_nr_pages: start_page = 34203171815
p9_nr_pages: end_age = 34203171816

CEPHFS_DEBUG: cephfs_preadv iov_len=4096
CEPHFS_DEBUG: cephfs_preadv iov_len=4096

Thanks,
Jevon

Quoted 2 lines

-aneesh