Re: [PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()
Am 25.11.2019 um 16:06 hat Alberto Garcia geschrieben:
> On Fri 22 Nov 2019 05:05:05 PM CET, Kevin Wolf wrote:
>
> > @@ -3405,6 +3412,7 @@ typedef struct TruncateCo {
> > int64_t offset;
> > bool exact;
> > PreallocMode prealloc;
> > +bool no_fallback;
> > Error **errp;
> > int ret;
> > } TruncateCo;
>
> You add the 'no_fallback' field here...
>
> > int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
> > - PreallocMode prealloc, Error **errp)
> > + PreallocMode prealloc, bool no_fallback, Error **errp)
> > {
> > Coroutine *co;
> > TruncateCo tco = {
>
> ...but then you don't use it when the structure is initialized.
Oops. Another proof that a series like this is too much for -rc3.
Kevin
Re: [PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()
On Fri 22 Nov 2019 05:05:05 PM CET, Kevin Wolf wrote:
> @@ -3405,6 +3412,7 @@ typedef struct TruncateCo {
> int64_t offset;
> bool exact;
> PreallocMode prealloc;
> +bool no_fallback;
> Error **errp;
> int ret;
> } TruncateCo;
You add the 'no_fallback' field here...
> int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
> - PreallocMode prealloc, Error **errp)
> + PreallocMode prealloc, bool no_fallback, Error **errp)
> {
> Coroutine *co;
> TruncateCo tco = {
...but then you don't use it when the structure is initialized.
Berto
Re: [PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()
On 22.11.19 17:05, Kevin Wolf wrote: > This adds a no_fallback parameter to bdrv_co_truncate(), bdrv_truncate() > and blk_truncate() in preparation for a fix that potentially needs to > zero-write the new area. no_fallback will use BDRV_REQ_NO_FALLBACK for > this operation and lets the truncate fail if an efficient zero write > isn't possible. > > Only qmp_block_resize() passes true for this parameter because it is a > blocking monitor command, so we don't want to add more potentially slow > I/O operations to it than we already have. > > All other users will accept even a slow fallback to avoid failure. > > Signed-off-by: Kevin Wolf > --- > include/block/block.h | 5 +++-- > include/sysemu/block-backend.h | 2 +- > block/block-backend.c | 4 ++-- > block/commit.c | 4 ++-- > block/crypto.c | 4 ++-- > block/io.c | 16 > block/mirror.c | 2 +- > block/parallels.c | 6 +++--- > block/qcow.c | 4 ++-- > block/qcow2-refcount.c | 2 +- > block/qcow2.c | 19 +++ > block/qed.c| 2 +- > block/raw-format.c | 2 +- > block/vdi.c| 2 +- > block/vhdx-log.c | 2 +- > block/vhdx.c | 6 +++--- > block/vmdk.c | 10 ++ > block/vpc.c| 2 +- > blockdev.c | 2 +- > qemu-img.c | 2 +- > qemu-io-cmds.c | 2 +- > tests/test-block-iothread.c| 6 +++--- > 22 files changed, 60 insertions(+), 46 deletions(-) With the typo pointed out by Eric fixed: Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()
On 11/22/19 10:05 AM, Kevin Wolf wrote:
This adds a no_fallback parameter to bdrv_co_truncate(), bdrv_truncate()
and blk_truncate() in preparation for a fix that potentially needs to
zero-write the new area. no_fallback will use BDRV_REQ_NO_FALLBACK for
this operation and lets the truncate fail if an efficient zero write
isn't possible.
Only qmp_block_resize() passes true for this parameter because it is a
blocking monitor command, so we don't want to add more potentially slow
I/O operations to it than we already have.
All other users will accept even a slow fallback to avoid failure.
Signed-off-by: Kevin Wolf
---
+++ b/include/block/block.h
@@ -347,9 +347,10 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState
*bs,
void bdrv_refresh_filename(BlockDriverState *bs);
int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp);
+ PreallocMode prealloc, bool no_fallback,
+ Error **errp);
int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp);
+ PreallocMode prealloc, bool no_fallback, Error **errp);
New signature, most of the changes are mechanical to pass the new
parameter...
+++ b/block/io.c
@@ -3313,9 +3313,15 @@ static void bdrv_parent_cb_resize(BlockDriverState *bs)
* If 'exact' is true, the file must be resized to exactly the given
* 'offset'. Otherwise, it is sufficient for the node to be at least
* 'offset' bytes in length.
+ *
+ * If 'no_fallback' is true, a possibly needed writte_zeroes operation to avoid
write
+ * making a longer backing file visible will use BDRV_REQ_NO_FALLBACK. If the
+ * zero write is necessary and this flag is set, bdrv_co_truncate() will fail
+ * if efficient zero writes cannot be provided.
*/
+++ b/qemu-img.c
@@ -3836,7 +3836,7 @@ static int img_resize(int argc, char **argv)
* resizing, so pass @exact=true. It is of no use to report
* success when the image has not actually been resized.
*/
-ret = blk_truncate(blk, total_size, true, prealloc, );
+ret = blk_truncate(blk, total_size, true, prealloc, false, );
if (!ret) {
qprintf(quiet, "Image resized.\n");
} else {
Hmm - thought for a future patch (not this one): are there situations
where it may be faster to perform bulk pre-zeroing of the tail of a file
by performing two truncates (smaller and then larger) because we know
that just-added bytes from a truncate will read as zero? This may be
true for some file systems (but is not true for block devices, nor for
things like NBD that lack resize). Anyway, unrelated to this patch.
With the typo fixed,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH v3 2/8] block: Add no_fallback parameter to bdrv_co_truncate()
This adds a no_fallback parameter to bdrv_co_truncate(), bdrv_truncate()
and blk_truncate() in preparation for a fix that potentially needs to
zero-write the new area. no_fallback will use BDRV_REQ_NO_FALLBACK for
this operation and lets the truncate fail if an efficient zero write
isn't possible.
Only qmp_block_resize() passes true for this parameter because it is a
blocking monitor command, so we don't want to add more potentially slow
I/O operations to it than we already have.
All other users will accept even a slow fallback to avoid failure.
Signed-off-by: Kevin Wolf
---
include/block/block.h | 5 +++--
include/sysemu/block-backend.h | 2 +-
block/block-backend.c | 4 ++--
block/commit.c | 4 ++--
block/crypto.c | 4 ++--
block/io.c | 16
block/mirror.c | 2 +-
block/parallels.c | 6 +++---
block/qcow.c | 4 ++--
block/qcow2-refcount.c | 2 +-
block/qcow2.c | 19 +++
block/qed.c| 2 +-
block/raw-format.c | 2 +-
block/vdi.c| 2 +-
block/vhdx-log.c | 2 +-
block/vhdx.c | 6 +++---
block/vmdk.c | 10 ++
block/vpc.c| 2 +-
blockdev.c | 2 +-
qemu-img.c | 2 +-
qemu-io-cmds.c | 2 +-
tests/test-block-iothread.c| 6 +++---
22 files changed, 60 insertions(+), 46 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 1df9848e74..3e44677905 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -347,9 +347,10 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState
*bs,
void bdrv_refresh_filename(BlockDriverState *bs);
int coroutine_fn bdrv_co_truncate(BdrvChild *child, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp);
+ PreallocMode prealloc, bool no_fallback,
+ Error **errp);
int bdrv_truncate(BdrvChild *child, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp);
+ PreallocMode prealloc, bool no_fallback, Error **errp);
int64_t bdrv_nb_sectors(BlockDriverState *bs);
int64_t bdrv_getlength(BlockDriverState *bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index b198deca0b..487b29d13e 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -238,7 +238,7 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk,
int64_t offset,
int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
int bytes);
int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp);
+ PreallocMode prealloc, bool no_fallback, Error **errp);
int blk_pdiscard(BlockBackend *blk, int64_t offset, int bytes);
int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
int64_t pos, int size);
diff --git a/block/block-backend.c b/block/block-backend.c
index 8b8f2a80a0..fcc9d60cdb 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2073,14 +2073,14 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t
offset, const void *buf,
}
int blk_truncate(BlockBackend *blk, int64_t offset, bool exact,
- PreallocMode prealloc, Error **errp)
+ PreallocMode prealloc, bool no_fallback, Error **errp)
{
if (!blk_is_available(blk)) {
error_setg(errp, "No medium inserted");
return -ENOMEDIUM;
}
-return bdrv_truncate(blk->root, offset, exact, prealloc, errp);
+return bdrv_truncate(blk->root, offset, exact, prealloc, no_fallback,
errp);
}
static void blk_pdiscard_entry(void *opaque)
diff --git a/block/commit.c b/block/commit.c
index 23c90b3b91..f074181d83 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -155,7 +155,7 @@ static int coroutine_fn commit_run(Job *job, Error **errp)
}
if (base_len < len) {
-ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, NULL);
+ret = blk_truncate(s->base, len, false, PREALLOC_MODE_OFF, false,
NULL);
if (ret) {
goto out;
}
@@ -472,7 +472,7 @@ int bdrv_commit(BlockDriverState *bs)
* we must return an error */
if (length > backing_length) {
ret = blk_truncate(backing, length, false, PREALLOC_MODE_OFF,
- _err);
+ false, _err);
if (ret < 0) {
error_report_err(local_err);
goto ro_cleanup;
diff --git a/block/crypto.c b/block/crypto.c
index 24823835c1..0f28e8b4e1 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -114,7 +114,7 @@ static ssize_t
