[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v12 02/10] block: Update comments on BDRV_BLOCK_
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v12 02/10] block: Update comments on BDRV_BLOCK_* meanings |
Date: |
Fri, 5 May 2017 22:06:39 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0.1 |
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 <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> 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 <address@hidden>
> + * 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);
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH v12 00/10] qcow2 zero-cluster tweaks [was add blkdebug tests], Eric Blake, 2017/05/03
- [Qemu-devel] [PATCH v12 01/10] qcow2: Use consistent switch indentation, Eric Blake, 2017/05/03
- [Qemu-devel] [PATCH v12 05/10] qcow2: Optimize zero_single_l2() to minimize L2 churn, Eric Blake, 2017/05/03
- [Qemu-devel] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious, Eric Blake, 2017/05/03
- [Qemu-devel] [PATCH v12 09/10] qcow2: Assert that cluster operations are aligned, Eric Blake, 2017/05/03