Re: [PATCH v3 2/2] quorum: Implement bdrv_co_pwrite_zeroes()

[Max Reitz <mreitz@redhat.com>]

On 13.11.20 17:26, Alberto Garcia wrote:
> On Fri 13 Nov 2020 05:11:20 PM CET, Max Reitz wrote:
> 
>>> We could set all supported_zero_flags as long as all children support
>>> them, right?
>>
>> Sure, I was just thinking that we could set these regardless of
>> whether the children support them, because (on zero-writes) the block
>> layer will figure out for us whether the child nodes support them. O:)
> 
> But it can happen that one child supports e.g. BDRV_REQ_NO_FALLBACK but
> the rest don't. In this case I think the block layer should return
> -ENOTSUP earlier without writing to the child(ren) that do support that
> flag.

That’s true.

> So Quorum's supported_zero_flags would be the logical and of all of its
> children's flags, right?

Yes.  (And so it would need to be recalculated whenever a child is added 
or removed.)

> I'm unsure about BDRV_REQ_WRITE_UNCHANGED, many filters set that on top
> of the other flags, but when would a BDS not support this flag?

The WRITE_UNCHANGED flag is basically just a hint for the permission 
system that for such a write, the WRITE_UNCHANGED permission is 
sufficient.  So drivers that can pass through *all* WRITE_UNCHANGED 
requests as they are (i.e., filter drivers) can support this write/zero 
flag and then pass that flag on.  This way, they are safe not to take 
the WRITE permission on their child unless their parent has taken the 
WRITE permission on them.

(Otherwise, they’d also have to take the WRITE permission if the parent 
only holds the WRITE_UNCHANGED permission.)

Supporting this flag doesn’t make sense for drivers that won’t be able 
to pass it on all the time.  For example, qcow2 generally cannot pass it 
on, because it still needs to perform WRITE_UNCHANGED requests as normal 
writes.  (WRITE_UNCHANGED comes from copy-on-read; the data will read 
the same, hence the _UNCHANGED, but it still needs to be allocated on 
the receiving format layer.)

(Technically, qcow2 could figure out whether the requests it generates 
from a WRITE_UNCHANGED request still result in unchanging writes in the 
protocol layer, but there is no reason to.  It needs the WRITE 
permission anyway, because there are going to be some non-unchanging 
writes it has to perform.  And if it holds the WRITE permission, there 
is no need to bother with the WRITE_UNCHANGED flag.)

>>> pwrite_zeroes() does this additionaly:
>>>
>>>       if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
>>>           flags &= ~BDRV_REQ_MAY_UNMAP;
>>>       }
>>
>> Interesting.  Technically, Quorum doesn’t support that flag (in
>> supported_zero_flags O:))), so it shouldn’t appear, but, er, well
>> then.
> 
> It would with the change that I'm proposing above.

Yes, I know.  Hence the “O:)”. O:)

Max