Re: [PATCH v3 09/33] block: Add generic bdrv_inherited_options()

[Max Reitz]

On 06.05.20 15:11, Kevin Wolf wrote:
> Am 06.05.2020 um 12:37 hat Kevin Wolf geschrieben:
>> Am 18.02.2020 um 13:42 hat Max Reitz geschrieben:
>>> After the series this patch belongs to, we want to have a common
>>> BdrvChildClass that encompasses all of child_file, child_format, and
>>> child_backing.  Such a single class needs a single .inherit_options()
>>> implementation, and this patch introduces it.
>>>
>>> The next patch will show how the existing implementations can fall back
>>> to it just by passing appropriate BdrvChildRole and parent_is_format
>>> values.
>>>
>>> Signed-off-by: Max Reitz <address@hidden>
>>> Reviewed-by: Eric Blake <address@hidden>
>>> ---
>>>  block.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 84 insertions(+)
>>>
>>> diff --git a/block.c b/block.c
>>> index c33f0e9b42..9179b9b604 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -998,6 +998,90 @@ static void bdrv_temp_snapshot_options(int 
>>> *child_flags, QDict *child_options,
>>>      *child_flags &= ~BDRV_O_NATIVE_AIO;
>>>  }
>>>  
>>> +/*
>>> + * Returns the options and flags that a generic child of a BDS should
>>> + * get, based on the given options and flags for the parent BDS.
>>> + */
>>> +static void __attribute__((unused))
>>> +    bdrv_inherited_options(BdrvChildRole role, bool parent_is_format,
>>> +                           int *child_flags, QDict *child_options,
>>> +                           int parent_flags, QDict *parent_options)
>>> +{
>>> +    int flags = parent_flags;
>>> +
>>> +    /*
>>> +     * First, decide whether to set, clear, or leave BDRV_O_PROTOCOL.
>>> +     * Generally, the question to answer is: Should this child be
>>> +     * format-probed by default?
>>> +     */
> 
> Just for clarity: Do you know a good reason to ever leave it (i.e.
> inherit it from the parent), except that that's what we have always been
> doing for backing files? Though of course, only formats have backing
> files, so the flag would never be set in practice in this case.

It seems correct for filters.

[...]

>>> +    if (parent_is_format && !(role & BDRV_CHILD_COW)) {
>>> +        /*
>>> +         * Our format drivers take care to send flushes and respect
>>> +         * unmap policy, so we can default to enable both on lower
>>> +         * layers regardless of the corresponding parent options.
>>> +         */
>>> +        qdict_set_default_str(child_options, BDRV_OPT_DISCARD, "unmap");
>>> +    }
>>
>> Why the restriction to format here? Don't we break "unmap" propagation
>> through filters with this?
>>
>> It would probably also be a good question why we don't propagate it to
>> the backing file, but this is preexisting.
> 
> Some patches later, I think the fix is an else branch that copies the
> flag from parent_options.

I thought about the same thing, but is that really necessary if
bdrv_co_pdiscard() will already suppress discards on the parent if unmap
is false?

Max

signature.asc
Description: OpenPGP digital signature