Re: [PATCH v3 2/4] qemu-img: make --block-size optional for compare --stat
On Thu, Oct 28, 2021 at 12:24:39PM +0200, Vladimir Sementsov-Ogievskiy wrote:
> Let's detect block-size automatically if not specified by user:
>
> If both files define cluster-size, use minimum to be more precise.
> If both files don't specify cluster-size, use default of 64K
> If only one file specify cluster-size, just use it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> ---
> docs/tools/qemu-img.rst | 7 +++-
> qemu-img.c | 71 +
> qemu-img-cmds.hx| 4 +--
> 3 files changed, 72 insertions(+), 10 deletions(-)
>
Reviewed-by: Eric Blake
> +if (cluster_size1 > 0 && cluster_size2 > 0) {
> +if (cluster_size1 == cluster_size2) {
> +block_size = cluster_size1;
> +} else {
> +block_size = MIN(cluster_size1, cluster_size2);
> +qprintf(quiet, "%s and %s have different cluster sizes: %d and
> %d "
> +"respectively. Using minimum as block-size for "
> +"accuracy: %d. %s\n",
> +fname1, fname2, cluster_size1,
> +cluster_size2, block_size, note);
Results in a long line; I don't know if it's worth trying to wrap it
(if we had a generic utility function that took arbitrary text, then
outputs it wrapped to the user's current terminal column width, I'd
suggest using that instead - but that's NOT something I expect you to
write, and I don't know if glib has such a utility).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 3/4] qemu-img: add --shallow option for qemu-img compare
On Thu, Oct 28, 2021 at 12:24:40PM +0200, Vladimir Sementsov-Ogievskiy wrote: > Allow compare only top images of backing chains. This is useful to Allow the comparison of only the top image of backing chains. > compare images with same backing file or to compare incremental images > from the chain of incremental backups with "--stat" option. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > Reviewed-by: Hanna Reitz > --- > docs/tools/qemu-img.rst | 9 - > qemu-img.c | 8 ++-- > qemu-img-cmds.hx| 4 ++-- > 3 files changed, 16 insertions(+), 5 deletions(-) > > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst > index 9bfdd93d6c..c6e9306c70 100644 > --- a/docs/tools/qemu-img.rst > +++ b/docs/tools/qemu-img.rst > @@ -176,6 +176,13 @@ Parameters to compare subcommand: > - If both files don't specify cluster-size, use default of 64K > - If only one file specifies cluster-size, just use that. > > +.. option:: --shallow > + > + This option prevents opening and comparing any backing files. > + This is useful to compare images with same backing file or to compare > + incremental images from the chain of incremental backups with > + ``--stat`` option. If I understand correctly, your implementation makes --shallow an all-or-none option (either both images are compared shallow, or neither is). Does it make sense to make it a per-image option (--shallow-source, --shallow-dest), where --shallow is a synonym for the more verbose --shallow-source --shallow-dest? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [PATCH] nbd/server: Advertise MULTI_CONN for shared writable exports
On Thu, Oct 28, 2021 at 04:37:36PM +0200, Kevin Wolf wrote:
> Am 27.08.2021 um 17:09 hat Eric Blake geschrieben:
> > According to the NBD spec, a server advertising
> > NBD_FLAG_CAN_MULTI_CONN promises that multiple client connections will
> > not see any cache inconsistencies: when properly separated by a single
> > flush, actions performed by one client will be visible to another
> > client, regardless of which client did the flush. We satisfy these
> > conditions in qemu because our block layer serializes any overlapping
> > operations (see bdrv_find_conflicting_request and friends): no matter
> > which client performs a flush, parallel requests coming from distinct
> > NBD clients will still be well-ordered by the time they are passed on
> > to the underlying device, with no caching in qemu proper to allow
> > stale results to leak after a flush.
> >
> > We don't want to advertise MULTI_CONN when we know that a second
> > client can connect (which is the default for qemu-nbd, but not for QMP
> > nbd-server-add),
>
> Do you mean when a second client _can't_ connect?
Oops, yes. The default for qemu-nbd is a single client (you have to
request -e for more than one), so a second client can't connect; for
nbd-server-add it is unlimited clients [1].
>
> > so it does require a QAPI addition. But other than
> > that, the actual change to advertise the bit for writable servers is
> > fairly small. The harder part of this patch is setting up an iotest
> > to demonstrate behavior of multiple NBD clients to a single server.
> > It might be possible with parallel qemu-io processes, but concisely
> > managing that in shell is painful.
>
> I think it should be fairly straightforward in a Python test case.
Probably, but my python is rather weak for writing such a case
off-hand from scratch. Is there an existing test that you are aware of
that might be easy to copy-and-paste from?
>
> Another option is using a single QEMU or QSD instance that has multiple
> -blockdev for the same NBD server. For the server these are multiple
> clients, even if all connnection come from a single process.
>
> > I found it easier to do by relying
> > on the libnbd project's nbdsh, which means this test will be skipped
> > on platforms where that is not available.
> >
> > Signed-off-by: Eric Blake
>
> > diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
> > index 10ce098a29bf..d03910f1e9eb 100644
> > --- a/docs/interop/nbd.txt
> > +++ b/docs/interop/nbd.txt
> > @@ -68,3 +68,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:",
> > NBD_CMD_CACHE
> > * 4.2: NBD_FLAG_CAN_MULTI_CONN for shareable read-only exports,
> > NBD_CMD_FLAG_FAST_ZERO
> > * 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"
> > +* 6.2: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports
> > diff --git a/docs/tools/qemu-nbd.rst b/docs/tools/qemu-nbd.rst
> > index 5643da26e982..81be32164a55 100644
> > --- a/docs/tools/qemu-nbd.rst
> > +++ b/docs/tools/qemu-nbd.rst
> > @@ -138,8 +138,7 @@ driver options if ``--image-opts`` is specified.
> > .. option:: -e, --shared=NUM
> >
> >Allow up to *NUM* clients to share the device (default
> > - ``1``), 0 for unlimited. Safe for readers, but for now,
> > - consistency is not guaranteed between multiple writers.
> > + ``1``), 0 for unlimited.
> >
> > .. option:: -t, --persistent
>
> If qemu-nbd supports a maximum number of connections rather than just a
> bool...
>
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index 0ed63442a819..b2085a9fdd4c 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> > @@ -95,11 +95,15 @@
> > #the metadata context name "qemu:allocation-depth" to
> > #inspect allocation details. (since 5.2)
> > #
> > +# @shared: True if the server should advertise that multiple clients may
> > +# connect, default false. (since 6.2)
> > +#
> > # Since: 5.2
> > ##
> > { 'struct': 'BlockExportOptionsNbd',
> >'base': 'BlockExportOptionsNbdBase',
> > - 'data': { '*bitmaps': ['str'], '*allocation-depth': 'bool' } }
> > + 'data': { '*bitmaps': ['str'], '*allocation-depth': 'bool',
> > + '*shared': 'bool' } }
>
> ...wouldn't it be better to mirror this in the QAPI interface?
Yeah, now that you mention it. (In fact, before I got to this part of
the email, at point [1] above I was trying to look in
block-export.json to see whether there is any way to use QMP to expose
less than unlimited clients, and couldn't find it - because it isn't
there)
>
> I think eventually we want to add everything missing to the built-in NBD
> server and then change qemu-nbd to use it instead of managing the
> connections itself. So I'm not sure if diverging here is a good idea.
That argument alone makes it sound like it is worth respinning this
series to at least pick up on exposing max-clients through QMP, so
that qemu-nbd and QMP have the same knobs (even if those knobs have
different defaul
Re: [PATCH v3 1/4] qemu-img: implement compare --stat
On Thu, Oct 28, 2021 at 12:24:38PM +0200, Vladimir Sementsov-Ogievskiy wrote: > With new option qemu-img compare will not stop at first mismatch, but With the new --stat option, qemu-img compare... [Of course the subject line should be self-contained: think 'git shortlog' and friends. But I also argue the commit body should generally be self-contained, rather than assuming you read the subject line - why? because there are enough user interfaces out there (including my email program) where the subject line is displayed visually far away from the body, to the point that it is not always safe to assume someone read the subject line. If it feels too redundant, visual tricks like starting the body with '...' force the reader to realize they need to read the subject for full context, but that comes with its own set of oddities] > instead calculate statistics: how many clusters with different data, > how many clusters with equal data, how many clusters were unallocated > but become data and so on. > > We compare images chunk by chunk. Chunk size depends on what > block_status returns for both images. It may return less than cluster > (remember about qcow2 subclusters), it may return more than cluster (if > several consecutive clusters share same status). Finally images may > have different cluster sizes. This all leads to ambiguity in how to > finally compare the data. > > What we can say for sure is that, when we compare two qcow2 images with > same cluster size, we should compare clusters with data separately. > Otherwise, if we for example compare 10 consecutive clusters of data > where only one byte differs we'll report 10 different clusters. > Expected result in this case is 1 different cluster and 9 equal ones. > > So, to serve this case and just to have some defined rule let's do the > following: > > 1. Select some block-size for compare procedure. In this commit it must >be specified by user, next commit will add some automatic logic and >make --block-size optional. > > 2. Go chunk-by-chunk using block_status as we do now with only one >differency: difference >If block_status() returns DATA region that intersects block-size >aligned boundary, crop this region at this boundary. > > This way it's still possible to compare less than cluster and report > subcluster-level accuracy, but we newer compare more than one cluster > of data. > > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > docs/tools/qemu-img.rst | 18 +++- > qemu-img.c | 210 +--- > qemu-img-cmds.hx| 4 +- > 3 files changed, 216 insertions(+), 16 deletions(-) > > diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst > index d58980aef8..8b6b799dd4 100644 > --- a/docs/tools/qemu-img.rst > +++ b/docs/tools/qemu-img.rst > @@ -159,6 +159,18 @@ Parameters to compare subcommand: > >Strict mode - fail on different image size or sector allocation > > +.. option:: --stat > + > + Instead of exiting on the first mismatch, compare the whole images > + and print statistics on the amount of different pairs of blocks, > + based on their block status and whether they are equal or not. > + > +.. option:: --block-size BLOCK_SIZE > + > + Block size for comparing with ``--stat``. This doesn't guarantee an > + exact size for comparing chunks, but it does guarantee that those > + chunks will never cross a block-size-aligned boundary. Would it be safe to require that this be a power-of-2? (If anything, requiring now and relaxing later is better than allowing any number now but later wishing it were a sane number) > + > Parameters to convert subcommand: > > .. program:: qemu-img-convert > @@ -378,7 +390,7 @@ Command description: > >The rate limit for the commit process is specified by ``-r``. > > -.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] > [-T SRC_CACHE] [-p] [-q] [-s] [-U] FILENAME1 FILENAME2 > +.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] > [-T SRC_CACHE] [-p] [-q] [-s] [-U] [--stat --block-size BLOCK_SIZE] FILENAME1 > FILENAME2 > >Check if two images have the same content. You can compare images with >different format or settings. > @@ -405,9 +417,9 @@ Command description: >The following table sumarizes all exit codes of the compare subcommand: > >0 > -Images are identical (or requested help was printed) > +Images are identical (or requested help was printed, or ``--stat`` was > used) >1 > -Images differ > +Images differ (1 is never returned when ``--stat`` option specified) Why not? Even when we print statistics, it's still easy to tell if the data appears identical (block status might differ, but the guest would see the same content), or had at least one difference. >2 > Error on opening an image >3 > diff --git a/qemu-img.c b/qemu-img.c > index f036a1d428..0cb7cebe91 100644 > --- a/qemu-img.c > +++ b/qemu-im
Re: [PATCH v2 8/9] qapi: Factor out compat_policy_input_ok()
Markus Armbruster writes:
> The code to check policy for handling deprecated input is triplicated.
> Factor it out into compat_policy_input_ok() before I mess with it in
> the next commit.
>
> Signed-off-by: Markus Armbruster
> Reviewed-by: Philippe Mathieu-Daudé
> ---
> include/qapi/compat-policy.h | 7 +
> qapi/qapi-visit-core.c | 18 +
> qapi/qmp-dispatch.c | 51 +++-
> qapi/qobject-input-visitor.c | 19 +++---
> 4 files changed, 55 insertions(+), 40 deletions(-)
>
> diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
> index 1083f95122..8b7b25c0b5 100644
> --- a/include/qapi/compat-policy.h
> +++ b/include/qapi/compat-policy.h
> @@ -13,10 +13,17 @@
> #ifndef QAPI_COMPAT_POLICY_H
> #define QAPI_COMPAT_POLICY_H
>
> +#include "qapi/error.h"
> #include "qapi/qapi-types-compat.h"
>
> extern CompatPolicy compat_policy;
>
> +bool compat_policy_input_ok(unsigned special_features,
> +const CompatPolicy *policy,
> +ErrorClass error_class,
> +const char *kind, const char *name,
> +Error **errp);
> +
> /*
> * Create a QObject input visitor for @obj for use with QMP
> *
> diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
> index 34c59286b2..6c13510a2b 100644
> --- a/qapi/qapi-visit-core.c
> +++ b/qapi/qapi-visit-core.c
> @@ -13,6 +13,7 @@
> */
>
> #include "qemu/osdep.h"
> +#include "qapi/compat-policy.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> #include "qapi/visitor.h"
> @@ -409,18 +410,11 @@ static bool input_type_enum(Visitor *v, const char
> *name, int *obj,
> }
>
> if (lookup->special_features
> -&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
> -switch (v->compat_policy.deprecated_input) {
> -case COMPAT_POLICY_INPUT_ACCEPT:
> -break;
> -case COMPAT_POLICY_INPUT_REJECT:
> -error_setg(errp, "Deprecated value '%s' disabled by policy",
> - enum_str);
> -return false;
> -case COMPAT_POLICY_INPUT_CRASH:
> -default:
> -abort();
> -}
> +&& !compat_policy_input_ok(lookup->special_features[value],
> + &v->compat_policy,
> + ERROR_CLASS_GENERIC_ERROR,
> + "value", enum_str, errp)) {
> +return false;
> }
>
> *obj = value;
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 8cca18c891..e29ade134c 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -28,6 +28,40 @@
>
> CompatPolicy compat_policy;
>
> +static bool compat_policy_input_ok1(const char *adjective,
> +CompatPolicyInput policy,
> +ErrorClass error_class,
> +const char *kind, const char *name,
> +Error **errp)
> +{
> +switch (policy) {
> +case COMPAT_POLICY_INPUT_ACCEPT:
> +return true;
> +case COMPAT_POLICY_INPUT_REJECT:
> +error_set(errp, error_class, "%s %s %s disabled by policy",
> + adjective, kind, name);
> +return false;
> +case COMPAT_POLICY_INPUT_CRASH:
> +default:
> +abort();
> +}
> +}
> +
> +bool compat_policy_input_ok(unsigned special_features,
> +const CompatPolicy *policy,
> +ErrorClass error_class,
> +const char *kind, const char *name,
> +Error **errp)
> +{
> +if ((special_features & 1u << QAPI_DEPRECATED)
> +&& !compat_policy_input_ok1("Deprecated",
> +policy->deprecated_input,
> +error_class, kind, name, errp)) {
> +return false;
> +}
> +return true;
> +}
> +
> Visitor *qobject_input_visitor_new_qmp(QObject *obj)
> {
> Visitor *v = qobject_input_visitor_new(obj);
I'm moving the new functions along with @compat_policy to qapi-util.c,
so the QAPI visitors survive linking without qmp-dispatch.o.
> @@ -176,19 +210,10 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject
> *request,
>"The command %s has not been found", command);
> goto out;
> }
> -if (cmd->special_features & 1u << QAPI_DEPRECATED) {
> -switch (compat_policy.deprecated_input) {
> -case COMPAT_POLICY_INPUT_ACCEPT:
> -break;
> -case COMPAT_POLICY_INPUT_REJECT:
> -error_set(&err, ERROR_CLASS_COMMAND_NOT_FOUND,
> - "Deprecated command %s disabled by policy",
> - command);
> -goto out;
> -case COMPAT_POLICY_INPUT_CRASH:
> -default:
> -
Re: [PATCH v2 8/9] qapi: Factor out compat_policy_input_ok()
On 10/29/21 18:55, Markus Armbruster wrote:
> Markus Armbruster writes:
>
>> The code to check policy for handling deprecated input is triplicated.
>> Factor it out into compat_policy_input_ok() before I mess with it in
>> the next commit.
>>
>> Signed-off-by: Markus Armbruster
>> Reviewed-by: Philippe Mathieu-Daudé
>> ---
>> include/qapi/compat-policy.h | 7 +
>> qapi/qapi-visit-core.c | 18 +
>> qapi/qmp-dispatch.c | 51 +++-
>> qapi/qobject-input-visitor.c | 19 +++---
>> 4 files changed, 55 insertions(+), 40 deletions(-)
>> +bool compat_policy_input_ok(unsigned special_features,
>> +const CompatPolicy *policy,
>> +ErrorClass error_class,
>> +const char *kind, const char *name,
>> +Error **errp)
>> +{
>> +if ((special_features & 1u << QAPI_DEPRECATED)
>> +&& !compat_policy_input_ok1("Deprecated",
>> +policy->deprecated_input,
>> +error_class, kind, name, errp)) {
>> +return false;
>> +}
>> +return true;
>> +}
>> +
>> Visitor *qobject_input_visitor_new_qmp(QObject *obj)
>> {
>> Visitor *v = qobject_input_visitor_new(obj);
>
> I'm moving the new functions along with @compat_policy to qapi-util.c,
> so the QAPI visitors survive linking without qmp-dispatch.o.
OK. I am waiting your series to get in before respining my QAPI
sysemu/user series which will re-scramble meson.build here.
Probably too late for this release. Not super important, it just
saves energy avoiding generating / building unused files.
[RFC PATCH 11/15] jobs: remove aiocontext locks since the functions are under BQL
In preparation to the job_lock/unlock patch, remove these
aiocontext locks.
The main reason these two locks are removed here is because
they are inside a loop iterating on the jobs list. Once the
job_lock is added, it will have to protect the whole loop,
wrapping also the aiocontext acquire/release.
We don't want this, as job_lock can only be *wrapped by*
the aiocontext lock, and not vice-versa, to avoid deadlocks.
Signed-off-by: Emanuele Giuseppe Esposito
---
blockdev.c | 4
job-qmp.c | 4
2 files changed, 8 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index d9bf842a81..67b55eec11 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3719,15 +3719,11 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
for (job = block_job_next(NULL); job; job = block_job_next(job)) {
BlockJobInfo *value;
-AioContext *aio_context;
if (block_job_is_internal(job)) {
continue;
}
-aio_context = blk_get_aio_context(job->blk);
-aio_context_acquire(aio_context);
value = block_job_query(job, errp);
-aio_context_release(aio_context);
if (!value) {
qapi_free_BlockJobInfoList(head);
return NULL;
diff --git a/job-qmp.c b/job-qmp.c
index 829a28aa70..a67745 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -173,15 +173,11 @@ JobInfoList *qmp_query_jobs(Error **errp)
for (job = job_next(NULL); job; job = job_next(job)) {
JobInfo *value;
-AioContext *aio_context;
if (job_is_internal(job)) {
continue;
}
-aio_context = job->aio_context;
-aio_context_acquire(aio_context);
value = job_query_single(job, errp);
-aio_context_release(aio_context);
if (!value) {
qapi_free_JobInfoList(head);
return NULL;
--
2.27.0
[RFC PATCH 14/15] jobs: add missing job locks to replace aiocontext lock
find_block_job() and find_job() cannot be handled like
all other functions in the previous commit: in order
to avoid unneccessary job lock/unlock, replace the
aiocontext lock with the job_lock.
However, once we start dropping these aiocontex locks
we also break the assumptions of the callees in the job
API, many of which assume the aiocontext lock is held.
To keep this commit simple, handle the rest of the aiocontext
removal in the next commit.
Signed-off-by: Emanuele Giuseppe Esposito
---
blockdev.c | 52 ++--
job-qmp.c | 46 --
2 files changed, 34 insertions(+), 64 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index c5a835d9ed..592ed4a0fc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3318,48 +3318,43 @@ out:
aio_context_release(aio_context);
}
-/* Get a block job using its ID and acquire its AioContext */
-static BlockJob *find_block_job(const char *id, AioContext **aio_context,
-Error **errp)
+/* Get a block job using its ID. Returns with job_lock held on success */
+static BlockJob *find_block_job(const char *id, Error **errp)
{
BlockJob *job;
assert(id != NULL);
-*aio_context = NULL;
+job_lock();
job = block_job_get(id);
if (!job) {
error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
"Block job '%s' not found", id);
+job_unlock();
return NULL;
}
-*aio_context = blk_get_aio_context(job->blk);
-aio_context_acquire(*aio_context);
-
return job;
}
void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
{
-AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job = find_block_job(device, errp);
if (!job) {
return;
}
block_job_set_speed(job, speed, errp);
-aio_context_release(aio_context);
+job_unlock();
}
void qmp_block_job_cancel(const char *device,
bool has_force, bool force, Error **errp)
{
-AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job = find_block_job(device, errp);
if (!job) {
return;
@@ -3378,13 +3373,12 @@ void qmp_block_job_cancel(const char *device,
trace_qmp_block_job_cancel(job);
job_user_cancel(&job->job, force, errp);
out:
-aio_context_release(aio_context);
+job_unlock();
}
void qmp_block_job_pause(const char *device, Error **errp)
{
-AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job = find_block_job(device, errp);
if (!job) {
return;
@@ -3392,13 +3386,12 @@ void qmp_block_job_pause(const char *device, Error
**errp)
trace_qmp_block_job_pause(job);
job_user_pause(&job->job, errp);
-aio_context_release(aio_context);
+job_unlock();
}
void qmp_block_job_resume(const char *device, Error **errp)
{
-AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job = find_block_job(device, errp);
if (!job) {
return;
@@ -3406,13 +3399,12 @@ void qmp_block_job_resume(const char *device, Error
**errp)
trace_qmp_block_job_resume(job);
job_user_resume(&job->job, errp);
-aio_context_release(aio_context);
+job_unlock();
}
void qmp_block_job_complete(const char *device, Error **errp)
{
-AioContext *aio_context;
-BlockJob *job = find_block_job(device, &aio_context, errp);
+BlockJob *job = find_block_job(device, errp);
if (!job) {
return;
@@ -3420,13 +3412,12 @@ void qmp_block_job_complete(const char *device, Error
**errp)
trace_qmp_block_job_complete(job);
job_complete(&job->job, errp);
-aio_context_release(aio_context);
+job_unlock();
}
void qmp_block_job_finalize(const char *id, Error **errp)
{
-AioContext *aio_context;
-BlockJob *job = find_block_job(id, &aio_context, errp);
+BlockJob *job = find_block_job(id, errp);
if (!job) {
return;
@@ -3436,20 +3427,13 @@ void qmp_block_job_finalize(const char *id, Error
**errp)
job_ref(&job->job);
job_finalize(&job->job, errp);
-/*
- * Job's context might have changed via job_finalize (and job_txn_apply
- * automatically acquires the new one), so make sure we release the correct
- * one.
- */
-aio_context = blk_get_aio_context(job->blk);
job_unref(&job->job);
-aio_context_release(aio_context);
+job_unlock();
}
void qmp_block_job_dismiss(const char *id, Error **errp)
{
-AioContext *aio_context;
-BlockJob *bjob = find_block_job(id, &aio_context, errp);
+BlockJob *bjob = find_block_job(id, errp);
Job *job;
if (!bjob) {
@@ -3459,7 +3443,7 @@ void qmp_block_job_dismiss(const char *id, Error **errp)
trace_qmp_block_job_dismiss(bjob);
[RFC PATCH 08/15] job.c: minor adjustments in preparation to job-driver
job_event_* functions can be all static, as they are not used
outside job.c
Add also missing notifier initialization for the on_idle
list in job_create().
Signed-off-by: Emanuele Giuseppe Esposito
---
job.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/job.c b/job.c
index db7ad79745..88d911f2d7 100644
--- a/job.c
+++ b/job.c
@@ -464,6 +464,7 @@ void *job_create(const char *job_id, const JobDriver
*driver, JobTxn *txn,
notifier_list_init(&job->on_finalize_completed);
notifier_list_init(&job->on_pending);
notifier_list_init(&job->on_ready);
+notifier_list_init(&job->on_idle);
job_state_transition(job, JOB_STATUS_CREATED);
aio_timer_init(qemu_get_aio_context(), &job->sleep_timer,
@@ -527,12 +528,20 @@ void job_progress_increase_remaining(Job *job, uint64_t
delta)
progress_increase_remaining(&job->progress, delta);
}
-void job_event_cancelled(Job *job)
+/**
+ * To be called when a cancelled job is finalised.
+ * Called with job_mutex held.
+ */
+static void job_event_cancelled(Job *job)
{
notifier_list_notify(&job->on_finalize_cancelled, job);
}
-void job_event_completed(Job *job)
+/**
+ * To be called when a successfully completed job is finalised.
+ * Called with job_mutex held.
+ */
+static void job_event_completed(Job *job)
{
notifier_list_notify(&job->on_finalize_completed, job);
}
--
2.27.0
[RFC PATCH 10/15] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
Same as AIO_WAIT_WHILE macro, but if we are in the Main loop
do not release and then acquire ctx_ 's aiocontext.
Once all Aiocontext locks go away, this macro will replace
AIO_WAIT_WHILE.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/block/aio-wait.h | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index b39eefb38d..ff27fe4eab 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -59,10 +59,11 @@ typedef struct {
extern AioWait global_aio_wait;
/**
- * AIO_WAIT_WHILE:
+ * _AIO_WAIT_WHILE:
* @ctx: the aio context, or NULL if multiple aio contexts (for which the
* caller does not hold a lock) are involved in the polling condition.
* @cond: wait while this conditional expression is true
+ * @unlock: whether to unlock and then lock again @ctx
*
* Wait while a condition is true. Use this to implement synchronous
* operations that require event loop activity.
@@ -75,7 +76,7 @@ extern AioWait global_aio_wait;
* wait on conditions between two IOThreads since that could lead to deadlock,
* go via the main loop instead.
*/
-#define AIO_WAIT_WHILE(ctx, cond) ({ \
+#define _AIO_WAIT_WHILE(ctx, cond, unlock) ({ \
bool waited_ = false; \
AioWait *wait_ = &global_aio_wait; \
AioContext *ctx_ = (ctx); \
@@ -90,11 +91,11 @@ extern AioWait global_aio_wait;
assert(qemu_get_current_aio_context() == \
qemu_get_aio_context());\
while ((cond)) { \
-if (ctx_) {\
+if (unlock && ctx_) { \
aio_context_release(ctx_); \
} \
aio_poll(qemu_get_aio_context(), true);\
-if (ctx_) {\
+if (unlock && ctx_) { \
aio_context_acquire(ctx_); \
} \
waited_ = true;\
@@ -103,6 +104,12 @@ extern AioWait global_aio_wait;
qatomic_dec(&wait_->num_waiters); \
waited_; })
+#define AIO_WAIT_WHILE(ctx, cond) \
+_AIO_WAIT_WHILE(ctx, cond, true)
+
+#define AIO_WAIT_WHILE_UNLOCKED(ctx, cond) \
+_AIO_WAIT_WHILE(ctx, cond, false)
+
/**
* aio_wait_kick:
* Wake up the main thread if it is waiting on AIO_WAIT_WHILE(). During
--
2.27.0
[RFC PATCH 05/15] job-monitor.h: define the job monitor API
These functions assume that the job lock is held by the caller, to avoid TOC/TOU conditions. Introduce also additional helpers that define _locked functions (useful when the job_mutex is globally applied). Signed-off-by: Emanuele Giuseppe Esposito --- include/qemu/job-monitor.h | 61 ++ job.c | 22 -- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/include/qemu/job-monitor.h b/include/qemu/job-monitor.h index b5b41a7548..d92bc4f39d 100644 --- a/include/qemu/job-monitor.h +++ b/include/qemu/job-monitor.h @@ -28,6 +28,21 @@ #include "job-common.h" +/* + * Job monitor API. + * + * These functions use are used by the QEMU monitor, for example + * to execute QMP commands. The monitor is aware of the job_mutex + * presence, so these functions assume it is held by the caller + * to protect job fields (see job-common.h). + * This prevents TOC/TOU bugs, allowing the caller to hold the + * lock between a check in the job state and the actual action. + * + * Therefore, each function in this API that needs protection + * must have the comment + * "Called between job_lock and job_unlock." + */ + /** * Allocate and return a new job transaction. Jobs can be added to the * transaction using job_txn_add_job(). @@ -56,18 +71,24 @@ void job_txn_unref(JobTxn *txn); * the reference that is automatically grabbed here. * * If @txn is NULL, the function does nothing. + * + * Called between job_lock and job_unlock. */ void job_txn_add_job(JobTxn *txn, Job *job); /** * Add a reference to Job refcnt, it will be decreased with job_unref, and then * be freed if it comes to be the last reference. + * + * Called between job_lock and job_unlock. */ void job_ref(Job *job); /** * Release a reference that was previously acquired with job_ref() or * job_create(). If it's the last reference to the object, it will be freed. + * + * Called between job_lock and job_unlock. */ void job_unref(Job *job); @@ -75,6 +96,8 @@ void job_unref(Job *job); * Conditionally enter the job coroutine if the job is ready to run, not * already busy and fn() returns true. fn() is called while under the job_lock * critical section. + * + * Called between job_lock and job_unlock, but it releases the lock temporarly. */ void job_enter_cond(Job *job, bool(*fn)(Job *job)); @@ -85,6 +108,7 @@ bool job_is_internal(Job *job); /** * Returns whether the job is in a completed state. + * Called between job_lock and job_unlock. */ bool job_is_completed(Job *job); @@ -92,28 +116,36 @@ bool job_is_completed(Job *job); * Request @job to pause at the next pause point. Must be paired with * job_resume(). If the job is supposed to be resumed by user action, call * job_user_pause() instead. + * + * Called between job_lock and job_unlock. */ void job_pause(Job *job); /** * Resumes a @job paused with job_pause. + * Called between job_lock and job_unlock. */ void job_resume(Job *job); /** * Asynchronously pause the specified @job. * Do not allow a resume until a matching call to job_user_resume. + * + * Called between job_lock and job_unlock. */ void job_user_pause(Job *job, Error **errp); /** * Returns true if the job is user-paused. + * Called between job_lock and job_unlock. */ bool job_user_paused(Job *job); /** * Resume the specified @job. * Must be paired with a preceding job_user_pause. + * + * Called between job_lock and job_unlock. */ void job_user_resume(Job *job, Error **errp); @@ -122,6 +154,8 @@ void job_user_resume(Job *job, Error **errp); * first one if @job is %NULL. * * Returns the requested job, or %NULL if there are no more jobs left. + * + * Called between job_lock and job_unlock. */ Job *job_next(Job *job); @@ -129,6 +163,8 @@ Job *job_next(Job *job); * Get the job identified by @id (which must not be %NULL). * * Returns the requested job, or %NULL if it doesn't exist. + * + * Called between job_lock and job_unlock. */ Job *job_get(const char *id); @@ -136,23 +172,30 @@ Job *job_get(const char *id); * Check whether the verb @verb can be applied to @job in its current state. * Returns 0 if the verb can be applied; otherwise errp is set and -EPERM * returned. + * + * Called between job_lock and job_unlock. */ int job_apply_verb(Job *job, JobVerb verb, Error **errp); /** * Asynchronously complete the specified @job. + * Called between job_lock and job_unlock, but it releases the lock temporarly. */ void job_complete(Job *job, Error **errp); /** * Asynchronously cancel the specified @job. If @force is true, the job should * be cancelled immediately without waiting for a consistent state. + * + * Called between job_lock and job_unlock. */ void job_cancel(Job *job, bool force); /** * Cancels the specified job like job_cancel(), but may refuse to do so if the * operation isn't meaningful in the current state of the job. + *
[RFC PATCH 12/15] jobs: protect jobs with job_lock/unlock
Introduce the job locking mechanism through the whole job API,
following the comments and requirements of job-monitor (assume
lock is held) and job-driver (lock is not held).
At this point, we do not care if the job lock is inside or outside
the aiocontext. The aiocontext is going away and it is useless
to add additional temporary job_lock/unlock pairs just to keep
this commit valid.
For example:
/* assumes job_lock *not* held */
static void job_exit(void *opaque) {
job_lock();
job_ref(job); // assumes job_lock held
aio_context_acquire(job->aio_context); // we do not want this!
However, adding an additional unlock after job_ref() and lock
under the aiocontext acquire seems unnecessary, as it will all
go away in the next commits.
Signed-off-by: Emanuele Giuseppe Esposito
---
block.c | 6 ++
block/mirror.c | 8 +-
block/replication.c | 4 +
blockdev.c | 13 +++
blockjob.c | 42 +-
job-qmp.c | 4 +
job.c | 190
monitor/qmp-cmds.c | 2 +
qemu-img.c | 8 +-
9 files changed, 232 insertions(+), 45 deletions(-)
diff --git a/block.c b/block.c
index da80e89ad4..a6dcd9eb36 100644
--- a/block.c
+++ b/block.c
@@ -4826,7 +4826,9 @@ static void bdrv_close(BlockDriverState *bs)
void bdrv_close_all(void)
{
+job_lock();
assert(job_next(NULL) == NULL);
+job_unlock();
assert(qemu_in_main_thread());
/* Drop references from requests still in flight, such as canceled block
@@ -5965,6 +5967,8 @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp)
}
}
+job_lock();
+
for (job = block_job_next(NULL); job; job = block_job_next(job)) {
GSList *el;
@@ -5975,6 +5979,8 @@ XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp)
}
}
+job_unlock();
+
QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) {
xdbg_graph_add_node(gr, bs, X_DBG_BLOCK_GRAPH_NODE_TYPE_BLOCK_DRIVER,
bs->node_name);
diff --git a/block/mirror.c b/block/mirror.c
index 00089e519b..5c4445a8c6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -653,7 +653,7 @@ static int mirror_exit_common(Job *job)
BlockDriverState *target_bs;
BlockDriverState *mirror_top_bs;
Error *local_err = NULL;
-bool abort = job->ret < 0;
+bool abort = job_has_failed(job);
int ret = 0;
if (s->prepared) {
@@ -1161,9 +1161,7 @@ static void mirror_complete(Job *job, Error **errp)
s->should_complete = true;
/* If the job is paused, it will be re-entered when it is resumed */
-if (!job->paused) {
-job_enter(job);
-}
+job_enter_not_paused(job);
}
static void coroutine_fn mirror_pause(Job *job)
@@ -1182,7 +1180,7 @@ static bool mirror_drained_poll(BlockJob *job)
* from one of our own drain sections, to avoid a deadlock waiting for
* ourselves.
*/
-if (!s->common.job.paused && !job_is_cancelled(&job->job) && !s->in_drain)
{
+if (!job_not_paused_nor_cancelled(&s->common.job) && !s->in_drain) {
return true;
}
diff --git a/block/replication.c b/block/replication.c
index 55c8f894aa..0f487cc215 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -149,7 +149,9 @@ static void replication_close(BlockDriverState *bs)
if (s->stage == BLOCK_REPLICATION_FAILOVER) {
commit_job = &s->commit_job->job;
assert(commit_job->aio_context == qemu_get_current_aio_context());
+job_lock();
job_cancel_sync(commit_job, false);
+job_unlock();
}
if (s->mode == REPLICATION_MODE_SECONDARY) {
@@ -726,7 +728,9 @@ static void replication_stop(ReplicationState *rs, bool
failover, Error **errp)
* disk, secondary disk in backup_job_completed().
*/
if (s->backup_job) {
+job_lock();
job_cancel_sync(&s->backup_job->job, true);
+job_unlock();
}
if (!failover) {
diff --git a/blockdev.c b/blockdev.c
index 67b55eec11..c5a835d9ed 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -150,6 +150,8 @@ void blockdev_mark_auto_del(BlockBackend *blk)
return;
}
+job_lock();
+
for (job = block_job_next(NULL); job; job = block_job_next(job)) {
if (block_job_has_bdrv(job, blk_bs(blk))) {
AioContext *aio_context = job->job.aio_context;
@@ -161,6 +163,8 @@ void blockdev_mark_auto_del(BlockBackend *blk)
}
}
+job_unlock();
+
dinfo->auto_del = 1;
}
@@ -1844,7 +1848,9 @@ static void drive_backup_abort(BlkActionState *common)
aio_context = bdrv_get_aio_context(state->bs);
aio_context_acquire(aio_context);
+job_lock();
job_cancel_sync(&state->job->job, true);
+job_unlock();
aio_context_release(aio_context);
}
@@ -1945,7 +1951,9 @@ static void blockdev_backup_abort(BlkActionState *common)
ai
[RFC PATCH 15/15] jobs: remove all unnecessary AioContext locks
Now that we removed the aiocontext in find_* functions,
we need to remove it also in all other functions of the job API
that assumed the lock was held.
Therefore:
- when possible, remove completely the aiocontext lock/unlock pair
- if it is used by some other funcitons too, just leave the job API
outside the aiocontext lock by adding unlock/lock pairs.
There is only one JobDriver callback, ->free() that assumes that
the aiocontext lock is held (because it calls bdrv_unref), so for
now keep that under aiocontext lock.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/qemu/job-monitor.h | 6
block/replication.c | 2 ++
blockdev.c | 19 ---
job.c| 57
tests/unit/test-bdrv-drain.c | 4 +--
tests/unit/test-block-iothread.c | 2 +-
tests/unit/test-blockjob.c | 13 ++--
7 files changed, 14 insertions(+), 89 deletions(-)
diff --git a/include/qemu/job-monitor.h b/include/qemu/job-monitor.h
index f473bd298f..4ba7e503d1 100644
--- a/include/qemu/job-monitor.h
+++ b/include/qemu/job-monitor.h
@@ -207,8 +207,6 @@ void job_user_cancel(Job *job, bool force, Error **errp);
*
* Returns the return value from the job if the job actually completed
* during the call, or -ECANCELED if it was canceled.
- *
- * Callers must hold the AioContext lock of job->aio_context.
*/
int job_cancel_sync(Job *job, bool force);
@@ -233,8 +231,6 @@ void job_cancel_sync_all(void);
*
* Returns the return value from the job.
*
- * Callers must hold the AioContext lock of job->aio_context.
- *
* Called between job_lock and job_unlock.
*/
int job_complete_sync(Job *job, Error **errp);
@@ -266,8 +262,6 @@ void job_dismiss(Job **job, Error **errp);
* Returns 0 if the job is successfully completed, -ECANCELED if the job was
* cancelled before completing, and -errno in other error cases.
*
- * Callers must hold the AioContext lock of job->aio_context.
- *
* Called between job_lock and job_unlock, but it releases the lock temporarly.
*/
int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error
**errp);
diff --git a/block/replication.c b/block/replication.c
index 0f487cc215..6a60c1af1a 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -728,9 +728,11 @@ static void replication_stop(ReplicationState *rs, bool
failover, Error **errp)
* disk, secondary disk in backup_job_completed().
*/
if (s->backup_job) {
+aio_context_release(aio_context);
job_lock();
job_cancel_sync(&s->backup_job->job, true);
job_unlock();
+aio_context_acquire(aio_context);
}
if (!failover) {
diff --git a/blockdev.c b/blockdev.c
index 592ed4a0fc..dfc73ef1bf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -154,12 +154,7 @@ void blockdev_mark_auto_del(BlockBackend *blk)
for (job = block_job_next(NULL); job; job = block_job_next(job)) {
if (block_job_has_bdrv(job, blk_bs(blk))) {
-AioContext *aio_context = job->job.aio_context;
-aio_context_acquire(aio_context);
-
job_cancel(&job->job, false);
-
-aio_context_release(aio_context);
}
}
@@ -1843,16 +1838,9 @@ static void drive_backup_abort(BlkActionState *common)
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
if (state->job) {
-AioContext *aio_context;
-
-aio_context = bdrv_get_aio_context(state->bs);
-aio_context_acquire(aio_context);
-
job_lock();
job_cancel_sync(&state->job->job, true);
job_unlock();
-
-aio_context_release(aio_context);
}
}
@@ -1946,16 +1934,9 @@ static void blockdev_backup_abort(BlkActionState *common)
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
common);
if (state->job) {
-AioContext *aio_context;
-
-aio_context = bdrv_get_aio_context(state->bs);
-aio_context_acquire(aio_context);
-
job_lock();
job_cancel_sync(&state->job->job, true);
job_unlock();
-
-aio_context_release(aio_context);
}
}
diff --git a/job.c b/job.c
index 6b3e860fa7..124b05f769 100644
--- a/job.c
+++ b/job.c
@@ -168,7 +168,6 @@ static int job_txn_apply(Job *job, int fn(Job *))
* break AIO_WAIT_WHILE from within fn.
*/
job_ref(job);
-aio_context_release(job->aio_context);
QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
rc = fn(other_job);
@@ -177,11 +176,6 @@ static int job_txn_apply(Job *job, int fn(Job *))
}
}
-/*
- * Note that job->aio_context might have been changed by calling fn, so we
- * can't use a local variable to cache it.
- */
-aio_context_acquire(job->aio_context);
job_unref(job);
return rc;
}
@@ -510,7 +504,10 @@ void job_unref(Job *job)
if (job->dr
[RFC PATCH 13/15] jobs: use job locks and helpers also in the unit tests
Add missing job synchronization in the unit tests, with
both explicit locks and helpers.
Signed-off-by: Emanuele Giuseppe Esposito
---
tests/unit/test-bdrv-drain.c | 40 +++---
tests/unit/test-block-iothread.c | 4 +++
tests/unit/test-blockjob-txn.c | 10 ++
tests/unit/test-blockjob.c | 57 +---
4 files changed, 72 insertions(+), 39 deletions(-)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 2d3c17e566..535c39b5a8 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -941,61 +941,63 @@ static void test_blockjob_common_drain_node(enum
drain_type drain_type,
}
}
-g_assert_cmpint(job->job.pause_count, ==, 0);
-g_assert_false(job->job.paused);
+g_assert_cmpint(job_get_pause_count(&job->job), ==, 0);
+g_assert_false(job_get_paused(&job->job));
g_assert_true(tjob->running);
-g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+g_assert_true(job_get_busy(&job->job)); /* We're in qemu_co_sleep_ns() */
do_drain_begin_unlocked(drain_type, drain_bs);
if (drain_type == BDRV_DRAIN_ALL) {
/* bdrv_drain_all() drains both src and target */
-g_assert_cmpint(job->job.pause_count, ==, 2);
+g_assert_cmpint(job_get_pause_count(&job->job), ==, 2);
} else {
-g_assert_cmpint(job->job.pause_count, ==, 1);
+g_assert_cmpint(job_get_pause_count(&job->job), ==, 1);
}
-g_assert_true(job->job.paused);
-g_assert_false(job->job.busy); /* The job is paused */
+g_assert_true(job_get_paused(&job->job));
+g_assert_false(job_get_busy(&job->job)); /* The job is paused */
do_drain_end_unlocked(drain_type, drain_bs);
if (use_iothread) {
/* paused is reset in the I/O thread, wait for it */
-while (job->job.paused) {
+while (job_get_paused(&job->job)) {
aio_poll(qemu_get_aio_context(), false);
}
}
-g_assert_cmpint(job->job.pause_count, ==, 0);
-g_assert_false(job->job.paused);
-g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+g_assert_cmpint(job_get_pause_count(&job->job), ==, 0);
+g_assert_false(job_get_paused(&job->job));
+g_assert_true(job_get_busy(&job->job)); /* We're in qemu_co_sleep_ns() */
do_drain_begin_unlocked(drain_type, target);
if (drain_type == BDRV_DRAIN_ALL) {
/* bdrv_drain_all() drains both src and target */
-g_assert_cmpint(job->job.pause_count, ==, 2);
+g_assert_cmpint(job_get_pause_count(&job->job), ==, 2);
} else {
-g_assert_cmpint(job->job.pause_count, ==, 1);
+g_assert_cmpint(job_get_pause_count(&job->job), ==, 1);
}
-g_assert_true(job->job.paused);
-g_assert_false(job->job.busy); /* The job is paused */
+g_assert_true(job_get_paused(&job->job));
+g_assert_false(job_get_busy(&job->job)); /* The job is paused */
do_drain_end_unlocked(drain_type, target);
if (use_iothread) {
/* paused is reset in the I/O thread, wait for it */
-while (job->job.paused) {
+while (job_get_paused(&job->job)) {
aio_poll(qemu_get_aio_context(), false);
}
}
-g_assert_cmpint(job->job.pause_count, ==, 0);
-g_assert_false(job->job.paused);
-g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
+g_assert_cmpint(job_get_pause_count(&job->job), ==, 0);
+g_assert_false(job_get_paused(&job->job));
+g_assert_true(job_get_busy(&job->job)); /* We're in qemu_co_sleep_ns() */
aio_context_acquire(ctx);
+job_lock();
ret = job_complete_sync(&job->job, &error_abort);
+job_unlock();
g_assert_cmpint(ret, ==, (result == TEST_JOB_SUCCESS ? 0 : -EIO));
if (use_iothread) {
diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c
index aea660aeed..f39cb8b7ef 100644
--- a/tests/unit/test-block-iothread.c
+++ b/tests/unit/test-block-iothread.c
@@ -456,7 +456,9 @@ static void test_attach_blockjob(void)
}
aio_context_acquire(ctx);
+job_lock();
job_complete_sync(&tjob->common.job, &error_abort);
+job_unlock();
blk_set_aio_context(blk, qemu_get_aio_context(), &error_abort);
aio_context_release(ctx);
@@ -630,7 +632,9 @@ static void test_propagate_mirror(void)
BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
false, "filter_node", MIRROR_COPY_MODE_BACKGROUND,
&error_abort);
+job_lock();
job = job_get("job0");
+job_unlock();
filter = bdrv_find_node("filter_node");
/* Change the AioContext of src */
diff --git a/tests/unit/test-blockjob-txn.c b/tests/unit/test-blockjob-txn.c
index 8bd13b9949..1ae3a9d443 100644
--- a/tests/unit/test-blockjob-txn.c
+++ b/tests/unit/test-blockjob-txn.c
@@ -124,16 +124,20 @@ static void test_single_job(int expected)
job = test_block_job
[RFC PATCH 09/15] job.c: move inner aiocontext lock in callbacks
Instead of having the lock in job_tnx_apply, move it inside
in the callback. This will be helpful for next commits, when
we introduce job_lock/unlock pairs.
job_transition_to_pending() and job_needs_finalize() do not
need to be protected by the aiocontext lock.
No functional change intended.
Signed-off-by: Emanuele Giuseppe Esposito
---
job.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/job.c b/job.c
index 88d911f2d7..eb6d321960 100644
--- a/job.c
+++ b/job.c
@@ -153,7 +153,6 @@ static void job_txn_del_job(Job *job)
static int job_txn_apply(Job *job, int fn(Job *))
{
-AioContext *inner_ctx;
Job *other_job, *next;
JobTxn *txn = job->txn;
int rc = 0;
@@ -168,10 +167,7 @@ static int job_txn_apply(Job *job, int fn(Job *))
aio_context_release(job->aio_context);
QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
-inner_ctx = other_job->aio_context;
-aio_context_acquire(inner_ctx);
rc = fn(other_job);
-aio_context_release(inner_ctx);
if (rc) {
break;
}
@@ -836,7 +832,10 @@ static void job_clean(Job *job)
static int job_finalize_single(Job *job)
{
+AioContext *ctx = job->aio_context;
+
assert(job_is_completed(job));
+aio_context_acquire(ctx);
/* Ensure abort is called for late-transactional failures */
job_update_rc(job);
@@ -863,6 +862,7 @@ static int job_finalize_single(Job *job)
job_txn_del_job(job);
job_conclude(job);
+aio_context_release(ctx);
return 0;
}
@@ -968,11 +968,16 @@ static void job_completed_txn_abort(Job *job)
static int job_prepare(Job *job)
{
+AioContext *ctx = job->aio_context;
assert(qemu_in_main_thread());
+
+aio_context_acquire(ctx);
if (job->ret == 0 && job->driver->prepare) {
job->ret = job->driver->prepare(job);
job_update_rc(job);
}
+aio_context_release(ctx);
+
return job->ret;
}
--
2.27.0
[RFC PATCH 03/15] job-common.h: categorize fields in struct Job
Categorize the fields in struct Job to understand which need
to be protected by the job muutex and which not.
Also move job_type() and job_type_str() there, as they are
common helper functions.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/qemu/job-common.h | 62 +--
include/qemu/job.h| 6
2 files changed, 40 insertions(+), 28 deletions(-)
diff --git a/include/qemu/job-common.h b/include/qemu/job-common.h
index dcc24fba48..d2968a848e 100644
--- a/include/qemu/job-common.h
+++ b/include/qemu/job-common.h
@@ -40,27 +40,52 @@ typedef struct JobTxn JobTxn;
* Long-running operation.
*/
typedef struct Job {
+
+/* Fields set at initialization (job_create), and never modified */
+
/** The ID of the job. May be NULL for internal jobs. */
char *id;
-/** The type of this job. */
+/**
+ * The type of this job.
+ * All callbacks are called with job_mutex *not* held.
+ */
const JobDriver *driver;
-/** Reference count of the block job */
-int refcnt;
-
-/** Current state; See @JobStatus for details. */
-JobStatus status;
-
/** AioContext to run the job coroutine in */
AioContext *aio_context;
/**
* The coroutine that executes the job. If not NULL, it is reentered when
* busy is false and the job is cancelled.
+ * Initialized in job_start()
*/
Coroutine *co;
+/** True if this job should automatically finalize itself */
+bool auto_finalize;
+
+/** True if this job should automatically dismiss itself */
+bool auto_dismiss;
+
+/** The completion function that will be called when the job completes. */
+BlockCompletionFunc *cb;
+
+/** The opaque value that is passed to the completion function. */
+void *opaque;
+
+/* ProgressMeter API is thread-safe */
+ProgressMeter progress;
+
+
+/** Protected by job_mutex */
+
+/** Reference count of the block job */
+int refcnt;
+
+/** Current state; See @JobStatus for details. */
+JobStatus status;
+
/**
* Timer that is used by @job_sleep_ns. Accessed under job_mutex (in
* job.c).
@@ -76,7 +101,7 @@ typedef struct Job {
/**
* Set to false by the job while the coroutine has yielded and may be
* re-entered by job_enter(). There may still be I/O or event loop activity
- * pending. Accessed under block_job_mutex (in blockjob.c).
+ * pending. Accessed under job_mutex.
*
* When the job is deferred to the main loop, busy is true as long as the
* bottom half is still pending.
@@ -112,14 +137,6 @@ typedef struct Job {
/** Set to true when the job has deferred work to the main loop. */
bool deferred_to_main_loop;
-/** True if this job should automatically finalize itself */
-bool auto_finalize;
-
-/** True if this job should automatically dismiss itself */
-bool auto_dismiss;
-
-ProgressMeter progress;
-
/**
* Return code from @run and/or @prepare callback(s).
* Not final until the job has reached the CONCLUDED status.
@@ -134,12 +151,6 @@ typedef struct Job {
*/
Error *err;
-/** The completion function that will be called when the job completes. */
-BlockCompletionFunc *cb;
-
-/** The opaque value that is passed to the completion function. */
-void *opaque;
-
/** Notifiers called when a cancelled job is finalised */
NotifierList on_finalize_cancelled;
@@ -167,6 +178,7 @@ typedef struct Job {
/**
* Callbacks and other information about a Job driver.
+ * All callbacks are invoked with job_mutex *not* held.
*/
struct JobDriver {
@@ -315,4 +327,10 @@ void job_lock(void);
*/
void job_unlock(void);
+/** Returns the JobType of a given Job. */
+JobType job_type(const Job *job);
+
+/** Returns the enum string for the JobType of a given Job. */
+const char *job_type_str(const Job *job);
+
#endif
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 3cfd79088c..e4e0c26672 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -174,12 +174,6 @@ void job_yield(Job *job);
void coroutine_fn job_sleep_ns(Job *job, int64_t ns);
-/** Returns the JobType of a given Job. */
-JobType job_type(const Job *job);
-
-/** Returns the enum string for the JobType of a given Job. */
-const char *job_type_str(const Job *job);
-
/** Returns true if the job should not be visible to the management layer. */
bool job_is_internal(Job *job);
--
2.27.0
[RFC PATCH 07/15] job-driver.h: add helper functions
These functions will be useful when job_lock is globally applied,
as they will allow drivers to access the job struct fields
without worrying about the job lock.
Now that we are done with the job API header split, update also
the comments in blockjob.c (and move them in job.c).
Signed-off-by: Emanuele Giuseppe Esposito
---
include/qemu/job-driver.h | 21 +++
blockjob.c| 20 ---
job.c | 116 +-
3 files changed, 135 insertions(+), 22 deletions(-)
diff --git a/include/qemu/job-driver.h b/include/qemu/job-driver.h
index 1efd196da8..19ae5ce8f0 100644
--- a/include/qemu/job-driver.h
+++ b/include/qemu/job-driver.h
@@ -149,4 +149,25 @@ void job_early_fail(Job *job);
/** Moves the @job from RUNNING to READY */
void job_transition_to_ready(Job *job);
+/** Enters the @job if it is not paused */
+void job_enter_not_paused(Job *job);
+
+/** returns @job->ret */
+bool job_has_failed(Job *job);
+
+/** Returns the @job->status */
+JobStatus job_get_status(Job *job);
+
+/** Returns the @job->pause_count */
+int job_get_pause_count(Job *job);
+
+/** Returns @job->paused */
+bool job_get_paused(Job *job);
+
+/** Returns @job->busy */
+bool job_get_busy(Job *job);
+
+/** Return true if @job not paused and not cancelled */
+bool job_not_paused_nor_cancelled(Job *job);
+
#endif /* JOB_DRIVER_H */
diff --git a/blockjob.c b/blockjob.c
index 4982f6a2b5..53c1e9c406 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -36,21 +36,6 @@
#include "qemu/main-loop.h"
#include "qemu/timer.h"
-/*
- * The block job API is composed of two categories of functions.
- *
- * The first includes functions used by the monitor. The monitor is
- * peculiar in that it accesses the block job list with block_job_get, and
- * therefore needs consistency across block_job_get and the actual operation
- * (e.g. block_job_set_speed). The consistency is achieved with
- * aio_context_acquire/release. These functions are declared in blockjob.h.
- *
- * The second includes functions used by the block job drivers and sometimes
- * by the core block layer. These do not care about locking, because the
- * whole coroutine runs under the AioContext lock, and are declared in
- * blockjob_int.h.
- */
-
static bool is_block_job(Job *job)
{
return job_type(job) == JOB_TYPE_BACKUP ||
@@ -433,11 +418,6 @@ static void block_job_event_ready(Notifier *n, void
*opaque)
}
-/*
- * API for block job drivers and the block layer. These functions are
- * declared in blockjob_int.h.
- */
-
void *block_job_create(const char *job_id, const BlockJobDriver *driver,
JobTxn *txn, BlockDriverState *bs, uint64_t perm,
uint64_t shared_perm, int64_t speed, int flags,
diff --git a/job.c b/job.c
index b66d59b746..db7ad79745 100644
--- a/job.c
+++ b/job.c
@@ -32,6 +32,23 @@
#include "trace/trace-root.h"
#include "qapi/qapi-events-job.h"
+/*
+ * The job API is composed of two categories of functions.
+ *
+ * The first includes functions used by the monitor. The monitor is
+ * peculiar in that it accesses the block job list with job_get, and
+ * therefore needs consistency across job_get and the actual operation
+ * (e.g. job_user_cancel). To achieve this consistency, the caller
+ * calls job_lock/job_unlock itself around the whole operation.
+ * These functions are declared in job-monitor.h.
+ *
+ *
+ * The second includes functions used by the block job drivers and sometimes
+ * by the core block layer. These delegate the locking to the callee instead,
+ * and are declared in job-driver.h.
+ */
+
+
/* job_mutex protects the jobs list, but also makes the job API thread-safe. */
static QemuMutex job_mutex;
@@ -213,18 +230,94 @@ const char *job_type_str(const Job *job)
return JobType_str(job_type(job));
}
-bool job_is_cancelled(Job *job)
+JobStatus job_get_status(Job *job)
+{
+JobStatus status;
+job_lock();
+status = job->status;
+job_unlock();
+return status;
+}
+
+int job_get_pause_count(Job *job)
+{
+int ret;
+job_lock();
+ret = job->pause_count;
+job_unlock();
+return ret;
+}
+
+bool job_get_paused(Job *job)
+{
+bool ret;
+job_lock();
+ret = job->paused;
+job_unlock();
+return ret;
+}
+
+bool job_get_busy(Job *job)
+{
+bool ret;
+job_lock();
+ret = job->busy;
+job_unlock();
+return ret;
+}
+
+bool job_has_failed(Job *job)
+{
+bool ret;
+job_lock();
+ret = job->ret < 0;
+job_unlock();
+return ret;
+}
+
+/* Called with job_mutex held. */
+static bool job_is_cancelled_locked(Job *job)
{
/* force_cancel may be true only if cancelled is true, too */
assert(job->cancelled || !job->force_cancel);
return job->force_cancel;
}
-bool job_cancel_requested(Job *job)
+/* Called with job_mutex *not* held. */
+bool job_is_cancelled(Job *job)
+{
+bool ret;
+job_lock();
+ret = job_is_cancelled_locked(job);
+job_u
[RFC PATCH 06/15] jobs: add job-driver.h
job-driver.h contains all functions of job.h that are used by the drivers (JobDriver, BlockJobDriver). These functions are unaware of the job_mutex, so they all will take and release the lock internally. No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito --- include/qemu/job-driver.h | 152 + include/qemu/job-monitor.h | 4 +- include/qemu/job.h | 114 +--- 3 files changed, 156 insertions(+), 114 deletions(-) create mode 100644 include/qemu/job-driver.h diff --git a/include/qemu/job-driver.h b/include/qemu/job-driver.h new file mode 100644 index 00..1efd196da8 --- /dev/null +++ b/include/qemu/job-driver.h @@ -0,0 +1,152 @@ +/* + * Declarations for background jobs + * + * Copyright (c) 2011 IBM Corp. + * Copyright (c) 2012, 2018 Red Hat, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#ifndef JOB_DRIVER_H +#define JOB_DRIVER_H + +#include "job-common.h" + +/* + * Job driver API. + * + * These functions use are used by job drivers like mirror, + * stream, commit etc. The driver is not aware of the job_mutex + * presence, so these functions use it internally to protect + * job fields (see job-common.h). + * + * Therefore, each function in this API that requires protection + * must have the comment + * "Called with job_mutext *not* held" + * in job.c + */ + +/** + * Create a new long-running job and return it. + * + * @job_id: The id of the newly-created job, or %NULL for internal jobs + * @driver: The class object for the newly-created job. + * @txn: The transaction this job belongs to, if any. %NULL otherwise. + * @ctx: The AioContext to run the job coroutine in. + * @flags: Creation flags for the job. See @JobCreateFlags. + * @cb: Completion function for the job. + * @opaque: Opaque pointer value passed to @cb. + * @errp: Error object. + */ +void *job_create(const char *job_id, const JobDriver *driver, JobTxn *txn, + AioContext *ctx, int flags, BlockCompletionFunc *cb, + void *opaque, Error **errp); + +/** + * @job: The job that has made progress + * @done: How much progress the job made since the last call + * + * Updates the progress counter of the job. + */ +void job_progress_update(Job *job, uint64_t done); + +/** + * @job: The job whose expected progress end value is set + * @remaining: Missing progress (on top of the current progress counter value) + * until the new expected end value is reached + * + * Sets the expected end value of the progress counter of a job so that a + * completion percentage can be calculated when the progress is updated. + */ +void job_progress_set_remaining(Job *job, uint64_t remaining); + +/** + * @job: The job whose expected progress end value is updated + * @delta: Value which is to be added to the current expected end + * value + * + * Increases the expected end value of the progress counter of a job. + * This is useful for parenthesis operations: If a job has to + * conditionally perform a high-priority operation as part of its + * progress, it calls this function with the expected operation's + * length before, and job_progress_update() afterwards. + * (So the operation acts as a parenthesis in regards to the main job + * operation running in background.) + */ +void job_progress_increase_remaining(Job *job, uint64_t delta); + +/** + * @job: A job that has not yet been started. + * + * Begins execution of a job. + * Takes ownership of one reference to the job object. + */ +void job_start(Job *job); + +/** + * @job: The job to enter. + * + * Continue the specified job by entering the coroutine. + */ +void job_enter(Job *job); + +/** + * @job: The job that is ready to pause. + * + * Pause now if job_pause() has been called. Jobs that perform lots of I/O + * must call this between requests so that the job can be paused. + */ +void coroutine_fn job_pause_point(Job *job); + +/**
[RFC PATCH 02/15] job.c: make job_lock/unlock public
job mutex will be used to protect the job struct elements and list,
replacing AioContext locks.
Right now use a shared lock for all jobs, in order to keep things
simple. Once the AioContext lock is gone, we can introduce per-job
locks.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/qemu/job-common.h | 18 ++
job.c | 12 +---
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/include/qemu/job-common.h b/include/qemu/job-common.h
index c115028e33..dcc24fba48 100644
--- a/include/qemu/job-common.h
+++ b/include/qemu/job-common.h
@@ -297,4 +297,22 @@ typedef enum JobCreateFlags {
JOB_MANUAL_DISMISS = 0x04,
} JobCreateFlags;
+/**
+ * job_lock:
+ *
+ * Take the mutex protecting the list of jobs and their status.
+ * Most functions called by the monitor need to call job_lock
+ * and job_unlock manually. On the other hand, function called
+ * by the block jobs themselves and by the block layer will take the
+ * lock for you.
+ */
+void job_lock(void);
+
+/**
+ * job_unlock:
+ *
+ * Release the mutex protecting the list of jobs and their status.
+ */
+void job_unlock(void);
+
#endif
diff --git a/job.c b/job.c
index 94b142684f..e003f136f0 100644
--- a/job.c
+++ b/job.c
@@ -32,6 +32,9 @@
#include "trace/trace-root.h"
#include "qapi/qapi-events-job.h"
+/* job_mutex protects the jobs list, but also makes the job API thread-safe. */
+static QemuMutex job_mutex;
+
static QLIST_HEAD(, Job) jobs = QLIST_HEAD_INITIALIZER(jobs);
/* Job State Transition Table */
@@ -74,17 +77,12 @@ struct JobTxn {
int refcnt;
};
-/* Right now, this mutex is only needed to synchronize accesses to job->busy
- * and job->sleep_timer, such as concurrent calls to job_do_yield and
- * job_enter. */
-static QemuMutex job_mutex;
-
-static void job_lock(void)
+void job_lock(void)
{
qemu_mutex_lock(&job_mutex);
}
-static void job_unlock(void)
+void job_unlock(void)
{
qemu_mutex_unlock(&job_mutex);
}
--
2.27.0
[RFC PATCH 04/15] jobs: add job-monitor.h
job-monitor.h contains all functions of job.h that are used by the monitor and essentially all functions that do not define a JobDriver/Blockdriver. Right now just move the headers, proper categorization and API definition will come in the next commit. No functional change intended. Signed-off-by: Emanuele Giuseppe Esposito --- include/qemu/job-monitor.h | 220 + include/qemu/job.h | 180 +- 2 files changed, 221 insertions(+), 179 deletions(-) create mode 100644 include/qemu/job-monitor.h diff --git a/include/qemu/job-monitor.h b/include/qemu/job-monitor.h new file mode 100644 index 00..b5b41a7548 --- /dev/null +++ b/include/qemu/job-monitor.h @@ -0,0 +1,220 @@ +/* + * Declarations for background jobs + * + * Copyright (c) 2011 IBM Corp. + * Copyright (c) 2012, 2018 Red Hat, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#ifndef JOB_MONITOR_H +#define JOB_MONITOR_H + +#include "job-common.h" + +/** + * Allocate and return a new job transaction. Jobs can be added to the + * transaction using job_txn_add_job(). + * + * The transaction is automatically freed when the last job completes or is + * cancelled. + * + * All jobs in the transaction either complete successfully or fail/cancel as a + * group. Jobs wait for each other before completing. Cancelling one job + * cancels all jobs in the transaction. + */ +JobTxn *job_txn_new(void); + +/** + * Release a reference that was previously acquired with job_txn_add_job or + * job_txn_new. If it's the last reference to the object, it will be freed. + */ +void job_txn_unref(JobTxn *txn); + +/** + * @txn: The transaction (may be NULL) + * @job: Job to add to the transaction + * + * Add @job to the transaction. The @job must not already be in a transaction. + * The caller must call either job_txn_unref() or job_completed() to release + * the reference that is automatically grabbed here. + * + * If @txn is NULL, the function does nothing. + */ +void job_txn_add_job(JobTxn *txn, Job *job); + +/** + * Add a reference to Job refcnt, it will be decreased with job_unref, and then + * be freed if it comes to be the last reference. + */ +void job_ref(Job *job); + +/** + * Release a reference that was previously acquired with job_ref() or + * job_create(). If it's the last reference to the object, it will be freed. + */ +void job_unref(Job *job); + +/** + * Conditionally enter the job coroutine if the job is ready to run, not + * already busy and fn() returns true. fn() is called while under the job_lock + * critical section. + */ +void job_enter_cond(Job *job, bool(*fn)(Job *job)); + +/** + * Returns true if the job should not be visible to the management layer. + */ +bool job_is_internal(Job *job); + +/** + * Returns whether the job is in a completed state. + */ +bool job_is_completed(Job *job); + +/** + * Request @job to pause at the next pause point. Must be paired with + * job_resume(). If the job is supposed to be resumed by user action, call + * job_user_pause() instead. + */ +void job_pause(Job *job); + +/** + * Resumes a @job paused with job_pause. + */ +void job_resume(Job *job); + +/** + * Asynchronously pause the specified @job. + * Do not allow a resume until a matching call to job_user_resume. + */ +void job_user_pause(Job *job, Error **errp); + +/** + * Returns true if the job is user-paused. + */ +bool job_user_paused(Job *job); + +/** + * Resume the specified @job. + * Must be paired with a preceding job_user_pause. + */ +void job_user_resume(Job *job, Error **errp); + +/** + * Get the next element from the list of block jobs after @job, or the + * first one if @job is %NULL. + * + * Returns the requested job, or %NULL if there are no more jobs left. + */ +Job *job_next(Job *job); + +/** + * Get the job identified by @id (which must not be %NULL). + * + * Returns the requested job, or %NULL if it doesn't exist. + */ +Job *job_get(c
[RFC PATCH 00/15] job: replace AioContext lock with job_mutex
In this series, we want to remove the AioContext lock and instead
use the already existent job_mutex to protect the job structures
and list. This is part of the work to get rid of AioContext lock
usage in favour of smaller granularity locks.
In patches 1-3-5-6-7, we split the job API in two headers:
job-driver.h and job-monitor.h.
As explained in job.c, job-monitor are the functions mainly used
by the monitor, and require consistency between the search of
a specific job (job_get) and the actual operation/action on it
(e.g. job_user_cancel). Therefore job-monitor API assume that
the job mutex lock is always held by the caller.
job-driver, on the other side, is the collection of functions
that are used by the job drivers or core block layer. These
functions are not aware of the job mutex, and delegate the
locking to the callee instead.
We also have job-common.h contains the job struct definition
and common functions that are not part of monitor or driver APIs.
job.h is left for legacy and to avoid changing all files that
include it.
After we split the job API, we have a couple of patches that try
to prepare and simplify the aiocontext lock replacement with
job lock, namely patch 10 introduces AIO_WAIT_WHILE_UNLOCKED
(same as the original AIO_WAIT_WHILE but does not release and
re-acquires the aiocontext lock if provided).
Patch 12 uses job_lock/unlock to protect the job fields and
shared structures. Note that this patch just adds the job locks,
and in order to simplify the rewiew, does remove any aiocontext lock, creating
deadlocks. Patch 13 takes care of the unit tests.
The reason why it does not handle possible deadlocks is because
doing so would just add additional job_lock/unlock that would
still be deleted in next patches together with the aiocontext lock.
RFC: not sure if the current patch organization is correct.
Bisecting in patches in between this serie would cause tests
to deadlock.
Example: currently patch 12 has this code
static void job_exit(void *opaque)
{
Job *job = (Job *)opaque;
+ job_lock();
job_ref(); // assumes the lock held
aio_context_acquire();
and then on patch 15:
static void job_exit(void *opaque)
{
Job *job = (Job *)opaque;
job_lock();
- job_ref(); // assumes the lock held
- aio_context_acquire();
This is not ideal in patch 12, because taking the aiocontext
lock after the job lock is already held can cause deadlocks
(in the worst case we might want the opposite order),
but in order to keep every patch clean we would need to
do 2 patches:
static void job_exit(void *opaque)
{
Job *job = (Job *)opaque;
+ job_lock();
job_ref(); // assumes the lock held
+ job_unlock();
aio_context_acquire();
+ job_lock();
and once the aiocontext is removed:
static void job_exit(void *opaque)
{
Job *job = (Job *)opaque;
job_lock();
- job_ref(); // assumes the lock held
- job_unlock();
- aio_context_acquire();
- job_lock();
which seems unnecessary.
Patch 14 instead starts replacing some aiocontext with job_locks,
and patch 15 takes care of completely eradicating them from the
job API (with the exception of driver->free()). Again the two
patches are kept separated to simplify the life of the reviewer.
Tested this series by running unit tests, qemu-iotests and qtests
(x86_64).
This serie is based on my previous series "block layer: split
block APIs in global state and I/O".
Based-on: <20211025101735.2060852-1-eespo...@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito
Emanuele Giuseppe Esposito (15):
jobs: add job-common.h
job.c: make job_lock/unlock public
job-common.h: categorize fields in struct Job
jobs: add job-monitor.h
job-monitor.h: define the job monitor API
jobs: add job-driver.h
job-driver.h: add helper functions
job.c: minor adjustments in preparation to job-driver
job.c: move inner aiocontext lock in callbacks
aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
jobs: remove aiocontext locks since the functions are under BQL
jobs: protect jobs with job_lock/unlock
jobs: use job locks and helpers also in the unit tests
jobs: add missing job locks to replace aiocontext lock
jobs: remove all unnecessary AioContext locks
include/block/aio-wait.h | 15 +-
include/qemu/job-common.h| 336 ++
include/qemu/job-driver.h| 173 ++
include/qemu/job-monitor.h | 275 +++
include/qemu/job.h | 569 +--
block.c | 6 +
block/mirror.c | 8 +-
block/replication.c | 6 +
blockdev.c | 88 ++---
blockjob.c | 62 ++--
job-qmp.c| 54 ++-
job.c| 413 --
monitor/qmp-cmds.c | 2 +
qemu-img.c | 8 +-
tests/unit/test-bdrv-drain.c | 44 +--
tests/unit/test-block-iothread.c | 6 +-
tests/u
[RFC PATCH 01/15] jobs: add job-common.h
job-common.h contains all struct and common function that currently
are in job.h and will be shared by job-monitor and job-driver in
the next commits.
No functional change intended.
Signed-off-by: Emanuele Giuseppe Esposito
---
include/qemu/job-common.h | 300 ++
include/qemu/job.h| 271 +-
2 files changed, 301 insertions(+), 270 deletions(-)
create mode 100644 include/qemu/job-common.h
diff --git a/include/qemu/job-common.h b/include/qemu/job-common.h
new file mode 100644
index 00..c115028e33
--- /dev/null
+++ b/include/qemu/job-common.h
@@ -0,0 +1,300 @@
+/*
+ * Declarations for background jobs
+ *
+ * Copyright (c) 2011 IBM Corp.
+ * Copyright (c) 2012, 2018 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef JOB_COMMON_H
+#define JOB_COMMON_H
+
+#include "qapi/qapi-types-job.h"
+#include "qemu/queue.h"
+#include "qemu/progress_meter.h"
+#include "qemu/coroutine.h"
+#include "block/aio.h"
+
+typedef struct JobDriver JobDriver;
+typedef struct JobTxn JobTxn;
+
+
+/**
+ * Long-running operation.
+ */
+typedef struct Job {
+/** The ID of the job. May be NULL for internal jobs. */
+char *id;
+
+/** The type of this job. */
+const JobDriver *driver;
+
+/** Reference count of the block job */
+int refcnt;
+
+/** Current state; See @JobStatus for details. */
+JobStatus status;
+
+/** AioContext to run the job coroutine in */
+AioContext *aio_context;
+
+/**
+ * The coroutine that executes the job. If not NULL, it is reentered when
+ * busy is false and the job is cancelled.
+ */
+Coroutine *co;
+
+/**
+ * Timer that is used by @job_sleep_ns. Accessed under job_mutex (in
+ * job.c).
+ */
+QEMUTimer sleep_timer;
+
+/**
+ * Counter for pause request. If non-zero, the block job is either paused,
+ * or if busy == true will pause itself as soon as possible.
+ */
+int pause_count;
+
+/**
+ * Set to false by the job while the coroutine has yielded and may be
+ * re-entered by job_enter(). There may still be I/O or event loop activity
+ * pending. Accessed under block_job_mutex (in blockjob.c).
+ *
+ * When the job is deferred to the main loop, busy is true as long as the
+ * bottom half is still pending.
+ */
+bool busy;
+
+/**
+ * Set to true by the job while it is in a quiescent state, where
+ * no I/O or event loop activity is pending.
+ */
+bool paused;
+
+/**
+ * Set to true if the job is paused by user. Can be unpaused with the
+ * block-job-resume QMP command.
+ */
+bool user_paused;
+
+/**
+ * Set to true if the job should cancel itself. The flag must
+ * always be tested just before toggling the busy flag from false
+ * to true. After a job has been cancelled, it should only yield
+ * if #aio_poll will ("sooner or later") reenter the coroutine.
+ */
+bool cancelled;
+
+/**
+ * Set to true if the job should abort immediately without waiting
+ * for data to be in sync.
+ */
+bool force_cancel;
+
+/** Set to true when the job has deferred work to the main loop. */
+bool deferred_to_main_loop;
+
+/** True if this job should automatically finalize itself */
+bool auto_finalize;
+
+/** True if this job should automatically dismiss itself */
+bool auto_dismiss;
+
+ProgressMeter progress;
+
+/**
+ * Return code from @run and/or @prepare callback(s).
+ * Not final until the job has reached the CONCLUDED status.
+ * 0 on success, -errno on failure.
+ */
+int ret;
+
+/**
+ * Error object for a failed job.
+ * If job->ret is nonzero and an error object was not set, it will be set
+ * to strerror(-job->ret) during job_completed.
+ */
+Error *err;
+
+/** The completion function t
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
On 10/29/21 17:34, Markus Armbruster wrote:
> Eric Blake writes:
>
>> On Thu, Oct 28, 2021 at 12:25:16PM +0200, Markus Armbruster wrote:
>>> The generated visitor functions call visit_deprecated_accept() and
>>> visit_deprecated() when visiting a struct member with special feature
>>> flag 'deprecated'. This makes the feature flag visible to the actual
>>> visitors. I want to make feature flag 'unstable' visible there as
>>> well, so I can add policy for it.
>>>
>>> To let me make it visible, replace these functions by
>>> visit_policy_reject() and visit_policy_skip(), which take the member's
>>> special features as an argument. Note that the new functions have the
>>> opposite sense, i.e. the return value flips.
>>>
>>> Signed-off-by: Markus Armbruster
>>> ---
>>
>>> +++ b/qapi/qapi-forward-visitor.c
>>> @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const
>>> char *name, bool *present)
>>> visit_optional(ffv->target, name, present);
>>> }
>>>
>>> -static bool forward_field_deprecated_accept(Visitor *v, const char *name,
>>> -Error **errp)
>>> +static bool forward_field_policy_reject(Visitor *v, const char *name,
>>> +unsigned special_features,
>>> +Error **errp)
>>> {
>>> ForwardFieldVisitor *ffv = to_ffv(v);
>>>
>>> if (!forward_field_translate_name(ffv, &name, errp)) {
>>> return false;
>>
>> Should this return value be flipped?
>>
>>> }
>>> -return visit_deprecated_accept(ffv->target, name, errp);
>>> +return visit_policy_reject(ffv->target, name, special_features, errp);
>>> }
>>>
>>> -static bool forward_field_deprecated(Visitor *v, const char *name)
>>> +static bool forward_field_policy_skip(Visitor *v, const char *name,
>>> + unsigned special_features)
>>> {
>>> ForwardFieldVisitor *ffv = to_ffv(v);
>>>
>>> if (!forward_field_translate_name(ffv, &name, NULL)) {
>>> return false;
>>
>> and here too?
>
> Good catch!
Ouch. I admit this patch logic is hard to review, but couldn't come
with a nice way to split it further.
> These functions are called indirectly like
>
> if (visit_policy_reject(v, "values", 1u << QAPI_DEPRECATED, errp)) {
> return false;
> }
> if (!visit_policy_skip(v, "values", 1u << QAPI_DEPRECATED)) {
> if (!visit_type_strList(v, "values", &obj->values, errp)) {
> return false;
> }
> }
>
> visit_policy_reject() must return true when it sets an error, or else we
> call visit_policy_skip() with @errp pointing to non-null.
>
> Same argument for visit_policy_skip().
>
>>> }
>>> -return visit_deprecated(ffv->target, name);
>>> +return visit_policy_skip(ffv->target, name, special_features);
>>> }
>>>
>>
>> Otherwise, the rest of the logic changes for flipped sense look right.
>
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
On 10/29/21 16:01, Markus Armbruster wrote: > Philippe Mathieu-Daudé writes: > >> On 10/28/21 12:25, Markus Armbruster wrote: >>> The generated visitor functions call visit_deprecated_accept() and >>> visit_deprecated() when visiting a struct member with special feature >>> flag 'deprecated'. This makes the feature flag visible to the actual >>> visitors. I want to make feature flag 'unstable' visible there as >>> well, so I can add policy for it. >>> >>> To let me make it visible, replace these functions by >>> visit_policy_reject() and visit_policy_skip(), which take the member's >>> special features as an argument. Note that the new functions have the >>> opposite sense, i.e. the return value flips. >>> >>> Signed-off-by: Markus Armbruster >>> --- >>> include/qapi/visitor-impl.h | 6 -- >>> include/qapi/visitor.h| 17 + >>> qapi/qapi-forward-visitor.c | 16 +--- >>> qapi/qapi-visit-core.c| 22 -- >>> qapi/qobject-input-visitor.c | 15 ++- >>> qapi/qobject-output-visitor.c | 9 ++--- >>> qapi/trace-events | 4 ++-- >>> scripts/qapi/visit.py | 14 +++--- >>> 8 files changed, 63 insertions(+), 40 deletions(-) >>> case COMPAT_POLICY_INPUT_CRASH: >> >> Clearer as: >> >>abort(); >>default: >>g_assert_not_reached(); > > Maybe, but making it so has nothing to do with this patch. It could > perhaps be done in PATCH 8, or in a followup patch. > >> Otherwise, >> Reviewed-by: Philippe Mathieu-Daudé > > Okay to tack your R-by to the unmodified patch? Sure.
Re: [PATCH v2 6/9] qapi: Generalize command policy checking
On 10/29/21 17:28, Eric Blake wrote:
> On Thu, Oct 28, 2021 at 12:25:17PM +0200, Markus Armbruster wrote:
>> The code to check command policy can see special feature flag
>> 'deprecated' as command flag QCO_DEPRECATED. I want to make feature
>> flag 'unstable' visible there as well, so I can add policy for it.
>>
>> To let me make it visible, add member @special_features (a bitset of
>> QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it
>> through qmp_register_command(). Then replace "QCO_DEPRECATED in
>> @flags" by QAPI_DEPRECATED in @special_features", and drop
>> QCO_DEPRECATED.
>>
>> Signed-off-by: Markus Armbruster
>> Reviewed-by: Philippe Mathieu-Daudé
>> Acked-by: John Snow
>> ---
>
>> +++ b/qapi/qmp-dispatch.c
>> @@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject
>> *request,
>>"The command %s has not been found", command);
>> goto out;
>> }
>> -if (cmd->options & QCO_DEPRECATED) {
>> +if (cmd->special_features & 1u << QAPI_DEPRECATED) {
>
> I admit having to check the C operator precedence table when reading
This doesn't seem a good use of (y)our time. Using a pair of parenthesis
is simpler.
I expect in a not far future that compilers emit a warning for this.
> this (<< is higher than &); if writing it myself, I would probably
> have used explicit () to avoid reviewer confusion, but what you have
> is correct. (After grepping for ' & 1.*<<' and ' & (1.*<<', it looks
> like authors using explicit precedence happens more often, but that
> there are other instances in the code base relying on implicit
> precedence.)
$ git grep -E ' & [0-9a-zA-Z_]+ <<'
hw/dma/pl330.c:997:if (~ch->parent->inten & ch->parent->ev_status &
1 << ev_id) {
hw/dma/xlnx-zynq-devcfg.c:198:if (s->regs[R_LOCK] & 1 << i) {
hw/intc/grlib_irqmp.c:144:if (s->broadcast & 1 << irq) {
hw/net/fsl_etsec/rings.c:491:if (etsec->regs[RSTAT].value & 1 << (23
- ring_nbr)) {
hw/net/virtio-net.c:748:(n->host_features & 1ULL <<
VIRTIO_NET_F_MTU)) {
hw/pci-host/mv64361.c:812:if ((ch & 0xff << i) && !(val
& 0xff << i)) {
hw/pci-host/mv64361.c:858:if (s->gpp_int_level && !(val & 0xff
<< b)) {
hw/ssi/xilinx_spi.c:123:qemu_set_irq(s->cs_lines[i],
!(~s->regs[R_SPISSR] & 1 << i));
hw/ssi/xilinx_spips.c:441:r[idx[!d]] |= x[idx[d]] & 1 <<
bit[d] ? 1 << bit[!d] : 0;
target/s390x/cpu_features.c:56:if (init[i] & 1ULL << j) {
tests/qtest/bios-tables-test.c:209:if (!(val & 1UL << 20 /*
HW_REDUCED_ACPI */)) {
Not that many.
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
Eric Blake writes:
> On Thu, Oct 28, 2021 at 12:25:16PM +0200, Markus Armbruster wrote:
>> The generated visitor functions call visit_deprecated_accept() and
>> visit_deprecated() when visiting a struct member with special feature
>> flag 'deprecated'. This makes the feature flag visible to the actual
>> visitors. I want to make feature flag 'unstable' visible there as
>> well, so I can add policy for it.
>>
>> To let me make it visible, replace these functions by
>> visit_policy_reject() and visit_policy_skip(), which take the member's
>> special features as an argument. Note that the new functions have the
>> opposite sense, i.e. the return value flips.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>
>> +++ b/qapi/qapi-forward-visitor.c
>> @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const
>> char *name, bool *present)
>> visit_optional(ffv->target, name, present);
>> }
>>
>> -static bool forward_field_deprecated_accept(Visitor *v, const char *name,
>> -Error **errp)
>> +static bool forward_field_policy_reject(Visitor *v, const char *name,
>> +unsigned special_features,
>> +Error **errp)
>> {
>> ForwardFieldVisitor *ffv = to_ffv(v);
>>
>> if (!forward_field_translate_name(ffv, &name, errp)) {
>> return false;
>
> Should this return value be flipped?
>
>> }
>> -return visit_deprecated_accept(ffv->target, name, errp);
>> +return visit_policy_reject(ffv->target, name, special_features, errp);
>> }
>>
>> -static bool forward_field_deprecated(Visitor *v, const char *name)
>> +static bool forward_field_policy_skip(Visitor *v, const char *name,
>> + unsigned special_features)
>> {
>> ForwardFieldVisitor *ffv = to_ffv(v);
>>
>> if (!forward_field_translate_name(ffv, &name, NULL)) {
>> return false;
>
> and here too?
Good catch!
These functions are called indirectly like
if (visit_policy_reject(v, "values", 1u << QAPI_DEPRECATED, errp)) {
return false;
}
if (!visit_policy_skip(v, "values", 1u << QAPI_DEPRECATED)) {
if (!visit_type_strList(v, "values", &obj->values, errp)) {
return false;
}
}
visit_policy_reject() must return true when it sets an error, or else we
call visit_policy_skip() with @errp pointing to non-null.
Same argument for visit_policy_skip().
>> }
>> -return visit_deprecated(ffv->target, name);
>> +return visit_policy_skip(ffv->target, name, special_features);
>> }
>>
>
> Otherwise, the rest of the logic changes for flipped sense look right.
Re: [PATCH v2 6/9] qapi: Generalize command policy checking
On Thu, Oct 28, 2021 at 12:25:17PM +0200, Markus Armbruster wrote:
> The code to check command policy can see special feature flag
> 'deprecated' as command flag QCO_DEPRECATED. I want to make feature
> flag 'unstable' visible there as well, so I can add policy for it.
>
> To let me make it visible, add member @special_features (a bitset of
> QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it
> through qmp_register_command(). Then replace "QCO_DEPRECATED in
> @flags" by QAPI_DEPRECATED in @special_features", and drop
> QCO_DEPRECATED.
>
> Signed-off-by: Markus Armbruster
> Reviewed-by: Philippe Mathieu-Daudé
> Acked-by: John Snow
> ---
> +++ b/qapi/qmp-dispatch.c
> @@ -176,7 +176,7 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject
> *request,
>"The command %s has not been found", command);
> goto out;
> }
> -if (cmd->options & QCO_DEPRECATED) {
> +if (cmd->special_features & 1u << QAPI_DEPRECATED) {
I admit having to check the C operator precedence table when reading
this (<< is higher than &); if writing it myself, I would probably
have used explicit () to avoid reviewer confusion, but what you have
is correct. (After grepping for ' & 1.*<<' and ' & (1.*<<', it looks
like authors using explicit precedence happens more often, but that
there are other instances in the code base relying on implicit
precedence.)
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
On Thu, Oct 28, 2021 at 12:25:16PM +0200, Markus Armbruster wrote:
> The generated visitor functions call visit_deprecated_accept() and
> visit_deprecated() when visiting a struct member with special feature
> flag 'deprecated'. This makes the feature flag visible to the actual
> visitors. I want to make feature flag 'unstable' visible there as
> well, so I can add policy for it.
>
> To let me make it visible, replace these functions by
> visit_policy_reject() and visit_policy_skip(), which take the member's
> special features as an argument. Note that the new functions have the
> opposite sense, i.e. the return value flips.
>
> Signed-off-by: Markus Armbruster
> ---
> +++ b/qapi/qapi-forward-visitor.c
> @@ -246,25 +246,27 @@ static void forward_field_optional(Visitor *v, const
> char *name, bool *present)
> visit_optional(ffv->target, name, present);
> }
>
> -static bool forward_field_deprecated_accept(Visitor *v, const char *name,
> -Error **errp)
> +static bool forward_field_policy_reject(Visitor *v, const char *name,
> +unsigned special_features,
> +Error **errp)
> {
> ForwardFieldVisitor *ffv = to_ffv(v);
>
> if (!forward_field_translate_name(ffv, &name, errp)) {
> return false;
Should this return value be flipped?
> }
> -return visit_deprecated_accept(ffv->target, name, errp);
> +return visit_policy_reject(ffv->target, name, special_features, errp);
> }
>
> -static bool forward_field_deprecated(Visitor *v, const char *name)
> +static bool forward_field_policy_skip(Visitor *v, const char *name,
> + unsigned special_features)
> {
> ForwardFieldVisitor *ffv = to_ffv(v);
>
> if (!forward_field_translate_name(ffv, &name, NULL)) {
> return false;
and here too?
> }
> -return visit_deprecated(ffv->target, name);
> +return visit_policy_skip(ffv->target, name, special_features);
> }
>
Otherwise, the rest of the logic changes for flipped sense look right.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 9/9] qapi: Extend -compat to set policy for unstable interfaces
Eric Blake writes:
> On Thu, Oct 28, 2021 at 12:25:20PM +0200, Markus Armbruster wrote:
>> New option parameters unstable-input and unstable-output set policy
>> for unstable interfaces just like deprecated-input and
>> deprecated-output set policy for deprecated interfaces (see commit
>> 6dd75472d5 "qemu-options: New -compat to set policy for deprecated
>> interfaces"). This is intended for testing users of the management
>> interfaces. It is experimental.
>>
>> For now, this covers only syntactic aspects of QMP, i.e. stuff tagged
>> with feature 'unstable'. We may want to extend it to cover semantic
>> aspects, or the command line.
>>
>> Note that there is no good way for management application to detect
>> presence of these new option parameters: they are not visible output
>> of query-qmp-schema or query-command-line-options. Tolerable, because
>> it's meant for testing. If running with -compat fails, skip the test.
>
> Not to mention, once we finish QAPIfying the command line, we could
> make sure it is visible through introspection at that time (it may
> require tagging the command line option with a feature, if nothing
> else makes it pop through).
>
>>
>> Signed-off-by: Markus Armbruster
>> Acked-by: John Snow
>> ---
>> qapi/compat.json | 6 +-
>> include/qapi/util.h | 1 +
>> qapi/qmp-dispatch.c | 6 ++
>> qapi/qobject-output-visitor.c | 8 ++--
>> qemu-options.hx | 20 +++-
>> scripts/qapi/events.py| 10 ++
>> scripts/qapi/schema.py| 10 ++
>> 7 files changed, 49 insertions(+), 12 deletions(-)
>>
>> diff --git a/qapi/compat.json b/qapi/compat.json
>> index 74a8493d3d..9bc9804abb 100644
>> --- a/qapi/compat.json
>> +++ b/qapi/compat.json
>> @@ -47,9 +47,13 @@
>> #
>> # @deprecated-input: how to handle deprecated input (default 'accept')
>> # @deprecated-output: how to handle deprecated output (default 'accept')
>> +# @unstable-input: how to handle unstable input (default 'accept')
>> +# @unstable-output: how to handle unstable output (default 'accept')
>
> Missing '(since 6.2)' doc tags on the two new policies.
Fixing...
>> #
>> # Since: 6.0
>> ##
>> { 'struct': 'CompatPolicy',
>>'data': { '*deprecated-input': 'CompatPolicyInput',
>> -'*deprecated-output': 'CompatPolicyOutput' } }
>> +'*deprecated-output': 'CompatPolicyOutput',
>> +'*unstable-input': 'CompatPolicyInput',
>> +'*unstable-output': 'CompatPolicyOutput' } }
>> diff --git a/include/qapi/util.h b/include/qapi/util.h
>> index 0cc98db9f9..81a2b13a33 100644
>> --- a/include/qapi/util.h
>> +++ b/include/qapi/util.h
>> @@ -13,6 +13,7 @@
>>
>> typedef enum {
>> QAPI_DEPRECATED,
>> +QAPI_UNSTABLE,
>> } QapiSpecialFeature;
>
>> +++ b/qemu-options.hx
>> @@ -3641,7 +3641,9 @@ DEFHEADING(Debug/Expert options:)
>>
>> DEF("compat", HAS_ARG, QEMU_OPTION_compat,
>> "-compat
>> [deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n"
>> -"Policy for handling deprecated management
>> interfaces\n",
>> +"Policy for handling deprecated management interfaces\n"
>> +"-compat
>> [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
>> +"Policy for handling unstable management interfaces\n",
>
> It may not be machine-introspectible, but at least we can grep --help
> output to see when the policy is usable for testing.
>
>> QEMU_ARCH_ALL)
>> SRST
>> ``-compat
>> [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
>> @@ -3659,6 +3661,22 @@ SRST
>> Suppress deprecated command results and events
>>
>> Limitation: covers only syntactic aspects of QMP.
>> +
>> +``-compat
>> [unstable-input=@var{input-policy}][,unstable-output=@var{output-policy}]``
>> +Set policy for handling unstable management interfaces (experimental):
>
> Once we QAPIfy the command line, this says we would add the 'unstable'
> feature flag to '-compat unstable-input'. How meta ;)
Yes :)
>And goes along
> with your comments earlier in the series about how we may use the
> 'unstable' feature even without the 'x-' naming prefix, once it is
> machine-detectible.
>
> With the doc tweak,
> Reviewed-by: Eric Blake
Thanks!
Re: [PATCH v2 9/9] qapi: Extend -compat to set policy for unstable interfaces
On Thu, Oct 28, 2021 at 12:25:20PM +0200, Markus Armbruster wrote:
> New option parameters unstable-input and unstable-output set policy
> for unstable interfaces just like deprecated-input and
> deprecated-output set policy for deprecated interfaces (see commit
> 6dd75472d5 "qemu-options: New -compat to set policy for deprecated
> interfaces"). This is intended for testing users of the management
> interfaces. It is experimental.
>
> For now, this covers only syntactic aspects of QMP, i.e. stuff tagged
> with feature 'unstable'. We may want to extend it to cover semantic
> aspects, or the command line.
>
> Note that there is no good way for management application to detect
> presence of these new option parameters: they are not visible output
> of query-qmp-schema or query-command-line-options. Tolerable, because
> it's meant for testing. If running with -compat fails, skip the test.
Not to mention, once we finish QAPIfying the command line, we could
make sure it is visible through introspection at that time (it may
require tagging the command line option with a feature, if nothing
else makes it pop through).
>
> Signed-off-by: Markus Armbruster
> Acked-by: John Snow
> ---
> qapi/compat.json | 6 +-
> include/qapi/util.h | 1 +
> qapi/qmp-dispatch.c | 6 ++
> qapi/qobject-output-visitor.c | 8 ++--
> qemu-options.hx | 20 +++-
> scripts/qapi/events.py| 10 ++
> scripts/qapi/schema.py| 10 ++
> 7 files changed, 49 insertions(+), 12 deletions(-)
>
> diff --git a/qapi/compat.json b/qapi/compat.json
> index 74a8493d3d..9bc9804abb 100644
> --- a/qapi/compat.json
> +++ b/qapi/compat.json
> @@ -47,9 +47,13 @@
> #
> # @deprecated-input: how to handle deprecated input (default 'accept')
> # @deprecated-output: how to handle deprecated output (default 'accept')
> +# @unstable-input: how to handle unstable input (default 'accept')
> +# @unstable-output: how to handle unstable output (default 'accept')
Missing '(since 6.2)' doc tags on the two new policies.
> #
> # Since: 6.0
> ##
> { 'struct': 'CompatPolicy',
>'data': { '*deprecated-input': 'CompatPolicyInput',
> -'*deprecated-output': 'CompatPolicyOutput' } }
> +'*deprecated-output': 'CompatPolicyOutput',
> +'*unstable-input': 'CompatPolicyInput',
> +'*unstable-output': 'CompatPolicyOutput' } }
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 0cc98db9f9..81a2b13a33 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -13,6 +13,7 @@
>
> typedef enum {
> QAPI_DEPRECATED,
> +QAPI_UNSTABLE,
> } QapiSpecialFeature;
> +++ b/qemu-options.hx
> @@ -3641,7 +3641,9 @@ DEFHEADING(Debug/Expert options:)
>
> DEF("compat", HAS_ARG, QEMU_OPTION_compat,
> "-compat
> [deprecated-input=accept|reject|crash][,deprecated-output=accept|hide]\n"
> -"Policy for handling deprecated management interfaces\n",
> +"Policy for handling deprecated management interfaces\n"
> +"-compat
> [unstable-input=accept|reject|crash][,unstable-output=accept|hide]\n"
> +"Policy for handling unstable management interfaces\n",
It may not be machine-introspectible, but at least we can grep --help
output to see when the policy is usable for testing.
> QEMU_ARCH_ALL)
> SRST
> ``-compat
> [deprecated-input=@var{input-policy}][,deprecated-output=@var{output-policy}]``
> @@ -3659,6 +3661,22 @@ SRST
> Suppress deprecated command results and events
>
> Limitation: covers only syntactic aspects of QMP.
> +
> +``-compat
> [unstable-input=@var{input-policy}][,unstable-output=@var{output-policy}]``
> +Set policy for handling unstable management interfaces (experimental):
Once we QAPIfy the command line, this says we would add the 'unstable'
feature flag to '-compat unstable-input'. How meta ;) And goes along
with your comments earlier in the series about how we may use the
'unstable' feature even without the 'x-' naming prefix, once it is
machine-detectible.
With the doc tweak,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
Philippe Mathieu-Daudé writes:
> On 10/26/21 11:46, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> On 10/25/21 07:25, Markus Armbruster wrote:
The code to check policy for handling deprecated input is triplicated.
Factor it out into compat_policy_input_ok() before I mess with it in
the next commit.
Signed-off-by: Markus Armbruster
---
include/qapi/compat-policy.h | 7 +
qapi/qapi-visit-core.c | 18 +
qapi/qmp-dispatch.c | 51 +++-
qapi/qobject-input-visitor.c | 19 +++---
4 files changed, 55 insertions(+), 40 deletions(-)
>>>
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 8cca18c891..e29ade134c 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -28,6 +28,40 @@
CompatPolicy compat_policy;
+static bool compat_policy_input_ok1(const char *adjective,
+CompatPolicyInput policy,
+ErrorClass error_class,
+const char *kind, const char *name,
+Error **errp)
+{
+switch (policy) {
+case COMPAT_POLICY_INPUT_ACCEPT:
+return true;
+case COMPAT_POLICY_INPUT_REJECT:
+error_set(errp, error_class, "%s %s %s disabled by policy",
+ adjective, kind, name);
+return false;
+case COMPAT_POLICY_INPUT_CRASH:
+default:
+abort();
>>>
>>> g_assert_not_reached() provides a nicer user experience.
>>
>> I find it hard to care for making the experience of a crash that should
>> never ever happen nicer :)
>
> Well COMPAT_POLICY_INPUT_CRASH can happen... What about:
Point.
>case COMPAT_POLICY_INPUT_CRASH:
>error_printf("%s %s %s disabled by policy",
> adjective, kind, name);
>abort();
>default:
>g_assert_not_reached();
Separate patch. I'd prefer to delay it a bit, to avoid rocking the boat
so close to the soft freeze.
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
Philippe Mathieu-Daudé writes:
> On 10/28/21 12:25, Markus Armbruster wrote:
>> The generated visitor functions call visit_deprecated_accept() and
>> visit_deprecated() when visiting a struct member with special feature
>> flag 'deprecated'. This makes the feature flag visible to the actual
>> visitors. I want to make feature flag 'unstable' visible there as
>> well, so I can add policy for it.
>>
>> To let me make it visible, replace these functions by
>> visit_policy_reject() and visit_policy_skip(), which take the member's
>> special features as an argument. Note that the new functions have the
>> opposite sense, i.e. the return value flips.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> include/qapi/visitor-impl.h | 6 --
>> include/qapi/visitor.h| 17 +
>> qapi/qapi-forward-visitor.c | 16 +---
>> qapi/qapi-visit-core.c| 22 --
>> qapi/qobject-input-visitor.c | 15 ++-
>> qapi/qobject-output-visitor.c | 9 ++---
>> qapi/trace-events | 4 ++--
>> scripts/qapi/visit.py | 14 +++---
>> 8 files changed, 63 insertions(+), 40 deletions(-)
>
>> -static bool qobject_input_deprecated_accept(Visitor *v, const char *name,
>> -Error **errp)
>> +static bool qobject_input_policy_reject(Visitor *v, const char *name,
>> +unsigned special_features,
>> +Error **errp)
>> {
>> +if (!(special_features & 1u << QAPI_DEPRECATED)) {
>> +return false;
>> +}
>> +
>> switch (v->compat_policy.deprecated_input) {
>> case COMPAT_POLICY_INPUT_ACCEPT:
>> -return true;
>> +return false;
>> case COMPAT_POLICY_INPUT_REJECT:
>> error_setg(errp, "Deprecated parameter '%s' disabled by policy",
>> name);
>> -return false;
>> +return true;
>> case COMPAT_POLICY_INPUT_CRASH:
>
> Clearer as:
>
>abort();
>default:
>g_assert_not_reached();
Maybe, but making it so has nothing to do with this patch. It could
perhaps be done in PATCH 8, or in a followup patch.
> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé
Okay to tack your R-by to the unmodified patch?
Thanks!
>
>> default:
>> abort();
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
On 10/28/21 12:25, Markus Armbruster wrote:
> The generated visitor functions call visit_deprecated_accept() and
> visit_deprecated() when visiting a struct member with special feature
> flag 'deprecated'. This makes the feature flag visible to the actual
> visitors. I want to make feature flag 'unstable' visible there as
> well, so I can add policy for it.
>
> To let me make it visible, replace these functions by
> visit_policy_reject() and visit_policy_skip(), which take the member's
> special features as an argument. Note that the new functions have the
> opposite sense, i.e. the return value flips.
>
> Signed-off-by: Markus Armbruster
> ---
> include/qapi/visitor-impl.h | 6 --
> include/qapi/visitor.h| 17 +
> qapi/qapi-forward-visitor.c | 16 +---
> qapi/qapi-visit-core.c| 22 --
> qapi/qobject-input-visitor.c | 15 ++-
> qapi/qobject-output-visitor.c | 9 ++---
> qapi/trace-events | 4 ++--
> scripts/qapi/visit.py | 14 +++---
> 8 files changed, 63 insertions(+), 40 deletions(-)
> -static bool qobject_input_deprecated_accept(Visitor *v, const char *name,
> -Error **errp)
> +static bool qobject_input_policy_reject(Visitor *v, const char *name,
> +unsigned special_features,
> +Error **errp)
> {
> +if (!(special_features & 1u << QAPI_DEPRECATED)) {
> +return false;
> +}
> +
> switch (v->compat_policy.deprecated_input) {
> case COMPAT_POLICY_INPUT_ACCEPT:
> -return true;
> +return false;
> case COMPAT_POLICY_INPUT_REJECT:
> error_setg(errp, "Deprecated parameter '%s' disabled by policy",
> name);
> -return false;
> +return true;
> case COMPAT_POLICY_INPUT_CRASH:
Clearer as:
abort();
default:
g_assert_not_reached();
Otherwise,
Reviewed-by: Philippe Mathieu-Daudé
> default:
> abort();
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
Juan Quintela writes: > Markus Armbruster wrote: >> The generated visitor functions call visit_deprecated_accept() and >> visit_deprecated() when visiting a struct member with special feature >> flag 'deprecated'. This makes the feature flag visible to the actual >> visitors. I want to make feature flag 'unstable' visible there as >> well, so I can add policy for it. >> >> To let me make it visible, replace these functions by >> visit_policy_reject() and visit_policy_skip(), which take the member's >> special features as an argument. Note that the new functions have the >> opposite sense, i.e. the return value flips. >> >> Signed-off-by: Markus Armbruster > > Reviewed-by: Juan Quintela > > Reversing accept/reject make things "interesting" for a review point of view. Sorry about that. >> + * @special_features is the member's special features encoded as a >> + * bitset of QapiSpecialFeature. > > Just to nitty pick, if you rename the variable to features, does the > sentece is clearer? Not to me, I'm afraid... Thanks!
Re: [PATCH 2/9] qapi: Mark unstable QMP parts with feature 'unstable'
On Mon, Oct 25, 2021 at 07:25:25AM +0200, Markus Armbruster wrote:
> Add special feature 'unstable' everywhere the name starts with 'x-',
> except for InputBarrierProperties member x-origin and
> MemoryBackendProperties member x-use-canonical-path-for-ramblock-id,
> because these two are actually stable.
>
> Signed-off-by: Markus Armbruster
> ---
> @@ -2495,27 +2508,57 @@
> #
> # Properties for throttle-group objects.
> #
> -# The options starting with x- are aliases for the same key without x- in
> -# the @limits object. As indicated by the x- prefix, this is not a stable
> -# interface and may be removed or changed incompatibly in the future. Use
> -# @limits for a supported stable interface.
> -#
> # @limits: limits to apply for this throttle group
> #
> +# Features:
> +# @unstable: All members starting with x- are aliases for the same key
> +#without x- in the @limits object. This is not a stable
> +#interface and may be removed or changed incompatibly in
> +#the future. Use @limits for a supported stable
> +#interface.
> +#
> # Since: 2.11
> ##
> { 'struct': 'ThrottleGroupProperties',
>'data': { '*limits': 'ThrottleLimits',
> -'*x-iops-total' : 'int', '*x-iops-total-max' : 'int',
> +'*x-iops-total': { 'type': 'int',
> + 'features': [ 'unstable' ] },
This struct has been around since 381bd74 (v6.0); but was not listed
as deprecated at the time. Do we still need it in 6.2, or have we
gone enough release cycles with the saner naming without x- that we
could drop this? But that is a question independent of this patch.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v2 7/9] qapi: Generalize enum member policy checking
On 10/28/21 12:25, Markus Armbruster wrote: > The code to check enumeration value policy can see special feature > flag 'deprecated' in QEnumLookup member flags[value]. I want to make > feature flag 'unstable' visible there as well, so I can add policy for > it. > > Instead of extending flags[], replace it by @special_features (a > bitset of QapiSpecialFeature), because that's how special features get > passed around elsewhere. > > Signed-off-by: Markus Armbruster > Acked-by: John Snow > --- > include/qapi/util.h| 5 + > qapi/qapi-visit-core.c | 3 ++- > scripts/qapi/types.py | 22 -- > 3 files changed, 15 insertions(+), 15 deletions(-) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 4/9] qapi: Tools for sets of special feature flags in generated code
On 10/28/21 12:25, Markus Armbruster wrote: > New enum QapiSpecialFeature enumerates the special feature flags. > > New helper gen_special_features() returns code to represent a > collection of special feature flags as a bitset. > > The next few commits will put them to use. > > Signed-off-by: Markus Armbruster > Reviewed-by: John Snow > --- > include/qapi/util.h| 4 > scripts/qapi/gen.py| 8 > scripts/qapi/schema.py | 3 +++ > 3 files changed, 15 insertions(+) Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 4/9] qapi: Tools for sets of special feature flags in generated code
Markus Armbruster wrote: > New enum QapiSpecialFeature enumerates the special feature flags. > > New helper gen_special_features() returns code to represent a > collection of special feature flags as a bitset. > > The next few commits will put them to use. > > Signed-off-by: Markus Armbruster > Reviewed-by: John Snow Reviewed-by: Juan Quintela
Re: [PATCH v2 6/9] qapi: Generalize command policy checking
Markus Armbruster wrote: > The code to check command policy can see special feature flag > 'deprecated' as command flag QCO_DEPRECATED. I want to make feature > flag 'unstable' visible there as well, so I can add policy for it. > > To let me make it visible, add member @special_features (a bitset of > QapiSpecialFeature) to QmpCommand, and adjust the generator to pass it > through qmp_register_command(). Then replace "QCO_DEPRECATED in > @flags" by QAPI_DEPRECATED in @special_features", and drop > QCO_DEPRECATED. > > Signed-off-by: Markus Armbruster > Reviewed-by: Philippe Mathieu-Daudé > Acked-by: John Snow Reviewed-by: Juan Quintela
Re: [PATCH v2 5/9] qapi: Generalize struct member policy checking
Markus Armbruster wrote: > The generated visitor functions call visit_deprecated_accept() and > visit_deprecated() when visiting a struct member with special feature > flag 'deprecated'. This makes the feature flag visible to the actual > visitors. I want to make feature flag 'unstable' visible there as > well, so I can add policy for it. > > To let me make it visible, replace these functions by > visit_policy_reject() and visit_policy_skip(), which take the member's > special features as an argument. Note that the new functions have the > opposite sense, i.e. the return value flips. > > Signed-off-by: Markus Armbruster Reviewed-by: Juan Quintela Reversing accept/reject make things "interesting" for a review point of view. > + * @special_features is the member's special features encoded as a > + * bitset of QapiSpecialFeature. Just to nitty pick, if you rename the variable to features, does the sentece is clearer? Later, Juan.
