Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking

[Markus Armbruster]

Subject line made me go "huh?" until I realized that you mean "Guest
locks up", not "QEMU uses locks to synchronize".  Suggest to add the
preposition :)

Peter Lieven <address@hidden> writes:

> If io_limits are specified during runtime that exceed the number of 
> operations in flight
> bs->io_base is not initialized in the else statement in 
> bdrv_exceed_io_limits().

I'm confused.

    if ((bs->slice_start < now)
        && (bs->slice_end > now)) {
        bs->slice_end = now + bs->slice_time;
    } else {
        bs->slice_time  =  5 * BLOCK_IO_SLICE_TIME;
        bs->slice_start = now;
        bs->slice_end   = now + bs->slice_time;

        bs->io_base.bytes[is_write]  = bs->nr_bytes[is_write];
        bs->io_base.bytes[!is_write] = bs->nr_bytes[!is_write];

        bs->io_base.ios[is_write]    = bs->nr_ops[is_write];
        bs->io_base.ios[!is_write]   = bs->nr_ops[!is_write];
    }

It's set in the else.  Could you explain what goes wrong in a bit more
detail?

> The wait time calculated in bdrv_exceed_{bps,iops}_limits is thus totally 
> wrong
> and the machine locks.

Please wrap lines around column 70.

>
> Signed-off-by: Peter Lieven <address@hidden>
> ---
>  block.c |    4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/block.c b/block.c
> index 4e28c55..309aa85 100644
> --- a/block.c
> +++ b/block.c
> @@ -159,6 +159,10 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
>      bs->slice_start = qemu_get_clock_ns(vm_clock);
>      bs->slice_end   = bs->slice_start + bs->slice_time;
>      memset(&bs->io_base, 0, sizeof(bs->io_base));
> +    bs->io_base.bytes[0] = bs->nr_bytes[0];
> +    bs->io_base.bytes[1] = bs->nr_bytes[1];
> +    bs->io_base.ios[0] = bs->nr_ops[0];
> +    bs->io_base.ios[1] = bs->nr_ops[1];
>      bs->io_limits_enabled = true;
>  }