[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 25/33] block_int-common.h: split function pointers in Bdrv
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v6 25/33] block_int-common.h: split function pointers in BdrvChildClass |
Date: |
Fri, 28 Jan 2022 16:08:16 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
On 26/01/2022 13:42, Hanna Reitz wrote:
> On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote:
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>> include/block/block_int-common.h | 67 +++++++++++++++++++-------------
>> 1 file changed, 40 insertions(+), 27 deletions(-)
>>
>> diff --git a/include/block/block_int-common.h
>> b/include/block/block_int-common.h
>> index e007dbf768..cc8c8835ba 100644
>> --- a/include/block/block_int-common.h
>> +++ b/include/block/block_int-common.h
>> @@ -815,12 +815,16 @@ struct BdrvChildClass {
>> */
>> bool parent_is_bds;
>> + /*
>> + * Global state (GS) API. These functions run under the BQL lock.
>> + *
>> + * See include/block/block-global-state.h for more information about
>> + * the GS API.
>> + */
>> void (*inherit_options)(BdrvChildRole role, bool parent_is_format,
>> int *child_flags, QDict *child_options,
>> int parent_flags, QDict *parent_options);
>> -
>> void (*change_media)(BdrvChild *child, bool load);
>> - void (*resize)(BdrvChild *child);
>> /*
>> * Returns a name that is supposedly more useful for human users
>> than the
>
> The method this comment belongs to is `.get_name()`. It’s exposed
> through `bdrv_get_parent_name()`, which is called by
> `bdrv_get_device_name()` and `bdrv_get_device_or_node_name()` – so I
> think it should be classified as I/O.
Agree on this, and also on comment on patch 23 about bdrv_probe.
>
>> @@ -837,6 +841,40 @@ struct BdrvChildClass {
>> */
>> char *(*get_parent_desc)(BdrvChild *child);
>
> This function is very similar, so we might also want to reconsider
> classifying it as I/O. There’s no need, because all of its callers do
> run in the main thread, but at the same time I don’t believe there’s
> anything stopping us (and it starts to sound to me like all functions of
> the “get name” kind perhaps should ideally be I/O, in that they
> shouldn’t require the GS context).
>
> Up to you. O:)
I understand what you mean, but I would like to leave it in global
state. Seeing the assertion/definition as GS tells me that it is already
under BQL, and I should not worry about I/O in that function. If no such
assert was present, it would lead me to think that there might be I/O
concurrently using it, and complicate the logic of whatever I want to do
there.
Thank you,
Emanuele
>
> (Rest of this patch looks good!)
>
> Hanna
>
- [PATCH v6 21/33] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate, (continued)
- [PATCH v6 21/33] block: move BQL logic of bdrv_co_invalidate_cache in bdrv_activate, Emanuele Giuseppe Esposito, 2022/01/21
- [PATCH v6 31/33] include/qemu/job.h: introduce job->pre_run() and use it in amend, Emanuele Giuseppe Esposito, 2022/01/21
- [PATCH v6 26/33] block_int-common.h: assertions in the callers of BdrvChildClass function pointers, Emanuele Giuseppe Esposito, 2022/01/21
- [PATCH v6 28/33] job.h: split function pointers in JobDriver, Emanuele Giuseppe Esposito, 2022/01/21
- [PATCH v6 25/33] block_int-common.h: split function pointers in BdrvChildClass, Emanuele Giuseppe Esposito, 2022/01/21
- [PATCH v6 30/33] include/block/block_int-common.h: introduce bdrv_amend_pre_run and bdrv_amend_clean, Emanuele Giuseppe Esposito, 2022/01/21
- [PATCH v6 27/33] block-backend-common.h: split function pointers in BlockDevOps, Emanuele Giuseppe Esposito, 2022/01/21
- [PATCH v6 32/33] crypto: delegate permission functions to JobDriver .pre_run, Emanuele Giuseppe Esposito, 2022/01/21
- [PATCH v6 33/33] block.c: assertions to the block layer permissions API, Emanuele Giuseppe Esposito, 2022/01/21