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

[Kevin Wolf <kwolf@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4277 bytes --]

Am 07.05.2020 um 10:49 hat Max Reitz geschrieben:
> On 06.05.20 12:37, Kevin Wolf wrote:
> > Am 18.02.2020 um 13:42 hat Max Reitz geschrieben:
> >> +    if (role & BDRV_CHILD_COW) {
> >> +        /* backing files are always opened read-only */
> > 
> > Not "always", just by default.
> 
> OK.  I just copied the comment from bdrv_backing_options().

Yes, sorry, I noticed this only later (it's how review goes when a move
is split into a copy in one patch and a removal later). I don't insist
on a change if you prefer to have a clean copy.

> >> +        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?
> 
> Right now (before this series), the behavior seems ambiguous, in that
> for filters that use bs->file, it is set, but for those that use
> bs->backing, it isn’t.

It's probably easy to agree that this is a bug.

> But I suspect the main reason for what I did is the way I interpreted
> the comment (which before this series only mentions block drivers in
> general, not specifically format drivers): It sounded to me as if the
> block driver needed to respect the unmap policy, and I didn’t think
> filters did that.  So it was my understanding that filter drivers would
> just propagate discards and thus we couldn’t default-enable unmap on
> their children.

This was actually my thought as well. And in order to propagate, we have
to copy the option from parent_options, no? Currently it will always be
disabled (unless specified explicitly for the node) because that's the
default setting for unmap.

> But I was wrong, the block driver doesn’t need to respect anything,
> because bdrv_co_pdiscard() already does.
> 
> So I suppose it should indeed be enabled for all children, with the
> comment changed to express that it isn’t any block driver that respects
> unmap policy, but bdrv_co_pdiscard(), e.g.:
> 
> bdrv_co_pdiscard() respects unmap policy for the parent, so we can
> default to enable it on lower layers regardless of the parent option.

This would restore the behaviour before this series for child_file and
child_format. It would be different in that it also enables "unmap" for
backing files, which should be okay.

> > It would probably also be a good question why we don't propagate it to
> > the backing file, but this is preexisting.
> 
> I suppose we should, although it’s irrelevant, so.  I suppose I’ll just
> drop the parent_is_format, adjust the comment and that should be fine
> for this series.

Isn't it relevant for zero writes during active commit? (The "normal"
intermediate commit job doesn't even try to optimise zero blocks...)

The job will have its own BdrvChild to access the node, but option
inheritance happens only from the parent that "owns" the backing file,
so if a qcow2 image doesn't set "unmap" on its COW child, unmap will be
disabled for the active commit job, too.

(Oops. Turns out that it's not the case because the 'unmap' option for
the job exists only for drive-mirror. blockdev-mirror passes true
unconditionally and block-commit passes false unconditionally. I'm
always amazed how consistently we fail to be consistent.

But I think using zero writes with MAY_UNMAP in a commit job is an
obvious extension, so considering it now can't hurt anyway.)

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]