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

[Kevin Wolf <kwolf@redhat.com>]

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