From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59816) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fk7Hw-0006Gt-RC for qemu-devel@nongnu.org; Mon, 30 Jul 2018 08:28:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fk7Hv-0007YX-RI for qemu-devel@nongnu.org; Mon, 30 Jul 2018 08:28:36 -0400 Date: Mon, 30 Jul 2018 14:28:28 +0200 From: Kevin Wolf Message-ID: <20180730122828.GD3775@localhost.localdomain> References: <20180729212744.23709-1-lbloch@janustech.com> <20180729212744.23709-5-lbloch@janustech.com> <20180730094320.GB3775@localhost.localdomain> <8d1a2a16-6fa6-1318-d278-ff0c8b45a6f5@janustech.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8d1a2a16-6fa6-1318-d278-ff0c8b45a6f5@janustech.com> Subject: Re: [Qemu-devel] [PATCH 4/6] qcow2: Update total_sectors when resizing the image List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Bloch Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org, Max Reitz , Eric Blake Am 30.07.2018 um 13:41 hat Leonid Bloch geschrieben: > On 07/30/2018 12:43 PM, Kevin Wolf wrote: > > Am 29.07.2018 um 23:27 hat Leonid Bloch geschrieben: > > > Signed-off-by: Leonid Bloch > > > --- > > > block/qcow2.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/block/qcow2.c b/block/qcow2.c > > > index ec9e6238a0..223d351e40 100644 > > > --- a/block/qcow2.c > > > +++ b/block/qcow2.c > > > @@ -3646,6 +3646,8 @@ static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset, > > > } > > > } > > > + bs->total_sectors = offset / 512; > > > + > > > /* write updated header.size */ > > > offset = cpu_to_be64(offset); > > > ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size), > > > > This shouldn't be necessary, bdrv_co_truncate() already updates > > bs->total_sectors after calling the block driver. > > It looks like without it the cache resize works only on the second resize. Yes, and after reading the rest of the series, it makes sense to me because qcow2_update_option() -> read_cache_sizes() uses bs->total_sectors, so if we call that in qcow2_co_truncate(), we already need to update the value early. The comment should explain this connection because otherwise it's not obvious when you read the code. > > If this is needed by one of the following patches, we need a comment > > that explains why this seemingly superfluous assignment is actually > > necessary. > > > > Also, 512 should be BDRV_SECTOR_SIZE. > > I was surprised that it's not, but it's 512 also in two other places, > including in qcow2_co_truncate itself. So I decided to keep that. Probably > would be better if I'd repair it in the other places instead. :) Yeah, or at least not introduce new places with a literal 512. Kevin