All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] virtiofsd: Reverse req_list before processing it
@ 2021-08-24 13:11 ` Sergio Lopez
  0 siblings, 0 replies; 6+ messages in thread
From: Sergio Lopez @ 2021-08-24 13:11 UTC (permalink / raw)
  To: qemu-devel
  Cc: virtio-fs, Sergio Lopez, Dr. David Alan Gilbert, Stefan Hajnoczi, vgoyal

With the thread pool disabled, we add the requests in the queue to a
GList, processing by iterating over there afterwards.

For adding them, we're using "g_list_prepend()", which is more
efficient but causes the requests to be processed in reverse order,
breaking the read-ahead and request-merging optimizations in the host
for sequential operations.

According to the documentation, if you need to process the request
in-order, using "g_list_prepend()" and then reversing the list with
"g_list_reverse()" is more efficient than using "g_list_append()", so
let's do it that way.

Testing on a spinning disk (to boost the increase of read-ahead and
request-merging) shows a 4x improvement on sequential write fio test:

Test:
fio --directory=/mnt/virtio-fs --filename=fio-file1 --runtime=20
--iodepth=16 --size=4G --direct=1 --blocksize=4K --ioengine libaio
--rw write --name seqwrite-libaio

Without "g_list_reverse()":
...
Jobs: 1 (f=1): [W(1)][100.0%][w=22.4MiB/s][w=5735 IOPS][eta 00m:00s]
seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=710: Tue Aug 24 12:58:16 2021
  write: IOPS=5709, BW=22.3MiB/s (23.4MB/s)(446MiB/20002msec); 0 zone resets
...

With "g_list_reverse()":
...
Jobs: 1 (f=1): [W(1)][100.0%][w=84.0MiB/s][w=21.5k IOPS][eta 00m:00s]
seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=716: Tue Aug 24 13:00:15 2021
  write: IOPS=21.3k, BW=83.1MiB/s (87.2MB/s)(1663MiB/20001msec); 0 zone resets
...

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index fc2564a603..8f4fd165b9 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -716,6 +716,7 @@ static void *fv_queue_thread(void *opaque)
 
         /* Process all the requests. */
         if (!se->thread_pool_size && req_list != NULL) {
+            req_list = g_list_reverse(req_list);
             g_list_foreach(req_list, fv_queue_worker, qi);
             g_list_free(req_list);
             req_list = NULL;
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [Virtio-fs] [PATCH] virtiofsd: Reverse req_list before processing it
@ 2021-08-24 13:11 ` Sergio Lopez
  0 siblings, 0 replies; 6+ messages in thread
From: Sergio Lopez @ 2021-08-24 13:11 UTC (permalink / raw)
  To: qemu-devel; +Cc: virtio-fs, vgoyal

With the thread pool disabled, we add the requests in the queue to a
GList, processing by iterating over there afterwards.

For adding them, we're using "g_list_prepend()", which is more
efficient but causes the requests to be processed in reverse order,
breaking the read-ahead and request-merging optimizations in the host
for sequential operations.

According to the documentation, if you need to process the request
in-order, using "g_list_prepend()" and then reversing the list with
"g_list_reverse()" is more efficient than using "g_list_append()", so
let's do it that way.

Testing on a spinning disk (to boost the increase of read-ahead and
request-merging) shows a 4x improvement on sequential write fio test:

Test:
fio --directory=/mnt/virtio-fs --filename=fio-file1 --runtime=20
--iodepth=16 --size=4G --direct=1 --blocksize=4K --ioengine libaio
--rw write --name seqwrite-libaio

Without "g_list_reverse()":
...
Jobs: 1 (f=1): [W(1)][100.0%][w=22.4MiB/s][w=5735 IOPS][eta 00m:00s]
seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=710: Tue Aug 24 12:58:16 2021
  write: IOPS=5709, BW=22.3MiB/s (23.4MB/s)(446MiB/20002msec); 0 zone resets
...

With "g_list_reverse()":
...
Jobs: 1 (f=1): [W(1)][100.0%][w=84.0MiB/s][w=21.5k IOPS][eta 00m:00s]
seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=716: Tue Aug 24 13:00:15 2021
  write: IOPS=21.3k, BW=83.1MiB/s (87.2MB/s)(1663MiB/20001msec); 0 zone resets
...

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 tools/virtiofsd/fuse_virtio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
index fc2564a603..8f4fd165b9 100644
--- a/tools/virtiofsd/fuse_virtio.c
+++ b/tools/virtiofsd/fuse_virtio.c
@@ -716,6 +716,7 @@ static void *fv_queue_thread(void *opaque)
 
         /* Process all the requests. */
         if (!se->thread_pool_size && req_list != NULL) {
+            req_list = g_list_reverse(req_list);
             g_list_foreach(req_list, fv_queue_worker, qi);
             g_list_free(req_list);
             req_list = NULL;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] virtiofsd: Reverse req_list before processing it
  2021-08-24 13:11 ` [Virtio-fs] " Sergio Lopez
@ 2021-08-24 13:38   ` Vivek Goyal
  -1 siblings, 0 replies; 6+ messages in thread
From: Vivek Goyal @ 2021-08-24 13:38 UTC (permalink / raw)
  To: Sergio Lopez
  Cc: virtio-fs, qemu-devel, Stefan Hajnoczi, Dr. David Alan Gilbert

On Tue, Aug 24, 2021 at 03:11:58PM +0200, Sergio Lopez wrote:
> With the thread pool disabled, we add the requests in the queue to a
> GList, processing by iterating over there afterwards.
> 
> For adding them, we're using "g_list_prepend()", which is more
> efficient but causes the requests to be processed in reverse order,
> breaking the read-ahead and request-merging optimizations in the host
> for sequential operations.
> 
> According to the documentation, if you need to process the request
> in-order, using "g_list_prepend()" and then reversing the list with
> "g_list_reverse()" is more efficient than using "g_list_append()", so
> let's do it that way.
> 
> Testing on a spinning disk (to boost the increase of read-ahead and
> request-merging) shows a 4x improvement on sequential write fio test:
> 
> Test:
> fio --directory=/mnt/virtio-fs --filename=fio-file1 --runtime=20
> --iodepth=16 --size=4G --direct=1 --blocksize=4K --ioengine libaio
> --rw write --name seqwrite-libaio
> 
> Without "g_list_reverse()":
> ...
> Jobs: 1 (f=1): [W(1)][100.0%][w=22.4MiB/s][w=5735 IOPS][eta 00m:00s]
> seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=710: Tue Aug 24 12:58:16 2021
>   write: IOPS=5709, BW=22.3MiB/s (23.4MB/s)(446MiB/20002msec); 0 zone resets
> ...
> 
> With "g_list_reverse()":
> ...
> Jobs: 1 (f=1): [W(1)][100.0%][w=84.0MiB/s][w=21.5k IOPS][eta 00m:00s]
> seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=716: Tue Aug 24 13:00:15 2021
>   write: IOPS=21.3k, BW=83.1MiB/s (87.2MB/s)(1663MiB/20001msec); 0 zone resets
> ...
> 

That's a very impressive improvememnt. Thanks Sergio for fixing this.

Reviewed-by: Vivek Goyal <vgoyal@redhat.com>

Vivek

> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
>  tools/virtiofsd/fuse_virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index fc2564a603..8f4fd165b9 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -716,6 +716,7 @@ static void *fv_queue_thread(void *opaque)
>  
>          /* Process all the requests. */
>          if (!se->thread_pool_size && req_list != NULL) {
> +            req_list = g_list_reverse(req_list);
>              g_list_foreach(req_list, fv_queue_worker, qi);
>              g_list_free(req_list);
>              req_list = NULL;
> -- 
> 2.26.2
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: Reverse req_list before processing it
@ 2021-08-24 13:38   ` Vivek Goyal
  0 siblings, 0 replies; 6+ messages in thread
From: Vivek Goyal @ 2021-08-24 13:38 UTC (permalink / raw)
  To: Sergio Lopez; +Cc: virtio-fs, qemu-devel

On Tue, Aug 24, 2021 at 03:11:58PM +0200, Sergio Lopez wrote:
> With the thread pool disabled, we add the requests in the queue to a
> GList, processing by iterating over there afterwards.
> 
> For adding them, we're using "g_list_prepend()", which is more
> efficient but causes the requests to be processed in reverse order,
> breaking the read-ahead and request-merging optimizations in the host
> for sequential operations.
> 
> According to the documentation, if you need to process the request
> in-order, using "g_list_prepend()" and then reversing the list with
> "g_list_reverse()" is more efficient than using "g_list_append()", so
> let's do it that way.
> 
> Testing on a spinning disk (to boost the increase of read-ahead and
> request-merging) shows a 4x improvement on sequential write fio test:
> 
> Test:
> fio --directory=/mnt/virtio-fs --filename=fio-file1 --runtime=20
> --iodepth=16 --size=4G --direct=1 --blocksize=4K --ioengine libaio
> --rw write --name seqwrite-libaio
> 
> Without "g_list_reverse()":
> ...
> Jobs: 1 (f=1): [W(1)][100.0%][w=22.4MiB/s][w=5735 IOPS][eta 00m:00s]
> seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=710: Tue Aug 24 12:58:16 2021
>   write: IOPS=5709, BW=22.3MiB/s (23.4MB/s)(446MiB/20002msec); 0 zone resets
> ...
> 
> With "g_list_reverse()":
> ...
> Jobs: 1 (f=1): [W(1)][100.0%][w=84.0MiB/s][w=21.5k IOPS][eta 00m:00s]
> seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=716: Tue Aug 24 13:00:15 2021
>   write: IOPS=21.3k, BW=83.1MiB/s (87.2MB/s)(1663MiB/20001msec); 0 zone resets
> ...
> 

That's a very impressive improvememnt. Thanks Sergio for fixing this.

Reviewed-by: Vivek Goyal <vgoyal@redhat.com>

Vivek

> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
>  tools/virtiofsd/fuse_virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index fc2564a603..8f4fd165b9 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -716,6 +716,7 @@ static void *fv_queue_thread(void *opaque)
>  
>          /* Process all the requests. */
>          if (!se->thread_pool_size && req_list != NULL) {
> +            req_list = g_list_reverse(req_list);
>              g_list_foreach(req_list, fv_queue_worker, qi);
>              g_list_free(req_list);
>              req_list = NULL;
> -- 
> 2.26.2
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] virtiofsd: Reverse req_list before processing it
  2021-08-24 13:11 ` [Virtio-fs] " Sergio Lopez
@ 2021-09-16  9:55   ` Dr. David Alan Gilbert
  -1 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2021-09-16  9:55 UTC (permalink / raw)
  To: Sergio Lopez; +Cc: virtio-fs, qemu-devel, Stefan Hajnoczi, vgoyal

* Sergio Lopez (slp@redhat.com) wrote:
> With the thread pool disabled, we add the requests in the queue to a
> GList, processing by iterating over there afterwards.
> 
> For adding them, we're using "g_list_prepend()", which is more
> efficient but causes the requests to be processed in reverse order,
> breaking the read-ahead and request-merging optimizations in the host
> for sequential operations.
> 
> According to the documentation, if you need to process the request
> in-order, using "g_list_prepend()" and then reversing the list with
> "g_list_reverse()" is more efficient than using "g_list_append()", so
> let's do it that way.
> 
> Testing on a spinning disk (to boost the increase of read-ahead and
> request-merging) shows a 4x improvement on sequential write fio test:
> 
> Test:
> fio --directory=/mnt/virtio-fs --filename=fio-file1 --runtime=20
> --iodepth=16 --size=4G --direct=1 --blocksize=4K --ioengine libaio
> --rw write --name seqwrite-libaio
> 
> Without "g_list_reverse()":
> ...
> Jobs: 1 (f=1): [W(1)][100.0%][w=22.4MiB/s][w=5735 IOPS][eta 00m:00s]
> seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=710: Tue Aug 24 12:58:16 2021
>   write: IOPS=5709, BW=22.3MiB/s (23.4MB/s)(446MiB/20002msec); 0 zone resets
> ...
> 
> With "g_list_reverse()":
> ...
> Jobs: 1 (f=1): [W(1)][100.0%][w=84.0MiB/s][w=21.5k IOPS][eta 00m:00s]
> seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=716: Tue Aug 24 13:00:15 2021
>   write: IOPS=21.3k, BW=83.1MiB/s (87.2MB/s)(1663MiB/20001msec); 0 zone resets
> ...
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>

Queued

> ---
>  tools/virtiofsd/fuse_virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index fc2564a603..8f4fd165b9 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -716,6 +716,7 @@ static void *fv_queue_thread(void *opaque)
>  
>          /* Process all the requests. */
>          if (!se->thread_pool_size && req_list != NULL) {
> +            req_list = g_list_reverse(req_list);
>              g_list_foreach(req_list, fv_queue_worker, qi);
>              g_list_free(req_list);
>              req_list = NULL;
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Virtio-fs] [PATCH] virtiofsd: Reverse req_list before processing it
@ 2021-09-16  9:55   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2021-09-16  9:55 UTC (permalink / raw)
  To: Sergio Lopez; +Cc: virtio-fs, qemu-devel, vgoyal

* Sergio Lopez (slp@redhat.com) wrote:
> With the thread pool disabled, we add the requests in the queue to a
> GList, processing by iterating over there afterwards.
> 
> For adding them, we're using "g_list_prepend()", which is more
> efficient but causes the requests to be processed in reverse order,
> breaking the read-ahead and request-merging optimizations in the host
> for sequential operations.
> 
> According to the documentation, if you need to process the request
> in-order, using "g_list_prepend()" and then reversing the list with
> "g_list_reverse()" is more efficient than using "g_list_append()", so
> let's do it that way.
> 
> Testing on a spinning disk (to boost the increase of read-ahead and
> request-merging) shows a 4x improvement on sequential write fio test:
> 
> Test:
> fio --directory=/mnt/virtio-fs --filename=fio-file1 --runtime=20
> --iodepth=16 --size=4G --direct=1 --blocksize=4K --ioengine libaio
> --rw write --name seqwrite-libaio
> 
> Without "g_list_reverse()":
> ...
> Jobs: 1 (f=1): [W(1)][100.0%][w=22.4MiB/s][w=5735 IOPS][eta 00m:00s]
> seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=710: Tue Aug 24 12:58:16 2021
>   write: IOPS=5709, BW=22.3MiB/s (23.4MB/s)(446MiB/20002msec); 0 zone resets
> ...
> 
> With "g_list_reverse()":
> ...
> Jobs: 1 (f=1): [W(1)][100.0%][w=84.0MiB/s][w=21.5k IOPS][eta 00m:00s]
> seqwrite-libaio: (groupid=0, jobs=1): err= 0: pid=716: Tue Aug 24 13:00:15 2021
>   write: IOPS=21.3k, BW=83.1MiB/s (87.2MB/s)(1663MiB/20001msec); 0 zone resets
> ...
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>

Queued

> ---
>  tools/virtiofsd/fuse_virtio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index fc2564a603..8f4fd165b9 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -716,6 +716,7 @@ static void *fv_queue_thread(void *opaque)
>  
>          /* Process all the requests. */
>          if (!se->thread_pool_size && req_list != NULL) {
> +            req_list = g_list_reverse(req_list);
>              g_list_foreach(req_list, fv_queue_worker, qi);
>              g_list_free(req_list);
>              req_list = NULL;
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-16  9:57 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-24 13:11 [PATCH] virtiofsd: Reverse req_list before processing it Sergio Lopez
2021-08-24 13:11 ` [Virtio-fs] " Sergio Lopez
2021-08-24 13:38 ` Vivek Goyal
2021-08-24 13:38   ` [Virtio-fs] " Vivek Goyal
2021-09-16  9:55 ` Dr. David Alan Gilbert
2021-09-16  9:55   ` [Virtio-fs] " Dr. David Alan Gilbert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.