Am 02.11.2011 16:43, schrieb Stefan Hajnoczi:
> On Tue, Nov 1, 2011 at 6:06 PM, Marcelo Tosatti <mtosa...@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
