Re: [PATCH v2 01/11] block: Expand block status mode from bool to enum

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Thu, Apr 17, 2025 at 01:39:06PM -0500, Eric Blake wrote:
> This patch is purely mechanical, changing bool want_zero into a new
> enum BlockStatusMode.  As of this patch, all implementations are
> unchanged (the old want_zero==true is now mode==BDRV_BSTAT_PRECISE),
> but the callers in io.c are set up so that future patches will be able
> to differente between whether the caller cares more about allocation

differentiate

> or about reads-as-zero, for driver implementations that will actually
> want to behave differently for those more-specific hints.
> 
> As for the background why this patch is useful: right now, the
> file-posix driver recognizes that if allocation is being queried, the
> entire image can be reported as allocated (there is no backing file to
> refer to) - but this throws away information on whether the entire
> image reads as zero (trivially true if lseek(SEEK_HOLE) at offset 0
> returns -ENXIO, a bit more complicated to prove if the raw file was
> created with 'qemu-img create' since we intentionally allocate a small
> chunk of all-zero data to help with alignment probing).  The next
> patches will add a generic algorithm for seeing if an entire file
> reads as zeroes, as well as tweak the file-posix driver to react to
> the new hints.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/coroutines.h               |  4 +--
>  include/block/block-common.h     | 26 ++++++++++++++++
>  include/block/block_int-common.h | 25 +++++++++-------
>  include/block/block_int-io.h     |  4 +--
>  block/io.c                       | 51 ++++++++++++++++----------------
>  block/blkdebug.c                 |  6 ++--
>  block/copy-before-write.c        |  4 +--
>  block/file-posix.c               |  4 +--
>  block/gluster.c                  |  4 +--
>  block/iscsi.c                    |  6 ++--
>  block/nbd.c                      |  4 +--
>  block/null.c                     |  6 ++--
>  block/parallels.c                |  6 ++--
>  block/qcow.c                     |  2 +-
>  block/qcow2.c                    |  6 ++--
>  block/qed.c                      |  6 ++--
>  block/quorum.c                   |  4 +--
>  block/raw-format.c               |  4 +--
>  block/rbd.c                      |  6 ++--
>  block/snapshot-access.c          |  4 +--
>  block/vdi.c                      |  4 +--
>  block/vmdk.c                     |  2 +-
>  block/vpc.c                      |  2 +-
>  block/vvfat.c                    |  6 ++--
>  tests/unit/test-block-iothread.c |  2 +-
>  25 files changed, 114 insertions(+), 84 deletions(-)
> 
> diff --git a/block/coroutines.h b/block/coroutines.h
> index 79e5efbf752..c8323aa67e6 100644
> --- a/block/coroutines.h
> +++ b/block/coroutines.h
> @@ -47,7 +47,7 @@ int coroutine_fn GRAPH_RDLOCK
>  bdrv_co_common_block_status_above(BlockDriverState *bs,
>                                    BlockDriverState *base,
>                                    bool include_base,
> -                                  bool want_zero,
> +                                  enum BlockStatusMode mode,
>                                    int64_t offset,
>                                    int64_t bytes,
>                                    int64_t *pnum,
> @@ -78,7 +78,7 @@ int co_wrapper_mixed_bdrv_rdlock
>  bdrv_common_block_status_above(BlockDriverState *bs,
>                                 BlockDriverState *base,
>                                 bool include_base,
> -                               bool want_zero,
> +                               enum BlockStatusMode mode,
>                                 int64_t offset,
>                                 int64_t bytes,
>                                 int64_t *pnum,
> diff --git a/include/block/block-common.h b/include/block/block-common.h
> index 0b831ef87b1..619e75b9c8d 100644
> --- a/include/block/block-common.h
> +++ b/include/block/block-common.h
> @@ -508,6 +508,32 @@ enum BdrvChildRoleBits {
>                                | BDRV_CHILD_PRIMARY,
>  };
> 
> +/* Modes for block status calls */
> +enum BlockStatusMode {
> +    /*
> +     * Status should be as accurate as possible: _OFFSET_VALID
> +     * and_OFFSET_ZERO should each be set where efficiently possible,

"and _OFFSET_ZERO"

> +     * extents may be smaller, and iteration through the entire block
> +     * device may take more calls.
> +     */
> +    BDRV_BSTAT_PRECISE,
> +
> +    /*
> +     * The caller is primarily concerned about overall allocation:
> +     * favor larger *pnum, perhaps by coalescing extents and reporting
> +     * _DATA instead of _ZERO, and without needing to read data or
> +     * bothering with _OFFSET_VALID.
> +     */
> +    BDRV_BSTAT_ALLOCATED,
> +
> +    /*
> +     * The caller is primarily concerned about whether the device
> +     * reads as zero: favor a result of _ZERO, even if it requires
> +     * reading a few sectors to verify, without needing _OFFSET_VALID.
> +     */
> +    BDRV_BSTAT_ZERO,
> +};

I have trouble understanding what the exact semantics are of these modes
are. Would it be possible to pass flags to block status calls that can
be ORed together instead: WANT_OFFSET_VALID, WANT_ZERO, etc? The flags
would be orthogonal and easier to understand than modes that seem to
combine multiple flag behaviors.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]