[PATCH] block： change type of bytes from int to int64_t for *bdrv_aio_pdiscard

[PATCH] block： change type of bytes from int to int64_t for *bdrv_aio_pdiscard

[Sunny Zhu]

Keep it consistent with *bdrv_co_pdiscard.

Currently, there is no BlockDriver implemented the bdrv_aio_pdiscard() function,
so we don’t need to make any adaptations either.

Signed-off-by: Sunny Zhu <sunnyzhyy@qq.com>
---
 include/block/block_int-common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index ebb4e56a50..4bf422d733 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -507,7 +507,7 @@ struct BlockDriver {
         BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque);
 
     BlockAIOCB * GRAPH_RDLOCK_PTR (*bdrv_aio_pdiscard)(
-        BlockDriverState *bs, int64_t offset, int bytes,
+        BlockDriverState *bs, int64_t offset, int64_t bytes,
         BlockCompletionFunc *cb, void *opaque);
 
     int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_readv)(BlockDriverState *bs,
-- 
2.43.0

Re: [PATCH] block： change type of bytes from int to int64_t for *bdrv_aio_pdiscard

[Eric Blake]

On Mon, Apr 21, 2025 at 12:19:14AM +0800, Sunny Zhu wrote:
> Keep it consistent with *bdrv_co_pdiscard.
> 
> Currently, there is no BlockDriver implemented the bdrv_aio_pdiscard() function,
> so we don’t need to make any adaptations either.

If there are no drivers implementing the callback, then why have it?
I think we have been moving towards more coroutine-based callbacks and
away from the aio callbacks; if so, should we instead be deleting this
callback as stale code?

> 
> Signed-off-by: Sunny Zhu <sunnyzhyy@qq.com>
> ---
>  include/block/block_int-common.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
> index ebb4e56a50..4bf422d733 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -507,7 +507,7 @@ struct BlockDriver {
>          BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque);
>  
>      BlockAIOCB * GRAPH_RDLOCK_PTR (*bdrv_aio_pdiscard)(
> -        BlockDriverState *bs, int64_t offset, int bytes,
> +        BlockDriverState *bs, int64_t offset, int64_t bytes,
>          BlockCompletionFunc *cb, void *opaque);
>  
>      int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_readv)(BlockDriverState *bs,
> -- 
> 2.43.0
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH] block： change type of bytes from int to int64_t for *bdrv_aio_pdiscard

[Sunny Zhu]

Eric Blake wrote:
>On Mon, Apr 21, 2025 at 12:19:14AM +0800, Sunny Zhu wrote:
>> Keep it consistent with *bdrv_co_pdiscard.
>> 
>> Currently, there is no BlockDriver implemented the bdrv_aio_pdiscard() function,
>> so we don’t need to make any adaptations either.
>
>If there are no drivers implementing the callback, then why have it?
>I think we have been moving towards more coroutine-based callbacks and
>away from the aio callbacks; if so, should we instead be deleting this
>callback as stale code?

Yes, that makes sense. I will make the changes in the next version.

Thanks.

>
>> 
>> Signed-off-by: Sunny Zhu <sunnyzhyy@qq.com>
>> ---
>>  include/block/block_int-common.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
>> index ebb4e56a50..4bf422d733 100644
>> --- a/include/block/block_int-common.h
>> +++ b/include/block/block_int-common.h
>> @@ -507,7 +507,7 @@ struct BlockDriver {
>>          BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque);
>>  
>>      BlockAIOCB * GRAPH_RDLOCK_PTR (*bdrv_aio_pdiscard)(
>> -        BlockDriverState *bs, int64_t offset, int bytes,
>> +        BlockDriverState *bs, int64_t offset, int64_t bytes,
>>          BlockCompletionFunc *cb, void *opaque);
>>  
>>      int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_readv)(BlockDriverState *bs,
>> -- 
>> 2.43.0
>> 
>> 
>
>-- 
>Eric Blake, Principal Software Engineer
>Red Hat, Inc.
>Virtualization:  qemu.org | libguestfs.org

Re: [PATCH] block： change type of bytes from int to int64_t for *bdrv_aio_pdiscard

[Philippe Mathieu-Daudé]

Hi Eric,

On 21/4/25 17:03, Eric Blake wrote:
> On Mon, Apr 21, 2025 at 12:19:14AM +0800, Sunny Zhu wrote:
>> Keep it consistent with *bdrv_co_pdiscard.
>>
>> Currently, there is no BlockDriver implemented the bdrv_aio_pdiscard() function,
>> so we don’t need to make any adaptations either.
> 
> If there are no drivers implementing the callback, then why have it?
> I think we have been moving towards more coroutine-based callbacks and
> away from the aio callbacks; if so, should we instead be deleting this
> callback as stale code?

Could we add a comment in BlockDriver prototypes about prefering co over
aio implementations, possibly mentioning them as legacy?

> 
>>
>> Signed-off-by: Sunny Zhu <sunnyzhyy@qq.com>
>> ---
>>   include/block/block_int-common.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
>> index ebb4e56a50..4bf422d733 100644
>> --- a/include/block/block_int-common.h
>> +++ b/include/block/block_int-common.h
>> @@ -507,7 +507,7 @@ struct BlockDriver {
>>           BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque);
>>   
>>       BlockAIOCB * GRAPH_RDLOCK_PTR (*bdrv_aio_pdiscard)(
>> -        BlockDriverState *bs, int64_t offset, int bytes,
>> +        BlockDriverState *bs, int64_t offset, int64_t bytes,
>>           BlockCompletionFunc *cb, void *opaque);
>>   
>>       int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_readv)(BlockDriverState *bs,
>> -- 
>> 2.43.0
>>
>>
> 

Re: [PATCH] block： change type of bytes from int to int64_t for *bdrv_aio_pdiscard

[Eric Blake]

On Tue, Apr 22, 2025 at 09:11:51AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Eric,
> 
> On 21/4/25 17:03, Eric Blake wrote:
> > On Mon, Apr 21, 2025 at 12:19:14AM +0800, Sunny Zhu wrote:
> > > Keep it consistent with *bdrv_co_pdiscard.
> > > 
> > > Currently, there is no BlockDriver implemented the bdrv_aio_pdiscard() function,
> > > so we don’t need to make any adaptations either.
> > 
> > If there are no drivers implementing the callback, then why have it?
> > I think we have been moving towards more coroutine-based callbacks and
> > away from the aio callbacks; if so, should we instead be deleting this
> > callback as stale code?
> 
> Could we add a comment in BlockDriver prototypes about prefering co over
> aio implementations, possibly mentioning them as legacy?

Thinking about this a bit more (but you'll definitely want Kevin's
opinion, not just mine):

$ git grep '\.bdrv_aio_'
block/file-win32.c:    .bdrv_aio_preadv    = raw_aio_preadv,
block/file-win32.c:    .bdrv_aio_pwritev   = raw_aio_pwritev,
block/file-win32.c:    .bdrv_aio_flush     = raw_aio_flush,
block/file-win32.c:    .bdrv_aio_preadv    = raw_aio_preadv,
block/file-win32.c:    .bdrv_aio_pwritev   = raw_aio_pwritev,
block/file-win32.c:    .bdrv_aio_flush     = raw_aio_flush,
block/iscsi.c:    .bdrv_aio_ioctl   = iscsi_aio_ioctl,
block/iscsi.c:    .bdrv_aio_ioctl   = iscsi_aio_ioctl,
block/null.c:    .bdrv_aio_preadv        = null_aio_preadv,
block/null.c:    .bdrv_aio_pwritev       = null_aio_pwritev,
block/null.c:    .bdrv_aio_flush         = null_aio_flush,

file-win32.c looks to be the major client of remaining aio interfaces.
How hard would it be to convert those over to coroutines?

iscsi.c uses aio only for ioctl.  How hard would it be to convert it
in the same way that we converted read/write/flush back in commit
063c3378?

null.c provides aio interfaces solely for benchmarking purposes - but
if it is the only remaining client of aio interfaces, it would be nice
to just rip out support for null-aio: and rely solely on null:

It sounds like we are close enough to a generic cleanup of detritus
that it would be better to just finish the job than to add a comment
about preferring co over aio.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH] block： change type of bytes from int to int64_t for *bdrv_aio_pdiscard

[Kevin Wolf]

Am 24.04.2025 um 19:40 hat Eric Blake geschrieben:
> On Tue, Apr 22, 2025 at 09:11:51AM +0200, Philippe Mathieu-Daudé wrote:
> > Hi Eric,
> > 
> > On 21/4/25 17:03, Eric Blake wrote:
> > > On Mon, Apr 21, 2025 at 12:19:14AM +0800, Sunny Zhu wrote:
> > > > Keep it consistent with *bdrv_co_pdiscard.
> > > > 
> > > > Currently, there is no BlockDriver implemented the bdrv_aio_pdiscard() function,
> > > > so we don’t need to make any adaptations either.
> > > 
> > > If there are no drivers implementing the callback, then why have it?
> > > I think we have been moving towards more coroutine-based callbacks and
> > > away from the aio callbacks; if so, should we instead be deleting this
> > > callback as stale code?
> > 
> > Could we add a comment in BlockDriver prototypes about prefering co over
> > aio implementations, possibly mentioning them as legacy?
> 
> Thinking about this a bit more (but you'll definitely want Kevin's
> opinion, not just mine):
> 
> $ git grep '\.bdrv_aio_'
> block/file-win32.c:    .bdrv_aio_preadv    = raw_aio_preadv,
> block/file-win32.c:    .bdrv_aio_pwritev   = raw_aio_pwritev,
> block/file-win32.c:    .bdrv_aio_flush     = raw_aio_flush,
> block/file-win32.c:    .bdrv_aio_preadv    = raw_aio_preadv,
> block/file-win32.c:    .bdrv_aio_pwritev   = raw_aio_pwritev,
> block/file-win32.c:    .bdrv_aio_flush     = raw_aio_flush,
> block/iscsi.c:    .bdrv_aio_ioctl   = iscsi_aio_ioctl,
> block/iscsi.c:    .bdrv_aio_ioctl   = iscsi_aio_ioctl,
> block/null.c:    .bdrv_aio_preadv        = null_aio_preadv,
> block/null.c:    .bdrv_aio_pwritev       = null_aio_pwritev,
> block/null.c:    .bdrv_aio_flush         = null_aio_flush,
> 
> file-win32.c looks to be the major client of remaining aio interfaces.
> How hard would it be to convert those over to coroutines?

Not terribly complicated. In case of doubt, they remain callback based
internally and just yield until the callback reenters the coroutine.

For file-win32 specifically, paio_submit() can be modified to call
thread_pool_submit_co(), which already does this. The win32-aio paths
won't necessarily become simpler, but that's okay.

> iscsi.c uses aio only for ioctl.  How hard would it be to convert it
> in the same way that we converted read/write/flush back in commit
> 063c3378?

iscsi_aio_ioctl() could probably be simplified a bit when the callback
is moved into the function itself, especially for the case of
iscsi_ioctl_handle_emulated().

> null.c provides aio interfaces solely for benchmarking purposes - but
> if it is the only remaining client of aio interfaces, it would be nice
> to just rip out support for null-aio: and rely solely on null:

null-aio is specifically an AIO based version, I think mainly to test
performance of AIO vs. coroutines. If we get rid of the AIO interfaces,
then we can probably just remove the whole null-aio driver. (Or rather,
make it an alias of null-co for now and deprecate it.)

> It sounds like we are close enough to a generic cleanup of detritus
> that it would be better to just finish the job than to add a comment
> about preferring co over aio.

Agreed.

Kevin