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 <[email protected]> > Reviewed-by: Eric Blake <[email protected]> > Reviewed-by: Alberto Garcia <[email protected]> > --- > 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.)
> + */
> + 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).
> flags |= BDRV_REQ_ZERO_WRITE;
> if (bs->detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP) {
> flags |= BDRV_REQ_MAY_UNMAP;
> @@ -1593,6 +1593,9 @@ static int coroutine_fn
> bdrv_co_do_zero_pwritev(BdrvChild *child,
>
> assert(flags & BDRV_REQ_ZERO_WRITE);
> if (head_padding_bytes || tail_padding_bytes) {
> + if (flags & BDRV_REQ_ALLOCATE) {
> + return -ENOTSUP;
> + }
> buf = qemu_blockalign(bs, align);
> iov = (struct iovec) {
> .iov_base = buf,
> @@ -1693,6 +1696,9 @@ int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
> return ret;
> }
>
> + /* allocation request with qiov provided doesn't make much sense */
> + assert(!(qiov && (flags & BDRV_REQ_ALLOCATE)));
> +
So I suppose the use of BDRV_REQ_ALLOCATE necessitates the use of
BDRV_REQ_ZERO_WRITE? That should be documented, then.
Max
> bdrv_inc_in_flight(bs);
> /*
> * Align write if necessary by performing a read-modify-write cycle.
> @@ -1822,6 +1828,14 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild
> *child, int64_t offset,
> {
> trace_bdrv_co_pwrite_zeroes(child->bs, offset, bytes, flags);
>
> + assert(!((flags & BDRV_REQ_MAY_UNMAP) && (flags & BDRV_REQ_ALLOCATE)));
> +
> + if ((flags & BDRV_REQ_ALLOCATE) &&
> + !(child->bs->supported_zero_flags & BDRV_REQ_ALLOCATE))
> + {
> + return -ENOTSUP;
> + }
> +
> if (!(child->bs->open_flags & BDRV_O_UNMAP)) {
> flags &= ~BDRV_REQ_MAY_UNMAP;
> }
>
signature.asc
Description: OpenPGP digital signature
