diff mbox series

[RFC,6/6] block: Add a thread-pool version of fstat

Message ID 20230523213903.18418-7-farosas@suse.de
State New
Headers show
Series block: Make raw_co_get_allocated_file_size asynchronous | expand

Commit Message

Fabiano Rosas May 23, 2023, 9:39 p.m. UTC 
From: João Silva <jsilva@suse.de>

The fstat call can take a long time to finish when running over
NFS. Add a version of it that runs in the thread pool.

Adapt one of its users, raw_co_get_allocated_file size to use the new
version. That function is called via QMP under the qemu_global_mutex
so it has a large chance of blocking VCPU threads in case it takes too
long to finish.

Signed-off-by: João Silva <jsilva@suse.de>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 block/file-posix.c      | 40 +++++++++++++++++++++++++++++++++++++---
 include/block/raw-aio.h |  4 +++-
 2 files changed, 40 insertions(+), 4 deletions(-)

Comments

Claudio Fontana May 24, 2023, 3:56 p.m. UTC | #1 
On 5/23/23 23:39, Fabiano Rosas wrote:
> From: João Silva <jsilva@suse.de>
> 
> The fstat call can take a long time to finish when running over
> NFS. Add a version of it that runs in the thread pool.
> 
> Adapt one of its users, raw_co_get_allocated_file size to use the new
> version. That function is called via QMP under the qemu_global_mutex
> so it has a large chance of blocking VCPU threads in case it takes too
> long to finish.
> 
> Signed-off-by: João Silva <jsilva@suse.de>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  block/file-posix.c      | 40 +++++++++++++++++++++++++++++++++++++---
>  include/block/raw-aio.h |  4 +++-
>  2 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 0ab158efba..0a29a6cc43 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -227,6 +227,9 @@ typedef struct RawPosixAIOData {
>          struct {
>              unsigned long op;
>          } zone_mgmt;
> +        struct {
> +            struct stat *st;
> +        } fstat;
>      };
>  } RawPosixAIOData;
>  
> @@ -2644,6 +2647,34 @@ static void raw_close(BlockDriverState *bs)
>      }
>  }
>  
> +static int handle_aiocb_fstat(void *opaque)
> +{
> +    RawPosixAIOData *aiocb = opaque;
> +
> +    if (fstat(aiocb->aio_fildes, aiocb->fstat.st) < 0) {
> +        return -errno;
> +    }
> +
> +    return 0;
> +}
> +
> +static int coroutine_fn raw_co_fstat(BlockDriverState *bs, struct stat *st)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    RawPosixAIOData acb;
> +
> +    acb = (RawPosixAIOData) {
> +        .bs             = bs,
> +        .aio_fildes     = s->fd,
> +        .aio_type       = QEMU_AIO_FSTAT,
> +        .fstat          = {
> +            .st = st,
> +        },
> +    };
> +
> +    return raw_thread_pool_submit(handle_aiocb_fstat, &acb);
> +}
> +
>  /**
>   * Truncates the given regular file @fd to @offset and, when growing, fills the
>   * new space according to @prealloc.
> @@ -2883,11 +2914,14 @@ static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)
>  static int64_t coroutine_fn raw_co_get_allocated_file_size(BlockDriverState *bs)
>  {
>      struct stat st;
> -    BDRVRawState *s = bs->opaque;
> +    int ret;
>  
> -    if (fstat(s->fd, &st) < 0) {
> -        return -errno;
> +    ret = raw_co_fstat(bs, &st);
> +
> +    if (ret) {
> +        return ret;
>      }
> +
>      return (int64_t)st.st_blocks * 512;
>  }
>  
> diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
> index 0fe85ade77..7dc6d24e21 100644
> --- a/include/block/raw-aio.h
> +++ b/include/block/raw-aio.h
> @@ -31,6 +31,7 @@
>  #define QEMU_AIO_ZONE_REPORT  0x0100
>  #define QEMU_AIO_ZONE_MGMT    0x0200
>  #define QEMU_AIO_ZONE_APPEND  0x0400
> +#define QEMU_AIO_FSTAT        0x0800
>  #define QEMU_AIO_TYPE_MASK \
>          (QEMU_AIO_READ | \
>           QEMU_AIO_WRITE | \
> @@ -42,7 +43,8 @@
>           QEMU_AIO_TRUNCATE | \
>           QEMU_AIO_ZONE_REPORT | \
>           QEMU_AIO_ZONE_MGMT | \
> -         QEMU_AIO_ZONE_APPEND)
> +         QEMU_AIO_ZONE_APPEND | \
> +         QEMU_AIO_FSTAT)
>  
>  /* AIO flags */
>  #define QEMU_AIO_MISALIGNED   0x1000


Reviewed-by: Claudio Fontana <cfontana@suse.de>
Eric Blake May 25, 2023, 3:45 p.m. UTC | #2 
On Tue, May 23, 2023 at 06:39:03PM -0300, Fabiano Rosas wrote:
> From: João Silva <jsilva@suse.de>
> 
> The fstat call can take a long time to finish when running over
> NFS. Add a version of it that runs in the thread pool.
> 
> Adapt one of its users, raw_co_get_allocated_file size to use the new
> version. That function is called via QMP under the qemu_global_mutex
> so it has a large chance of blocking VCPU threads in case it takes too
> long to finish.
> 
> Signed-off-by: João Silva <jsilva@suse.de>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  block/file-posix.c      | 40 +++++++++++++++++++++++++++++++++++++---
>  include/block/raw-aio.h |  4 +++-
>  2 files changed, 40 insertions(+), 4 deletions(-)

Should this change occur earlier in the series, before calling
commands are marked with QAPI coroutine flags?  Otherwise, you have a
bisection bug, where something marked coroutine can end up hanging
when it calls a blocking syscall in the wrong context without the help
of this patch offloading the syscall into a helper thread.
Fabiano Rosas May 26, 2023, 2:20 p.m. UTC | #3 
Eric Blake <eblake@redhat.com> writes:

> On Tue, May 23, 2023 at 06:39:03PM -0300, Fabiano Rosas wrote:
>> From: João Silva <jsilva@suse.de>
>> 
>> The fstat call can take a long time to finish when running over
>> NFS. Add a version of it that runs in the thread pool.
>> 
>> Adapt one of its users, raw_co_get_allocated_file size to use the new
>> version. That function is called via QMP under the qemu_global_mutex
>> so it has a large chance of blocking VCPU threads in case it takes too
>> long to finish.
>> 
>> Signed-off-by: João Silva <jsilva@suse.de>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  block/file-posix.c      | 40 +++++++++++++++++++++++++++++++++++++---
>>  include/block/raw-aio.h |  4 +++-
>>  2 files changed, 40 insertions(+), 4 deletions(-)
>
> Should this change occur earlier in the series, before calling
> commands are marked with QAPI coroutine flags?  Otherwise, you have a
> bisection bug, where something marked coroutine can end up hanging
> when it calls a blocking syscall in the wrong context without the help
> of this patch offloading the syscall into a helper thread.

Hmm, I'm not sure. To submit the work to the thread pool we need to be
in a coroutine already. If the syscall blocks for too long we'd be
trading blocking the coroutine vs. blocking a vcpu thread anyway.

I have tested each patch to avoid bisection issues, but maybe it would
be warranted to merge both parts into a single patch. Or arrange them in
some other way... I'll experiment with it.
diff mbox series

Patch

diff --git a/block/file-posix.c b/block/file-posix.c
index 0ab158efba..0a29a6cc43 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -227,6 +227,9 @@  typedef struct RawPosixAIOData {
         struct {
             unsigned long op;
         } zone_mgmt;
+        struct {
+            struct stat *st;
+        } fstat;
     };
 } RawPosixAIOData;
 
@@ -2644,6 +2647,34 @@  static void raw_close(BlockDriverState *bs)
     }
 }
 
+static int handle_aiocb_fstat(void *opaque)
+{
+    RawPosixAIOData *aiocb = opaque;
+
+    if (fstat(aiocb->aio_fildes, aiocb->fstat.st) < 0) {
+        return -errno;
+    }
+
+    return 0;
+}
+
+static int coroutine_fn raw_co_fstat(BlockDriverState *bs, struct stat *st)
+{
+    BDRVRawState *s = bs->opaque;
+    RawPosixAIOData acb;
+
+    acb = (RawPosixAIOData) {
+        .bs             = bs,
+        .aio_fildes     = s->fd,
+        .aio_type       = QEMU_AIO_FSTAT,
+        .fstat          = {
+            .st = st,
+        },
+    };
+
+    return raw_thread_pool_submit(handle_aiocb_fstat, &acb);
+}
+
 /**
  * Truncates the given regular file @fd to @offset and, when growing, fills the
  * new space according to @prealloc.
@@ -2883,11 +2914,14 @@  static int64_t coroutine_fn raw_co_getlength(BlockDriverState *bs)
 static int64_t coroutine_fn raw_co_get_allocated_file_size(BlockDriverState *bs)
 {
     struct stat st;
-    BDRVRawState *s = bs->opaque;
+    int ret;
 
-    if (fstat(s->fd, &st) < 0) {
-        return -errno;
+    ret = raw_co_fstat(bs, &st);
+
+    if (ret) {
+        return ret;
     }
+
     return (int64_t)st.st_blocks * 512;
 }
 
diff --git a/include/block/raw-aio.h b/include/block/raw-aio.h
index 0fe85ade77..7dc6d24e21 100644
--- a/include/block/raw-aio.h
+++ b/include/block/raw-aio.h
@@ -31,6 +31,7 @@ 
 #define QEMU_AIO_ZONE_REPORT  0x0100
 #define QEMU_AIO_ZONE_MGMT    0x0200
 #define QEMU_AIO_ZONE_APPEND  0x0400
+#define QEMU_AIO_FSTAT        0x0800
 #define QEMU_AIO_TYPE_MASK \
         (QEMU_AIO_READ | \
          QEMU_AIO_WRITE | \
@@ -42,7 +43,8 @@ 
          QEMU_AIO_TRUNCATE | \
          QEMU_AIO_ZONE_REPORT | \
          QEMU_AIO_ZONE_MGMT | \
-         QEMU_AIO_ZONE_APPEND)
+         QEMU_AIO_ZONE_APPEND | \
+         QEMU_AIO_FSTAT)
 
 /* AIO flags */
 #define QEMU_AIO_MISALIGNED   0x1000