Re: [PATCH 02/26] block: add missing coroutine_fn annotations
On Sat, Sep 24, 2022 at 2:42 PM Paolo Bonzini wrote:
> Because at the time I wrote the patch, blk_pwrite_zeroes() was a
> coroutine_fn. :)
>
> It is called from bdrv_co_create_opts_simple which is coroutine_fn and
> performs I/O, so it should be a coroutine_fn. I have a few more patches
> to not go through the generated_co_wrappers, but I was curious to see
> if we could automate those changes through your tool.
The static analyzer [1] should find all such cases, e.g.:
./static-analyzer.py -c no_coroutine_fn build block
coroutine_fn and generated_co_wrapper must be adjusted for this to
work on master:
#define coroutine_fn __attribute__((__annotate__("coroutine_fn")))
#define generated_co_wrapper
__attribute__((__annotate__("no_coroutine_fn")))
[1] https://gitlab.com/albertofaria/qemu/-/tree/static-analysis
Re: [PATCH v5 11/12] blkio: implement BDRV_REQ_REGISTERED_BUF optimization
On Wed, Sep 28, 2022 at 8:21 PM Stefan Hajnoczi wrote:
> On Tue, Sep 27, 2022 at 03:34:30PM -0400, Stefan Hajnoczi wrote:
> > +ret = blkio_get_bool(s->blkio,
> > + "mem-regions-pinned",
> > + >mem_regions_pinned);
> > +if (ret < 0) {
> > +/* Be conservative (assume pinning) if the property is not
> > supported */
> > +s->mem_regions_pinned = true;
>
> This is too conservative :). It can be changed to:
>
> s->mem_regions_pinned = s->needs_mem_regions;
>
> That way we avoid ram_block_discard_disable() for libblkio drivers (like
> io_uring in libblkio 1.0) that don't use memory regions and don't
> support the "mem-regions-pinned" property yet.
Even if a driver doesn't _need_ memory regions to be mapped before
use, it may still do something special with the ones that _are_
mapped, so we may have no choice but to set s->mem_regions_pinned =
true.
(Unless we are assuming that all future libblkio versions will either
not have such drivers, or will provide a "mem-regions-pinned"
property, but that feels brittle.)
Alberto
Re: [PATCH 02/26] block: add missing coroutine_fn annotations
On Thu, Sep 22, 2022 at 9:49 AM Paolo Bonzini wrote: > Callers of coroutine_fn must be coroutine_fn themselves, or the call > must be within "if (qemu_in_coroutine())". Apply coroutine_fn to > functions where this holds. > > Signed-off-by: Paolo Bonzini > --- > block.c | 6 +++--- > block/block-backend.c | 10 +- > block/io.c| 22 +++--- > 3 files changed, 19 insertions(+), 19 deletions(-) > > diff --git a/block.c b/block.c > index bc85f46eed..5c34ada53f 100644 > --- a/block.c > +++ b/block.c > @@ -631,9 +631,9 @@ static int64_t create_file_fallback_truncate(BlockBackend > *blk, > * Helper function for bdrv_create_file_fallback(): Zero the first > * sector to remove any potentially pre-existing image header. > */ > -static int create_file_fallback_zero_first_sector(BlockBackend *blk, > - int64_t current_size, > - Error **errp) > +static int coroutine_fn create_file_fallback_zero_first_sector(BlockBackend > *blk, > + int64_t > current_size, > + Error **errp) Why mark this coroutine_fn? Maybe the intention was to also replace the call to blk_pwrite_zeroes() with blk_co_pwrite_zeroes()? Regardless: Reviewed-by: Alberto Faria
