Re: [Qemu-devel] [PATCH v12 02/10] block: Update comments on BDRV_BLOCK_* meanings

[Max Reitz <mreitz@redhat.com>]

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

On 04.05.2017 05:07, Eric Blake wrote:
> We had some conflicting documentation: a nice 8-way table that
> described all possible combinations of DATA, ZERO, and
> OFFSET_VALID, contrasted with text that implied that OFFSET_VALID
> always meant raw data could be read directly.  Furthermore, the
> text refers a lot to bs->file, even though the interface was
> updated back in 67a0fd2a to let the driver pass back which BDS (not
> necessarily bs->file).

Not sure about my English skills here, but is this missing a verb? ("to
pass back which BDS...")

>                         As the 8-way table is the intended
> semantics, simplify the rest of the text to get rid of the
> confusion.
> 
> ALLOCATED is always set by the block layer for convenience (drivers
> do not have to worry about it). RAW is used only internally, but

Just one space after the period? How inconsistent! :-)

> by more than the raw driver.  Document these additional items on
> the driver callback.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> ---
> v12: even more wording tweaks
> v11: reserved for blkdebug half of v10
> v10: new patch
> ---
>  include/block/block.h     | 35 +++++++++++++++++++----------------
>  include/block/block_int.h |  7 +++++++
>  2 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index 862eb56..c8bec7d 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -120,29 +120,32 @@ typedef struct HDGeometry {
>  #define BDRV_REQUEST_MAX_BYTES (BDRV_REQUEST_MAX_SECTORS << BDRV_SECTOR_BITS)
> 
>  /*
> - * Allocation status flags
> - * BDRV_BLOCK_DATA: data is read from a file returned by bdrv_get_block_status.
> - * BDRV_BLOCK_ZERO: sectors read as zero
> - * BDRV_BLOCK_OFFSET_VALID: sector stored as raw data in a file returned by
> - *                          bdrv_get_block_status.
> + * Allocation status flags for bdrv_get_block_status() and friends.
> + *
> + * Public flags:
> + * BDRV_BLOCK_DATA: allocation for data at offset is tied to this layer
> + * BDRV_BLOCK_ZERO: offset reads as zero
> + * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data
>   * BDRV_BLOCK_ALLOCATED: the content of the block is determined by this
> - *                       layer (as opposed to the backing file)
> - * BDRV_BLOCK_RAW: used internally to indicate that the request
> - *                 was answered by the raw driver and that one
> - *                 should look in bs->file directly.
> + *                       layer (short for DATA || ZERO), set by block layer
>   *
> - * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 represent the offset in
> - * bs->file where sector data can be read from as raw data.
> + * Internal flag:
> + * BDRV_BLOCK_RAW: used internally to indicate that the request was
> + *                 answered by a passthrough driver such as raw and that the

s/passthrough/filter/? But I'm not even sure myself. Well, "passthrough"
is a safe bet, so let's just go with it.

With the commit message fixed or a “no it's fine”:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> + *                 block layer should recompute the answer from bs->file.
>   *
> - * DATA == 0 && ZERO == 0 means that data is read from backing_hd if present.
> + * If BDRV_BLOCK_OFFSET_VALID is set, bits 9-62 (BDRV_BLOCK_OFFSET_MASK)
> + * represent the offset in the returned BDS that is allocated for the
> + * corresponding raw data; however, whether that offset actually contains
> + * data also depends on BDRV_BLOCK_DATA and BDRV_BLOCK_ZERO, as follows:
>   *
>   * DATA ZERO OFFSET_VALID
> - *  t    t        t       sectors read as zero, bs->file is zero at offset
> - *  t    f        t       sectors read as valid from bs->file at offset
> - *  f    t        t       sectors preallocated, read as zero, bs->file not
> + *  t    t        t       sectors read as zero, returned file is zero at offset
> + *  t    f        t       sectors read as valid from file at offset
> + *  f    t        t       sectors preallocated, read as zero, returned file not
>   *                        necessarily zero at offset
>   *  f    f        t       sectors preallocated but read from backing_hd,
> - *                        bs->file contains garbage at offset
> + *                        returned file contains garbage at offset
>   *  t    t        f       sectors preallocated, read as zero, unknown offset
>   *  t    f        f       sectors read from unknown file or offset
>   *  f    t        f       not allocated or unknown offset, read as zero
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8773940..1fdfff7 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -165,6 +165,13 @@ struct BlockDriver {
>          int64_t offset, int count, BdrvRequestFlags flags);
>      int coroutine_fn (*bdrv_co_pdiscard)(BlockDriverState *bs,
>          int64_t offset, int count);
> +
> +    /*
> +     * Building block for bdrv_block_status[_above]. The driver should
> +     * answer only according to the current layer, and should not
> +     * set BDRV_BLOCK_ALLOCATED, but may set BDRV_BLOCK_RAW.  See block.h
> +     * for the meaning of _DATA, _ZERO, and _OFFSET_VALID.
> +     */
>      int64_t coroutine_fn (*bdrv_co_get_block_status)(BlockDriverState *bs,
>          int64_t sector_num, int nb_sectors, int *pnum,
>          BlockDriverState **file);
> 



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