From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45440) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZybLi-0002pt-Ep for qemu-devel@nongnu.org; Tue, 17 Nov 2015 03:10:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZybLe-0005V4-5i for qemu-devel@nongnu.org; Tue, 17 Nov 2015 03:10:46 -0500 Received: from relay.parallels.com ([195.214.232.42]:59855) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZybLd-0005M9-TU for qemu-devel@nongnu.org; Tue, 17 Nov 2015 03:10:42 -0500 References: <1447687481-29244-1-git-send-email-den@openvz.org> <1447687481-29244-6-git-send-email-den@openvz.org> <20151117072201.GI16268@stefanha-x1.localdomain> From: "Denis V. Lunev" Message-ID: <564AE0F2.5010208@openvz.org> Date: Tue, 17 Nov 2015 11:10:26 +0300 MIME-Version: 1.0 In-Reply-To: <20151117072201.GI16268@stefanha-x1.localdomain> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 05/10] snapshot: create bdrv_all_find_snapshot helper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , qemu-devel@nongnu.org, Juan Quintela On 11/17/2015 10:22 AM, Stefan Hajnoczi wrote: > On Mon, Nov 16, 2015 at 06:24:36PM +0300, Denis V. Lunev wrote: >> +int bdrv_all_find_snapshot(const char *name, bool skip_read_only, >> + BlockDriverState **first_bad_bs) >> +{ >> + QEMUSnapshotInfo sn; >> + int err = 0; >> + BlockDriverState *bs = NULL; >> + >> + while (err == 0 && (bs = bdrv_next(bs))) { >> + AioContext *ctx = bdrv_get_aio_context(bs); >> + >> + if (skip_read_only && >> + (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs))) { > These must be called with AioContext acquired. > AFAIK no. this is called without that in bdrv jobs code. bdrv_is_read_only is a simple flag, which is filled on open bdrv_is_inserted is defined for CDROM only and does not rely on AIO context stuff. >> + continue; >> + } >> + >> + aio_context_acquire(ctx); >> + if (bdrv_can_snapshot(bs)) { > The !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) checks above are > redundant since bdrv_can_snapshot() checks too: > > int bdrv_can_snapshot(BlockDriverState *bs) > { > BlockDriver *drv = bs->drv; > if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) { > return 0; > } > > It also means that the "skip_read_only" name is inaccurate. Read-only > drives are always skipped, regardless of skip_read_only's value. > > The skip_read_only argument can be dropped and the earlier > !bdrv_is_inserted(bs) || bdrv_is_read_only(bs) check can be dropped too. I have though on this several times and once again I am wrong :( This looks like a punishment for my sins. OK :( >> @@ -2168,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict) >> available_snapshots = g_new0(int, nb_sns); >> total = 0; >> for (i = 0; i < nb_sns; i++) { >> - sn = &sn_tab[i]; >> - available = 1; >> - bs1 = NULL; >> - >> - while ((bs1 = bdrv_next(bs1))) { >> - if (bdrv_can_snapshot(bs1) && bs1 != bs) { >> - ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str); >> - if (ret < 0) { >> - available = 0; >> - break; >> - } >> - } >> - } >> - >> - if (available) { >> + if (bdrv_all_find_snapshot(sn_tab[i].id_str, false, &bs1) == 0) { > bdrv_all_find_snapshot() doesn't do the bs1 != bs exclusion so the new > code behaves differently from the old code. That seems like a bug. no. The result will be the same. This is a minor optimisation: - we get the first block device we can make snapshot on - we get snapshot list on that device - after that we start iterations over the list and removing not available snapshots on other devices This means that if bs == bs1 the check will be "always true"