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

[Stefan Hajnoczi <stefanha@redhat.com>]

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