Re: [PATCH 0/9] parallels: Refactor the code of images checks and fix a bug
On 18.08.2022 17:14, Alexander Ivanov wrote: Fix image inflation when offset in BAT is out of image. Replace whole BAT syncing by flushing only dirty blocks. Move all the checks outside the main check function in separate functions Use WITH_QEMU_LOCK_GUARD for simplier code. v4 changes: Move s->data_end fixing to parallels_co_check(). Split the check in parallels_open() and the fix in parallels_co_check() to two patches. Move offset convertation to parallels_set_bat_entry(). Fix 'ret' rewriting by bdrv_co_flush() results. Keep 'i' as uint32_t. Alexander Ivanov (9): parallels: Out of image offset in BAT leads to image inflation parallels: Fix data_end field value in parallels_co_check() parallels: create parallels_set_bat_entry_helper() to assign BAT value parallels: Use generic infrastructure for BAT writing in parallels_co_check() parallels: Move check of unclean image to a separate function parallels: Move check of cluster outside image to a separate function parallels: Move check of leaks to a separate function parallels: Move statistic collection to a separate function parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD block/parallels.c | 197 +- 1 file changed, 141 insertions(+), 56 deletions(-) That is VERY bad that: * you have lost (not applied) Reviewed-by: from previous iteration for unchanged patches * you have lost v4 tag in the subject Den
Re: [PATCH 9/9] parallels: Replace qemu_co_mutex_lock by WITH_QEMU_LOCK_GUARD
On 18.08.2022 17:14, Alexander Ivanov wrote:
Replace the way we use mutex in parallels_co_check() for simplier
and less error prone code.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 26 --
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index f19e86d5d2..173c5d3721 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -563,24 +563,22 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
BDRVParallelsState *s = bs->opaque;
int ret = 0;
-qemu_co_mutex_lock(&s->lock);
+WITH_QEMU_LOCK_GUARD(&s->lock) {
+parallels_check_unclean(bs, res, fix);
-parallels_check_unclean(bs, res, fix);
+ret = parallels_check_outside_image(bs, res, fix);
+if (ret < 0) {
+return ret;
+}
-ret = parallels_check_outside_image(bs, res, fix);
-if (ret < 0) {
-goto out;
-}
-
-ret = parallels_check_leak(bs, res, fix);
-if (ret < 0) {
-goto out;
-}
+ret = parallels_check_leak(bs, res, fix);
+if (ret < 0) {
+return ret;
+}
-parallels_collect_statistics(bs, res, fix);
+parallels_collect_statistics(bs, res, fix);
-out:
-qemu_co_mutex_unlock(&s->lock);
+}
if (ret == 0) {
this check is redundant from now on.
ret = bdrv_co_flush(bs);
Re: [PATCH 6/9] parallels: Move check of cluster outside image to a separate function
On 18.08.2022 17:14, Alexander Ivanov wrote:
We will add more and more checks so we need a better code structure
in parallels_co_check. Let each check performs in a separate loop
in a separate helper.
s->data_end fix relates to out-of-image check so move it
to the helper too.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 67 +++
1 file changed, 45 insertions(+), 22 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 3900a0f4a9..1c7626c867 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -438,6 +438,46 @@ static void parallels_check_unclean(BlockDriverState *bs,
}
}
+static int parallels_check_outside_image(BlockDriverState *bs,
+ BdrvCheckResult *res,
+ BdrvCheckMode fix)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t i;
+int64_t off, size;
+
+size = bdrv_getlength(bs->file->bs);
+if (size < 0) {
+res->check_errors++;
+return size;
+}
+
+for (i = 0; i < s->bat_size; i++) {
+off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (off > size) {
+fprintf(stderr, "%s cluster %u is outside image\n",
+fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
+res->corruptions++;
+if (fix & BDRV_FIX_ERRORS) {
+parallels_set_bat_entry(s, i, 0);
+res->corruptions_fixed++;
+}
+}
+}
+
+/*
+ * If there were an out-of-image cluster it would be repaired,
+ * but s->data_end still would point outside image.
+ * Fix s->data_end by the file size.
+ */
+size >>= BDRV_SECTOR_BITS;
+if (s->data_end > size) {
+s->data_end = size;
+}
and this is incorrect IMHO. In the next patch you could truncate the file
inside leak check and thus data_end will point to a wrong too lengthy
value.
+
+return 0;
+}
+
static int coroutine_fn parallels_co_check(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -457,6 +497,11 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
parallels_check_unclean(bs, res, fix);
+ret = parallels_check_outside_image(bs, res, fix);
+if (ret < 0) {
+goto out;
+}
+
res->bfi.total_clusters = s->bat_size;
res->bfi.compressed_clusters = 0; /* compression is not supported */
@@ -469,19 +514,6 @@ static int coroutine_fn parallels_co_check(BlockDriverState *bs,
continue;
}
-/* cluster outside the image */
-if (off > size) {
-fprintf(stderr, "%s cluster %u is outside image\n",
-fix & BDRV_FIX_ERRORS ? "Repairing" : "ERROR", i);
-res->corruptions++;
-if (fix & BDRV_FIX_ERRORS) {
-prev_off = 0;
-parallels_set_bat_entry(s, i, 0);
-res->corruptions_fixed++;
-continue;
-}
-}
-
res->bfi.allocated_clusters++;
if (off > high_off) {
high_off = off;
@@ -518,15 +550,6 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
res->leaks_fixed += count;
}
}
-/*
- * If there were an out-of-image cluster it would be repaired,
- * but s->data_end still would point outside image.
- * Fix s->data_end by the file size.
- */
-size >>= BDRV_SECTOR_BITS;
-if (s->data_end > size) {
-s->data_end = size;
-}
out:
qemu_co_mutex_unlock(&s->lock);
Re: [PATCH 2/9] parallels: Fix data_end field value in parallels_co_check()
On 18.08.2022 17:14, Alexander Ivanov wrote:
When an image is opened for check there is no error if an offset in the BAT
points outside the image. In such a way we can repair the image.
Out-of-image offsets are repaired in the check, but data_end field
still points outside. Fix this field by file size.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/block/parallels.c b/block/parallels.c
index c245ca35cd..24c05b95e8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -513,7 +513,15 @@ static int coroutine_fn
parallels_co_check(BlockDriverState *bs,
res->leaks_fixed += count;
}
}
-
+/*
+ * If there were an out-of-image cluster it would be repaired,
+ * but s->data_end still would point outside image.
+ * Fix s->data_end by the file size.
+ */
+size >>= BDRV_SECTOR_BITS;
+if (s->data_end > size) {
+s->data_end = size;
+}
out:
qemu_co_mutex_unlock(&s->lock);
return ret;
yes, but the comment is wrong. You could have adjustment to data_end
additionally once clusters outside of image are dropped - inside
leak check. Where data_end could be reduced. And this leads to
error further in the series.
Den
Re: [PATCH 1/2] block: use bdrv_is_sg() helper instead of raw bs->sg reading
On 8/17/22 11:37, Denis V. Lunev wrote: I believe that if the helper exists, it must be used always for reading of the value. It breaks expectations in the other case. Signed-off-by: Denis V. Lunev CC: Kevin Wolf CC: Hanna Reitz CC: Stefan Hajnoczi CC: Fam Zheng CC: Ronnie Sahlberg CC: Paolo Bonzini CC: Peter Lieven CC: Vladimir Sementsov-Ogievskiy Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH v10 18/21] job.c: enable job lock/unlock and remove Aiocontext locks
On 8/18/22 10:46, Emanuele Giuseppe Esposito wrote:
Am 17/08/2022 um 20:54 schrieb Vladimir Sementsov-Ogievskiy:
On 8/16/22 15:52, Emanuele Giuseppe Esposito wrote:
}
@@ -501,8 +481,12 @@ void job_unref_locked(Job *job)>
assert(!job->txn);
if (job->driver->free) {
+ AioContext *aio_context = job->aio_context;
job_unlock();
+ /* FIXME: aiocontext lock is required because cb calls
blk_unref */
+ aio_context_acquire(aio_context);
job->driver->free(job);
+ aio_context_release(aio_context);
So, job_unref_locked() must be called with aio_context not locked,
otherwise we dead-lock here? That should be documented in function
declaration comment.
For example in qemu-img.c in run_block_job() we do exactly that: call
job_unref_locked() inside aio-context lock critical seaction..
No, job_unref_locked has also status and refcnt and all the other fields
that need to be protectd. Only free must be called without job lock held.
I mean, don't we try here to acquire aio_context twice, when we call
job_unref_locked() with aio_context _already_ acquired?
You're right, so why do we need the AioContext lock in qemu-img then?
All jobs API don't need it, aio_poll seems not to need it either, and
progress API has its own lock.
I guess I could remove it?
Not sure, but hope that yes.
Anyway, trying to lock it twice will dead-lock, as I understand.
--
Best regards,
Vladimir
Re: [PATCH 2/2] block: make serializing requests functions 'void'
On 8/17/22 11:37, Denis V. Lunev wrote: Return codes of the following functions are never used in the code: * bdrv_wait_serialising_requests_locked * bdrv_wait_serialising_requests * bdrv_make_request_serialising Signed-off-by: Denis V. Lunev CC: Kevin Wolf CC: Hanna Reitz CC: Stefan Hajnoczi CC: Fam Zheng CC: Ronnie Sahlberg CC: Paolo Bonzini CC: Peter Lieven CC: Vladimir Sementsov-Ogievskiy Oh, good cleanup! Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [PATCH 1/2] block: pass OnOffAuto instead of bool to block_acct_setup()
On 8/17/22 12:28, Denis V. Lunev wrote:
We would have one more place for block_acct_setup() calling, which should
not corrupt original value.
Signed-off-by: Denis V. Lunev
CC: Peter Krempa
CC: Markus Armbruster
CC: John Snow
CC: Kevin Wolf
CC: Hanna Reitz
CC: Vladimir Sementsov-Ogievskiy
---
block/accounting.c | 24
blockdev.c | 17 ++---
include/block/accounting.h | 6 +++---
3 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/block/accounting.c b/block/accounting.c
index 2030851d79..73ac639656 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -40,11 +40,27 @@ void block_acct_init(BlockAcctStats *stats)
}
}
-void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
- bool account_failed)
+static bool bool_from_onoffauto(OnOffAuto val, bool def)
{
-stats->account_invalid = account_invalid;
-stats->account_failed = account_failed;
+switch (val) {
+case ON_OFF_AUTO_AUTO:
+return def;
+case ON_OFF_AUTO_ON:
+return true;
+case ON_OFF_AUTO_OFF:
+return false;
+default:
+abort();
+}
+}
+
+void block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
+ enum OnOffAuto account_failed)
+{
+stats->account_invalid = bool_from_onoffauto(account_invalid,
+ stats->account_invalid);
+stats->account_failed = bool_from_onoffauto(account_failed,
+stats->account_failed);
}
void block_acct_cleanup(BlockAcctStats *stats)
diff --git a/blockdev.c b/blockdev.c
index 9230888e34..392d9476e6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -455,6 +455,17 @@ static void extract_common_blockdev_options(QemuOpts
*opts, int *bdrv_flags,
}
}
+static OnOffAuto account_get_opt(QemuOpts *opts, const char *name)
+{
+if (!qemu_opt_find(opts, name)) {
+return ON_OFF_AUTO_AUTO;
+}
+if (qemu_opt_get_bool(opts, name, true)) {
+return ON_OFF_AUTO_ON;
+}
+return ON_OFF_AUTO_OFF;
+}
+
/* Takes the ownership of bs_opts */
static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
Error **errp)
@@ -462,7 +473,7 @@ static BlockBackend *blockdev_init(const char *file, QDict
*bs_opts,
const char *buf;
int bdrv_flags = 0;
int on_read_error, on_write_error;
-bool account_invalid, account_failed;
+OnOffAuto account_invalid, account_failed;
bool writethrough, read_only;
BlockBackend *blk;
BlockDriverState *bs;
@@ -496,8 +507,8 @@ static BlockBackend *blockdev_init(const char *file, QDict
*bs_opts,
/* extract parameters */
snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
-account_invalid = qemu_opt_get_bool(opts, "stats-account-invalid", true);
-account_failed = qemu_opt_get_bool(opts, "stats-account-failed", true);
+account_invalid = account_get_opt(opts, "stats-account-invalid");
+account_failed = account_get_opt(opts, "stats-account-failed");
Hm. Actually here you change default behavior from true to false.
In the next patch you fix it back, but better is to avoid the extra change
somehow.
writethrough = !qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, true);
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 878b4c3581..b9caad60d5 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -27,7 +27,7 @@
#include "qemu/timed-average.h"
#include "qemu/thread.h"
-#include "qapi/qapi-builtin-types.h"
+#include "qapi/qapi-types-common.h"
typedef struct BlockAcctTimedStats BlockAcctTimedStats;
typedef struct BlockAcctStats BlockAcctStats;
@@ -100,8 +100,8 @@ typedef struct BlockAcctCookie {
} BlockAcctCookie;
void block_acct_init(BlockAcctStats *stats);
-void block_acct_setup(BlockAcctStats *stats, bool account_invalid,
- bool account_failed);
+void block_acct_setup(BlockAcctStats *stats, enum OnOffAuto account_invalid,
+ enum OnOffAuto account_failed);
void block_acct_cleanup(BlockAcctStats *stats);
void block_acct_add_interval(BlockAcctStats *stats, unsigned interval_length);
BlockAcctTimedStats *block_acct_interval_next(BlockAcctStats *stats,
--
Best regards,
Vladimir
[RFC PATCH] qemu-options: try and clarify preferred block semantics
Try to correct any confusion about QEMU's Byzantine disk options by
laying out the preferred "modern" options as-per:
" (best: -device + -blockdev, 2nd obsolete syntax: -device +
-drive, 3rd obsolete syntax: -drive, 4th obsolete syntax: -hdNN)"
Signed-off-by: Alex Bennée
Cc: qemu-block@nongnu.org
Cc: Kevin Wolf
Cc: Hanna Reitz
Cc: Daniel P. Berrange
Cc: Thomas Huth
---
qemu-options.hx | 13 +
1 file changed, 13 insertions(+)
diff --git a/qemu-options.hx b/qemu-options.hx
index 3f23a42fa8..d57239d364 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1105,6 +1105,19 @@ DEFHEADING()
DEFHEADING(Block device options:)
+SRST
+The QEMU block device handling options have a long history and
+have gone through several iterations as the feature set and complexity
+of the block layer have grown. Many online guides to QEMU often
+reference older and deprecated options which can lead to confusion.
+
+The recommended modern way to describe disks is to use combination of
+``-device`` to specify the hardware device and ``-blockdev`` to
+describe the backend. The device defines what the guest sees and the
+backend describes how QEMU handles the data.
+
+ERST
+
DEF("fda", HAS_ARG, QEMU_OPTION_fda,
"-fda/-fdb file use 'file' as floppy disk 0/1 image\n", QEMU_ARCH_ALL)
DEF("fdb", HAS_ARG, QEMU_OPTION_fdb, "", QEMU_ARCH_ALL)
--
2.30.2
