From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41398) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZrN6U-0002PI-KW for qemu-devel@nongnu.org; Wed, 28 Oct 2015 05:33:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZrN6T-0005ZG-MA for qemu-devel@nongnu.org; Wed, 28 Oct 2015 05:33:10 -0400 Date: Wed, 28 Oct 2015 10:33:01 +0100 From: Kevin Wolf Message-ID: <20151028093301.GB3836@noname.str.redhat.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 0/1] Test the reopening of overlay_bs in 'block-commit' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: Stefan Hajnoczi , qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz Am 27.10.2015 um 17:43 hat Alberto Garcia geschrieben: > Hi, looks like we have a bug in the bdrv_reopen() code. > > It turns out that 'block-commit' fails if the 'top' node is not the > active layer or its immediate backing file, and none of our test cases > has detected that. I'm attaching one that reproduces the problem. > > What happens is that 'block-commit' reopens the overlay of the top > node in read-write mode in order to update the backing file string. In > addition to that, the 'base' image also needs to be reopened in r/w. > > Here's the relevant code from commit_start(): > > if (!(orig_base_flags & BDRV_O_RDWR)) { > reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL, > orig_base_flags | BDRV_O_RDWR); > } > if (!(orig_overlay_flags & BDRV_O_RDWR)) { > reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL, > orig_overlay_flags | BDRV_O_RDWR); > } > if (reopen_queue) { > bdrv_reopen_multiple(reopen_queue, &local_err); > /*...*/ > } > > 'base' is reopened first in r/w mode, then 'overlay_bs'. However it > seems that the latter has the side effect or reopening 'base' again in > read-only mode, therefore the job ends up failing with -EPERM. > > Just swapping the order of the bdrv_reopen_queue() calls is enough to > fix the problem, but I'm sure this needs deeper changes in the > bdrv_reopen() code instead. I think this might be fixed by applying the rest of my bdrv_reopen() patches and then converting read-only from a flag to an option. The reason is that we really have three values here: ro, rw and inherit from parent. This can be represented by options (false, true, missing QDict entry), but not by flags. I've applied your test case to my working branch so I won't forget about this. Maybe I should really try to get the series into 2.5 then. All that being said, having the same BDS twice in a reopen queue is calling for trouble. I guess currently the last one wins, with all changes made in the first call reverted even if the respective options aren't touched by the second one. I wonder if we need to merge multiple entries for the same BDS. Kevin