On Wed, Nov 20, 2013 at 12:01:46PM +0100, Paolo Bonzini wrote:
> 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;
> }
Thanks, that is clearer.
Stefan
