From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48749) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vj5Xl-0003So-SF for qemu-devel@nongnu.org; Wed, 20 Nov 2013 06:02:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vj5Xf-0004tt-Rn for qemu-devel@nongnu.org; Wed, 20 Nov 2013 06:02:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:63884) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vj5Xf-0004tl-K2 for qemu-devel@nongnu.org; Wed, 20 Nov 2013 06:01:55 -0500 Message-ID: <528C969A.1090704@redhat.com> Date: Wed, 20 Nov 2013 12:01:46 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1384880863-10434-1-git-send-email-pbonzini@redhat.com> <1384880863-10434-7-git-send-email-pbonzini@redhat.com> <20131120102256.GC25447@stefanha-thinkpad.muc.redhat.com> In-Reply-To: <20131120102256.GC25447@stefanha-thinkpad.muc.redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 06/20] block: make bdrv_co_do_write_zeroes stricter in producing aligned requests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, pl@kamp.de, qemu-devel@nongnu.org, stefanha@redhat.com Il 20/11/2013 11:22, Stefan Hajnoczi ha scritto: >> > + && num > bs->bl.write_zeroes_alignment) { > Here '>' is used... > >> > + if (sector_num % bs->bl.write_zeroes_alignment != 0) { >> > + /* Make a small request up to the first aligned sector. */ >> > num = bs->bl.write_zeroes_alignment; >> > + num -= sector_num % bs->bl.write_zeroes_alignment; >> > + } else if (num >= bs->bl.write_zeroes_alignment) { > ...but here '>=' is used. > > The == case here cannot happen. Did you mean '>=' in both places? I meant what I wrote, in the sense that the two "if"s make sense the way I wrote them. However, it is not too clear indeed. > if (bs->bl.write_zeroes_alignment > && num > bs->bl.write_zeroes_alignment) { Here, '>' is a necessary (though not sufficient) for being able to split the operation in one misaligned request and one aligned request. If '<', the request is misaligned in either the starting sector or the ending sector (or both), and there's no need to split it. If '==', either the request is aligned or we can only split it in two parts but they remain misaligned. In either case there's no need to do anyting. Because the condition is not sufficient, we may end up splitting a request that cannot be aligned anyway (e.g. sector_num = 1, num = 129, alignment = 128; will be split into [1,128) and [128,130) which are both misaligned. This is not important. > if (sector_num % bs->bl.write_zeroes_alignment != 0) { > /* Make a small request up to the first aligned sector. */ > num = bs->bl.write_zeroes_alignment; > num -= sector_num % bs->bl.write_zeroes_alignment; > } else if (num >= bs->bl.write_zeroes_alignment) { > /* Shorten the request to the last aligned sector. */ > num -= (sector_num + num) % bs->bl.write_zeroes_alignment; Here, the "if" checks that we can have no underflow in the subtraction. However, in addition to what you pointed out, it's not immediately obvious that the subtraction has no effect if sector_num and num are correctly aligned. So I will rewrite the "if" this way: if (sector_num % bs->bl.write_zeroes_alignment != 0) { /* Make a small request up to the first aligned sector. */ num = bs->bl.write_zeroes_alignment; num -= sector_num % bs->bl.write_zeroes_alignment; } else if ((sector_num + num) % bs->bl.write_zeroes_alignment != 0) { /* Shorten the request to the last aligned sector. num cannot * underflow because num > bs->bl.write_zeroes_alignment. */ num -= (sector_num + num) % bs->bl.write_zeroes_alignment; } Paolo