On 07.05.20 13:19, Kevin Wolf wrote:
> 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.

Alles kaputt :)

> 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.)

OK.  So I’ll just make unmap=on the default always.

Max