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

[Kevin Wolf <kwolf@redhat.com>]

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 <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  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?
> +     */
> +
> +    /*
> +     * Pure and non-filtered data children of non-format nodes should
> +     * be probed by default (even when the node itself has BDRV_O_PROTOCOL
> +     * set).  This only affects a very limited set of drivers (namely
> +     * quorum and blkverify when this comment was written).
> +     * Force-clear BDRV_O_PROTOCOL then.
> +     */
> +    if (!parent_is_format &&
> +        (role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
> +                 BDRV_CHILD_FILTERED)) ==
> +            BDRV_CHILD_DATA)

You could avoid the odd indentation (I can't decide whether or not it
should be one space more to align correctly) and probably also make the
expression more readable if you split it into:

    (role & BDRV_CHILD_DATA) &&
    !(role & (BDRV_CHILD_METADATA | BDRV_CHILD_FILTERED))

> +    {
> +        flags &= ~BDRV_O_PROTOCOL;
> +    }
> +
> +    /*
> +     * All children of format nodes (except for COW children) and all
> +     * metadata children in general should never be format-probed.
> +     * Force-set BDRV_O_PROTOCOL then.
> +     */
> +    if ((parent_is_format && !(role & BDRV_CHILD_COW)) ||
> +        (role & BDRV_CHILD_METADATA))
> +    {
> +        flags |= BDRV_O_PROTOCOL;
> +    }
> +
> +    /*
> +     * If the cache mode isn't explicitly set, inherit direct and no-flush from
> +     * the parent.
> +     */
> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
> +    qdict_copy_default(child_options, parent_options, BDRV_OPT_FORCE_SHARE);
> +
> +    if (role & BDRV_CHILD_COW) {
> +        /* backing files are always opened read-only */

Not "always", just by default.

> +        qdict_set_default_str(child_options, BDRV_OPT_READ_ONLY, "on");
> +        qdict_set_default_str(child_options, BDRV_OPT_AUTO_READ_ONLY, "off");
> +    } else {
> +        /* Inherit the read-only option from the parent if it's not set */
> +        qdict_copy_default(child_options, parent_options, BDRV_OPT_READ_ONLY);
> +        qdict_copy_default(child_options, parent_options,
> +                           BDRV_OPT_AUTO_READ_ONLY);
> +    }
> +
> +    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.

> +    /* Clear flags that only apply to the top layer */
> +    flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
> +
> +    if (role & BDRV_CHILD_METADATA) {
> +        flags &= ~BDRV_O_NO_IO;
> +    }

Hm... This is interesting, but I guess it makes sense. It just never was
a hard rule that a format driver must not access data even internally
with NO_IO, but I think it holds true.

> +    if (role & BDRV_CHILD_COW) {
> +        flags &= ~BDRV_O_TEMPORARY;
> +    }

We could probably have a long discussion about whether this is right in
theory, but in practice BDRV_O_TEMPORARY only exists for snapshot=on,
for which we know that it's always qcow2 with a file and a backing
child. And there is no doubt that we make the right distinction in this
case.

> +    *child_flags = flags;
> +}
> +
>  /*
>   * Returns the options and flags that bs->file should get if a protocol driver
>   * is expected, based on the given options and flags for the parent BDS
> -- 
> 2.24.1

Kevin