Re: [Qemu-devel] [PATCH 3/8] block: add image streaming block job

[Kevin Wolf <kwolf@redhat.com>]

Am 02.11.2011 16:43, schrieb Stefan Hajnoczi:
> On Tue, Nov 1, 2011 at 6:06 PM, Marcelo Tosatti <mtosatti@redhat.com> wrote:
>> On Thu, Oct 27, 2011 at 04:22:50PM +0100, Stefan Hajnoczi wrote:
>>> +static int stream_one_iteration(StreamBlockJob *s, int64_t sector_num,
>>> +                                void *buf, int max_sectors, int *n)
>>> +{
>>> +    BlockDriverState *bs = s->common.bs;
>>> +    int ret;
>>> +
>>> +    trace_stream_one_iteration(s, sector_num, max_sectors);
>>> +
>>> +    ret = bdrv_is_allocated(bs, sector_num, max_sectors, n);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>
>> bdrv_is_allocated is still synchronous? If so, there should be at least
>> a plan to make it asynchronous.
> 
> Yes, that's a good discussion to have.  My thoughts are that
> bdrv_is_allocated() should be executed in coroutine context.  

bdrv_is_allocated() isn't coroutine-safe. You need to introduce
bdrv_co_is_allocated and convert all drivers before you can do this. You
don't want to access the qcow2 metadata cache without holding the lock,
for example.

> The
> semantics are a little tricky because of parallel requests:
> 
> 1. If a write request is in progress when we do bdrv_is_allocated() we
> might get back "unallocated" even though clusters are just being
> allocated.
> 2. If a TRIM request is in progress when we do bdrv_is_allocated() we
> might get back "allocated" even though clusters are just being
> deallocated.
> 
> In order to be reliable the caller needs to be aware of parallel
> requests.  I think it's correct to defer this problem to the caller.

I agree.

> In the case of image streaming we're not TRIM-safe, I haven't really
> thought about it yet.  But we are safe against parallel write requests
> because there is serialization to prevent copy-on-read requests from
> racing with write requests.

I don't think it matters. If you lose a bdrv_discard, nothing bad has
happened. bdrv_discard means that you have undefined content afterwards.

Kevin