From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54100) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fqByk-0007ID-P8 for qemu-devel@nongnu.org; Thu, 16 Aug 2018 02:41:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fqByf-0006cH-IX for qemu-devel@nongnu.org; Thu, 16 Aug 2018 02:41:53 -0400 Date: Thu, 16 Aug 2018 08:41:28 +0200 From: Kevin Wolf Message-ID: <20180816064128.GA5511@localhost.localdomain> References: <20180807043349.27196-1-jsnow@redhat.com> <20180807043349.27196-5-jsnow@redhat.com> <20180808162948.GF15410@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 04/21] block/commit: utilize job_exit shim List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Jeff Cody , Max Reitz , jtc@redhat.com, Markus Armbruster , "Dr. David Alan Gilbert" , Eric Blake Am 15.08.2018 um 22:52 hat John Snow geschrieben: > On 08/08/2018 12:29 PM, Kevin Wolf wrote: > > Am 07.08.2018 um 06:33 hat John Snow geschrieben: > >> Change the manual deferment to commit_complete into the implicit > >> callback to job_exit. > >> > >> Signed-off-by: John Snow > > > > There is one tricky thing in this patch that the commit message could be > > a bit more explicit about, which is moving job_completed() to a later > > point. > > > > This is the code that happens between the old call of job_completed() > > and the new one: > > > > /* If bdrv_drop_intermediate() didn't already do that, remove the commit > > * filter driver from the backing chain. Do this as the final step so that > > * the 'consistent read' permission can be granted. */ > > if (remove_commit_top_bs) { > > bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL, > > &error_abort); > > bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs), > > &error_abort); > > } > > > > bdrv_unref(commit_top_bs); > > bdrv_unref(top); > > > > As the comment states, bdrv_replace_node() requires that the permission > > restrictions that the commit job made are already lifted. The most > > important part is done by the explicit block_job_remove_all_bdrv() call > > right before this hunk. It still leaves bjob->blk around, which could > > have implications, but luckily we didn't take any permissions for that > > one: > > > > s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL, > > speed, JOB_DEFAULT, NULL, NULL, errp); > > > > So I think we got everything out of the way and bdrv_replace_node() can > > do what it wants to do. > > > > Kevin > > > > I suppose it will be up to the author of a job to be aware of any > permissions they pick up at creation time that might have an effect on > the cleanup they wish to do during completion time. I'm really only talking about the commit job, the specific conversion in this patch and the terse commit message. Kevin