| Message ID | 1516297747-107232-4-git-send-email-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <anton.nefedov@virtuozzo.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > 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; > } >
On 29/1/2018 10:37 PM, Max Reitz wrote: > 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 <anton.nefedov@virtuozzo.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> --- >> 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.) > I can't quite come up with what a regular write with ALLOCATE flag can suppose to mean. Will document that. >> + */ >> + 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). > Now !(flags & BDRV_REQ_ALLOCATE) is always true here, as REQ_ALLOCATE implies REQ_ZERO_WRITE. But conceptually yes I think the check should only forbid setting MAY_UNMAP. Offtop: does REQ_ZERO_WRITE override REQ_WRITE_COMPRESSED in this function? at least with !REQ_MAY_UNMAP it looks wrong >> 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 > Yes, will document. >> 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; >> } >> > >
On 2018-01-30 13:34, Anton Nefedov wrote: > > > On 29/1/2018 10:37 PM, Max Reitz wrote: >> 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 <anton.nefedov@virtuozzo.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> Reviewed-by: Alberto Garcia <berto@igalia.com> >>> --- >>> 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.) >> > > I can't quite come up with what a regular write with ALLOCATE flag can > suppose to mean. It could mean that when zero detection is active, that zero range will be allocated. But considering ALLOCATE explicitly means "do not write data", it probably doesn't make sense for data writes in general. > Will document that. Thanks! >>> + */ >>> + 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). >> > > Now !(flags & BDRV_REQ_ALLOCATE) is always true here, as REQ_ALLOCATE > implies REQ_ZERO_WRITE. > But conceptually yes I think the check should only forbid setting > MAY_UNMAP. > > Offtop: does REQ_ZERO_WRITE override REQ_WRITE_COMPRESSED in this > function? at least with !REQ_MAY_UNMAP it looks wrong Looks like zero detection will indeed override compression. I think that was intended, but I don't even have an opinion either way. Of course, it wouldn't be so nice if you tried to compress something and then, because the zero write failed, you actually write uncompressed zeroes... But since zero detection is an optional feature, it might be your own fault if you enable it when you want compression anyway, and if you write to some format/protocol combination that doesn't allow zero writes. Max
On 31/1/2018 8:31 PM, Max Reitz wrote: > On 2018-01-30 13:34, Anton Nefedov wrote: >> Offtop: does REQ_ZERO_WRITE override REQ_WRITE_COMPRESSED in this >> function? at least with !REQ_MAY_UNMAP it looks wrong > > Looks like zero detection will indeed override compression. I think > that was intended, but I don't even have an opinion either way. > > Of course, it wouldn't be so nice if you tried to compress something and > then, because the zero write failed, you actually write uncompressed > zeroes... But since zero detection is an optional feature, it might be > your own fault if you enable it when you want compression anyway, and if > you write to some format/protocol combination that doesn't allow zero > writes. > > Max > Is it detection only? I guess in case the caller sets (REQ_ZERO_WRITE | REQ_COMPRESSED) it also fires here. Looks like noone does that though, but for example backup might; it supports compression, and it also does a zero detection itself and calls write_zeroes() when it can. It sets REQ_MAY_UNMAP which should indeed override a compression, but unmap might be not supported. /Anton
On 2018-02-01 14:34, Anton Nefedov wrote: > On 31/1/2018 8:31 PM, Max Reitz wrote: >> On 2018-01-30 13:34, Anton Nefedov wrote: >>> Offtop: does REQ_ZERO_WRITE override REQ_WRITE_COMPRESSED in this >>> function? at least with !REQ_MAY_UNMAP it looks wrong >> >> Looks like zero detection will indeed override compression. I think >> that was intended, but I don't even have an opinion either way. >> >> Of course, it wouldn't be so nice if you tried to compress something and >> then, because the zero write failed, you actually write uncompressed >> zeroes... But since zero detection is an optional feature, it might be >> your own fault if you enable it when you want compression anyway, and if >> you write to some format/protocol combination that doesn't allow zero >> writes. >> >> Max >> > > Is it detection only? I guess in case the caller sets > (REQ_ZERO_WRITE | REQ_COMPRESSED) it also fires here. True, but that's the caller's "fault". One of those things has to take precedence. And I've just noticed that when bdrv_co_do_pwrite_zeroes() falls back to a bounce buffer, it passes the original flags (without BDRV_REQ_ZERO_WRITE) to bdrv_driver_pwritev(), so compression will take effect then. So the only result is that we first try a zero write, and if that fails, we do a compressed write. That sounds reasonable to me. (I mean, we could do it the other way around, but I firstly I don't think it matters much and secondly it's probably better this way because I'd suspect zero writes to be more efficient than compressing the bounce buffer; at least that's how it is for qcow2.) Max > Looks like noone does that though, but for example backup might; > it supports compression, and it also does a zero detection itself and > calls write_zeroes() when it can. > It sets REQ_MAY_UNMAP which should indeed override a compression, but > unmap might be not supported. > > /Anton
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. + */ + 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)) { 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))); + 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; }
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 <anton.nefedov@virtuozzo.com> Reviewed-by: Eric Blake <eblake@redhat.com> Reviewed-by: Alberto Garcia <berto@igalia.com> --- include/block/block.h | 6 +++++- include/block/block_int.h | 2 +- block/io.c | 20 +++++++++++++++++--- 3 files changed, 23 insertions(+), 5 deletions(-)