Re: [Qemu-devel] [PATCH v2 06/20] block: make bdrv_co_do_write_zeroes stricter in producing aligned requests

[Paolo Bonzini <pbonzini@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