Re: [PATCH v2 01/11] block: Expand block status mode from bool to enum
On Fri, Apr 18, 2025 at 02:02:20PM -0500, Eric Blake wrote: > > 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. > > I can give that a try. If I'm understanding the request correctly, I > would map it as follows: > > BDRV_BSTAT_PRECISE => WANT_ZERO | WANT_OFFSET_VALID | WANT_ALLOCATED > BDRV_BSTAT_ALLOCATED => WANT_ALLOCATED > BDRV_BSTAT_ZERO => WANT_ZERO > > while still trying to keep it a mechanical conversion in this patch. I've done that as a v2.5 reply to patch 1 and 2 of this series (the rest of the series is unchanged except for one obvious word change in the addition of bdrv_co_is_all_zeroes). If we like it better, I can resend the full series as v3. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
Re: [PATCH v2 01/11] block: Expand block status mode from bool to enum
On Thu, Apr 17, 2025 at 04:17:55PM -0400, Stefan Hajnoczi wrote:
> 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.
> >
> > +/* 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.
I can give that a try. If I'm understanding the request correctly, I
would map it as follows:
BDRV_BSTAT_PRECISE => WANT_ZERO | WANT_OFFSET_VALID | WANT_ALLOCATED
BDRV_BSTAT_ALLOCATED => WANT_ALLOCATED
BDRV_BSTAT_ZERO => WANT_ZERO
while still trying to keep it a mechanical conversion in this patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
Re: [PATCH v2 01/11] block: Expand block status mode from bool to enum
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
> ---
> 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 semanti
