From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60304) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bsPu7-0002rH-QS for qemu-devel@nongnu.org; Fri, 07 Oct 2016 03:49:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bsPu3-000110-Lb for qemu-devel@nongnu.org; Fri, 07 Oct 2016 03:49:14 -0400 Sender: Paolo Bonzini References: <1475272849-19990-1-git-send-email-jsnow@redhat.com> <1475272849-19990-5-git-send-email-jsnow@redhat.com> <20161005140251.GC1657@noname.str.redhat.com> <60b400e2-4b30-fa4b-77fc-6985c975f815@redhat.com> From: Paolo Bonzini Message-ID: <0a42fb74-50d8-1751-631e-b7bd68927196@redhat.com> Date: Fri, 7 Oct 2016 09:49:07 +0200 MIME-Version: 1.0 In-Reply-To: <60b400e2-4b30-fa4b-77fc-6985c975f815@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 04/11] blockjobs: Always use block_job_get_aio_context List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow , Kevin Wolf Cc: vsementsov@virtuozzo.com, famz@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org, stefanha@redhat.com On 06/10/2016 22:22, John Snow wrote: > Calls .complete(), for which the only implementation is > mirror_complete. Uh, this actually seems messy. Looks like there's > nothing to prevent us from calling this after we've already told it to > complete once. Yeah, it should have an if (s->should_complete) { return; } at the beginning. I have other mirror.c patches in my queue so I can take care of that. > block_job_cancel and block_job_complete are different. > > block_job_cancel is called in many places, but we can just add a similar > block_job_user_cancel if we wanted a version which takes care to acquire > context and one that does not. (Or we could just acquire the context > regardless, but Paolo warned me ominously that recursive locks are EVIL. > He sounded serious.) Not that many places: - block_job_finish_sync calls it, and you can just release/reacquire around the call to "finish(job, &local_err)". - there are two callers in blockdev.c, and you can just remove the acquire/release from blockdev.c if you push it in block_job_cancel. As to block_job_cancel_sync: - replication_stop is not acquiring s->secondary_disk->bs's AioContext. - there is no need to hold the AioContext between ->prepare and ->clean. My suggestion is to ref the blockjob after storing it in state->job (you probably should do that anyway) and unref'ing it in ->clean. Then you can call again let block_job_cancel_sync(bs->job) take the AioContext, which it will do in block_job_finish_sync. > block_job_complete has no direct callers outside of QMP, but it is also > used as a callback by block_job_complete_sync, used in qemu-img for > run_block_job. I can probably rewrite qemu_img to avoid this usage. No need to: qemu-img is not acquiring the AioContext, so it's okay to let block_job_complete do that (like block_job_cancel, block_job_complete will be called by block_job_finish_sync without the AioContext acquired). Paolo