[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations |
Date: |
Fri, 4 Nov 2022 09:44:05 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 |
On 11/4/22 08:35, Emanuele Giuseppe Esposito wrote:
But isn't it a bug also not to mark a function _only_ called by
coroutine_fn? My point is that if this function is an implementation of
a BlockDriver callback marked as coroutine_fn (like in patch 6 with
vmdk), then it would make sense.
If a function implements a coroutine_fn callback but does not suspend,
then it makes sense to mark it coroutine_fn.
In general it's not a bug. In most cases it would only be a coincidence
that the function is called from a coroutine_fn. For example consider
bdrv_round_to_clusters(). Marking it coroutine_fn signals that it may
suspend now (it doesn't) or in the future. However it's only doing some
math based on the result of bdrv_get_info(), so it is extremely unlikely
that this will happen.
In this case... oh wait. block_copy_is_cluster_allocated is calling
bdrv_is_allocated, and block_copy_reset_unallocated calls
block_copy_is_cluster_allocated. bdrv_is_allocated is a mixed
coroutine/non-coroutine function, and in this case it is useful to
document that bdrv_is_allocated will suspend. The patch is correct,
only the commit message is wrong.
Likewise for blockstatus_to_extents in patch 3, where the commit message
does mention bdrv_* functions. As I mentioned in my quick review of
patch 3, this can also snowball into a series of its own to clean up all
callees of bdrv_co_common_block_status_above, similar to what Alberto
did for read/write functions back in June, so that they are properly
marked as coroutine_fn. If you want to do it, don't do it by hand
though, you can use his static analyzer. It's slow but it's faster than
doing it by hand.
This is actually the point of this serie (which I might not have
explained well in the cover letter), every function marked here is
eventually called by/calling a BlockDriver callback marked as coroutine_fn.
Again I don't think this is useful in general, but your three patches
(2/3/6) did catch cases that wants to be coroutine_fn. So my objection
is dropped with just a better commit message.
Currently we have something like this:
BlockDriver {
void coroutine_fn (*bdrv_A)(void) = implA;
}
void coroutine_fn implA() {
funcB();
funcC();
}
void funcB() {}; <--- missing coroutine_fn?
void funcC() {}; <--- missing coroutine_fn?
In addition, as I understand draining is not allowed in coroutines.
... except we have bdrv_co_yield_to_drain() to allow that, sort of. :/
If a function/callback only running in coroutines is not marked as
coroutine_fn, then it will be less obvious to notice that draining is
not allowed there.
I think it has to be judged case by base. Your patches prove that, in
most cases, you have coroutine_fn for things that ultimately do some
kind of I/O or query. In general the interesting path to explore is
"coroutine_fn calls (indirectly) non-coroutine_fn calls (indirectly)
generated_co_wrapper". The vrc tool could be extended to help finding
them, with commands like
label coroutine_fn bdrv_co_read
label coroutine_fn bdrv_co_write
...
label generated_co_wrapper bdrv_read
label generated_co_wrapper bdrv_write
paths coroutine_fn !coroutine_fn generated_co_wrapper
Paolo
- Re: [PATCH 7/9] block: bdrv_create_file is a coroutine_fn, (continued)
- [PATCH 3/9] nbd/server.c: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
- [PATCH 8/9] block: bdrv_create is never called in non-coroutine context, Emanuele Giuseppe Esposito, 2022/11/03
- [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Kevin Wolf, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/03
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations,
Paolo Bonzini <=
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Paolo Bonzini, 2022/11/04
- Re: [PATCH 2/9] block-copy: add missing coroutine_fn annotations, Kevin Wolf, 2022/11/04
[PATCH 6/9] block/vmdk: add missing coroutine_fn annotations, Emanuele Giuseppe Esposito, 2022/11/03
[PATCH 9/9] block/dirty-bitmap: remove unnecessary qemu_in_coroutine() case, Emanuele Giuseppe Esposito, 2022/11/03