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