[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 06/20] block: make bdrv_co_do_write_zeroes st
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH v2 06/20] block: make bdrv_co_do_write_zeroes stricter in producing aligned requests |
Date: |
Wed, 20 Nov 2013 12:01:46 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130923 Thunderbird/17.0.9 |
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
- [Qemu-devel] [PATCH v2 00/20] block & scsi: write_zeroes support through the whole stack, Paolo Bonzini, 2013/11/19
- [Qemu-devel] [PATCH v2 02/20] block: add flags to BlockRequest, Paolo Bonzini, 2013/11/19
- [Qemu-devel] [PATCH v2 01/20] block: generalize BlockLimits handling to cover bdrv_aio_discard too, Paolo Bonzini, 2013/11/19
- [Qemu-devel] [PATCH v2 04/20] block: add bdrv_aio_write_zeroes, Paolo Bonzini, 2013/11/19
- [Qemu-devel] [PATCH v2 03/20] block: add flags argument to bdrv_co_write_zeroes tracepoint, Paolo Bonzini, 2013/11/19
- [Qemu-devel] [PATCH v2 05/20] block: handle ENOTSUP from discard in generic code, Paolo Bonzini, 2013/11/19
- [Qemu-devel] [PATCH v2 06/20] block: make bdrv_co_do_write_zeroes stricter in producing aligned requests, Paolo Bonzini, 2013/11/19
- Re: [Qemu-devel] [PATCH v2 06/20] block: make bdrv_co_do_write_zeroes stricter in producing aligned requests, Peter Lieven, 2013/11/21
- [Qemu-devel] [PATCH v2 07/20] vpc, vhdx: add get_info, Paolo Bonzini, 2013/11/19
- [Qemu-devel] [PATCH v2 08/20] block drivers: add discard/write_zeroes properties to bdrv_get_info implementation, Paolo Bonzini, 2013/11/19