[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 17/42] job: Move defer_to_main_loop to Job

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH 17/42] job: Move defer_to_main_loop to Job
Date: Wed, 16 May 2018 13:32:04 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0

On 05/16/2018 05:51 AM, Max Reitz wrote:

-static void commit_complete(BlockJob *job, void *opaque)
+static void commit_complete(Job *job, void *opaque)
-    CommitBlockJob *s = container_of(job, CommitBlockJob, common);
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);

Now this is just abuse.

...but it's not the first time someone packs two container_of() into
one, it appears.  So, whatever, I guess.

I don't think it's abuse. Why wouldn't I directly cast to the type that
I really want instead of casting to each of the uninteresting parent
classes, too?

Because the final parameter is called "member" and not "path". :-)

container_of() is using offsetof(); and in C99 7.17, the parameter is named "member-designator" which can indeed jump through multiple layers (any valid address constant, as defined in 6.6P9). I don't see this as abuse of the interface.

The best explanation I can come up with is that the original code
acquired the AioContext both of the block device at the time of the BH
(because that needs to be done), and at the time of
block_job_defer_to_main_loop() -- because the latter is probably the
context the block_job_defer_to_main_loop() call came from, so it should
be (b)locked.

But if that's the case, then the same should be done here.  The context
of the job may change between scheduling the BH and the BH being
executed, so we might lock a different context here than the one
job_defer_to_main_loop() ran in (i.e., job->aio_context at the time of
job_defer_to_main_loop() running).  And maybe we should lock that old
context, too -- just like block_job_defer_to_main_loop_bh() did.

Why should we lock the old context? We don't access anything protected
by it. Even the data->job->bs access has gone away because we now have

Because the old code did so and I don't know why. O:-)

Your explanation does make sense to me, though, so:

Then it's best to include that explanation in the commit message itself, to save the future reader the hassle of digging up this thread.

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

reply via email to

[Prev in Thread] Current Thread [Next in Thread]