Re: [Qemu-devel] [PATCH v7 3/9] block: introduce BDRV_REQ_ALLOCATE flag

[Max Reitz <mreitz@redhat.com>]

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

On 2018-01-30 13:34, Anton Nefedov wrote:
> 
> 
> On 29/1/2018 10:37 PM, Max Reitz wrote:
>> On 2018-01-18 18:49, Anton Nefedov wrote:
>>> The flag is supposed to indicate that the region of the disk image has
>>> to be sufficiently allocated so it reads as zeroes.
>>>
>>> The call with the flag set must return -ENOTSUP if allocation cannot
>>> be done efficiently.
>>> This has to be made sure of by both
>>>    - the drivers that support the flag
>>>    - and the common block layer (so it will not fall back to any
>>> slowpath
>>>      (like writing zero buffers) in case the driver does not support
>>>      the flag).
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>>   include/block/block.h     |  6 +++++-
>>>   include/block/block_int.h |  2 +-
>>>   block/io.c                | 20 +++++++++++++++++---
>>>   3 files changed, 23 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index 9b12774..3e31b89 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -65,9 +65,13 @@ typedef enum {
>>>       BDRV_REQ_NO_SERIALISING     = 0x8,
>>>       BDRV_REQ_FUA                = 0x10,
>>>       BDRV_REQ_WRITE_COMPRESSED   = 0x20,
>>> +    /* The BDRV_REQ_ALLOCATE flag is used to indicate that the
>>> driver has to
>>> +     * efficiently allocate the space so it reads as zeroes, or
>>> return an error.
>>
>> What happens if you specify this for a normal write operation that does
>> not write zeroes?
>>
>> (I suppose the answer is "don't do that", but that would need to be
>> documented more clearly here.)
>>
> 
> I can't quite come up with what a regular write with ALLOCATE flag can
> suppose to mean.

It could mean that when zero detection is active, that zero range will
be allocated.  But considering ALLOCATE explicitly means "do not write
data", it probably doesn't make sense for data writes in general.

> Will document that.

Thanks!

>>> +     */
>>> +    BDRV_REQ_ALLOCATE           = 0x40,
>>>         /* Mask of valid flags */
>>> -    BDRV_REQ_MASK               = 0x3f,
>>> +    BDRV_REQ_MASK               = 0x7f,
>>>   } BdrvRequestFlags;
>>>     typedef struct BlockSizes {
>>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>>> index 29cafa4..b141710 100644
>>> --- a/include/block/block_int.h
>>> +++ b/include/block/block_int.h
>>> @@ -632,7 +632,7 @@ struct BlockDriverState {
>>>       /* Flags honored during pwrite (so far: BDRV_REQ_FUA) */
>>>       unsigned int supported_write_flags;
>>>       /* Flags honored during pwrite_zeroes (so far: BDRV_REQ_FUA,
>>> -     * BDRV_REQ_MAY_UNMAP) */
>>> +     * BDRV_REQ_MAY_UNMAP, BDRV_REQ_ALLOCATE) */
>>>       unsigned int supported_zero_flags;
>>>         /* the following member gives a name to every node on the bs
>>> graph. */
>>> diff --git a/block/io.c b/block/io.c
>>> index 7ea4023..cf2f84c 100644
>>> --- a/block/io.c
>>> +++ b/block/io.c
>>> @@ -1424,7 +1424,7 @@ static int coroutine_fn
>>> bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
>>>               assert(!bs->supported_zero_flags);
>>>           }
>>>   -        if (ret == -ENOTSUP) {
>>> +        if (ret == -ENOTSUP && !(flags & BDRV_REQ_ALLOCATE)) {
>>>               /* Fall back to bounce buffer if write zeroes is
>>> unsupported */
>>>               BdrvRequestFlags write_flags = flags &
>>> ~BDRV_REQ_ZERO_WRITE;
>>>   @@ -1514,8 +1514,8 @@ static int coroutine_fn
>>> bdrv_aligned_pwritev(BdrvChild *child,
>>>       ret =
>>> notifier_with_return_list_notify(&bs->before_write_notifiers, req);
>>>         if (!ret && bs->detect_zeroes !=
>>> BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF &&
>>> -        !(flags & BDRV_REQ_ZERO_WRITE) && drv->bdrv_co_pwrite_zeroes &&
>>> -        qemu_iovec_is_zero(qiov)) {
>>> +        !(flags & BDRV_REQ_ZERO_WRITE) && !(flags &
>>> BDRV_REQ_ALLOCATE) &&
>>> +        drv->bdrv_co_pwrite_zeroes && qemu_iovec_is_zero(qiov)) {
>>
>> Do we really need to add the BDRV_REQ_ALLOCATE check here?  If the
>> caller specifies that flag, then we won't invalidate it by adding the
>> BDRV_REQ_ZERO_WRITE flag (as long as we don't add BDRV_REQ_MAY_UNMAP).
>>
> 
> Now !(flags & BDRV_REQ_ALLOCATE) is always true here, as REQ_ALLOCATE
> implies REQ_ZERO_WRITE.
> But conceptually yes I think the check should only forbid setting
> MAY_UNMAP.
> 
> Offtop: does REQ_ZERO_WRITE override REQ_WRITE_COMPRESSED in this
> function? at least with !REQ_MAY_UNMAP it looks wrong

Looks like zero detection will indeed override compression.  I think
that was intended, but I don't even have an opinion either way.

Of course, it wouldn't be so nice if you tried to compress something and
then, because the zero write failed, you actually write uncompressed
zeroes...  But since zero detection is an optional feature, it might be
your own fault if you enable it when you want compression anyway, and if
you write to some format/protocol combination that doesn't allow zero
writes.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]