From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dT6ra-00080W-N4 for qemu-devel@nongnu.org; Thu, 06 Jul 2017 09:30:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dT6rZ-0006xm-Lm for qemu-devel@nongnu.org; Thu, 06 Jul 2017 09:30:34 -0400 Date: Thu, 6 Jul 2017 15:30:17 +0200 From: Kevin Wolf Message-ID: <20170706133017.GE5975@noname.redhat.com> References: <20170705210842.960-1-eblake@redhat.com> <20170705210842.960-13-eblake@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170705210842.960-13-eblake@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 12/21] mirror: Switch mirror_do_read() to byte-based List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, jsnow@redhat.com, jcody@redhat.com, qemu-block@nongnu.org, Max Reitz Am 05.07.2017 um 23:08 hat Eric Blake geschrieben: > We are gradually converting to byte-based interfaces, as they are > easier to reason about than sector-based. Convert another internal > function (no semantic change). > > Signed-off-by: Eric Blake > Reviewed-by: John Snow > Reviewed-by: Jeff Cody > - /* The sector range must meet granularity because: > + assert(bytes <= s->buf_size); > + /* The range will be sector-aligned because: > * 1) Caller passes in aligned values; > - * 2) mirror_cow_align is used only when target cluster is larger. */ > - assert(!(sector_num % sectors_per_chunk)); > - nb_chunks = DIV_ROUND_UP(nb_sectors, sectors_per_chunk); > + * 2) mirror_cow_align is used only when target cluster is larger. > + * But it might not be cluster-aligned at end-of-file. */ > + assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE)); > + nb_chunks = DIV_ROUND_UP(bytes, s->granularity); The assertion in the old code was about sector_num (i.e. that the start of the range is cluster-aligned), not about nb_sectors, so while you add a new assertion, it is asserting something different. This explains why you had to switch to sector aligned even though the semantics shouldn't be changed by this patch. Is this intentional or did you confuse sector_num and nb_sectors? I think we can just have both assertions. The rest of the patch looks okay. Kevin