Re: [Qemu-devel] [PATCH 05/27] block/parallels: add get_block_status

["Denis V. Lunev" <den@openvz.org>]

On 23/04/15 12:03, Stefan Hajnoczi wrote:
> On Wed, Apr 22, 2015 at 03:42:23PM +0300, Denis V. Lunev wrote:
>> On 22/04/15 15:39, Stefan Hajnoczi wrote:
>>> On Wed, Mar 11, 2015 at 01:27:59PM +0300, Denis V. Lunev wrote:
>>>> +static int64_t coroutine_fn parallels_co_get_block_status(BlockDriverState *bs,
>>>> +        int64_t sector_num, int nb_sectors, int *pnum)
>>>> +{
>>>> +    BDRVParallelsState *s = bs->opaque;
>>>> +    int64_t offset;
>>>> +
>>>> +    qemu_co_mutex_lock(&s->lock);
>>>> +    offset = seek_to_sector(s, sector_num);
>>>> +    qemu_co_mutex_unlock(&s->lock);
>>> The lock isn't necessary here yet.  It may become necessary when write
>>> support is added, but probably not even then since seek_to_sector()
>>> cannot yield (it's not a coroutine function), so there are no possible
>>> races with other coroutines.
>>>
>>> The same also applies for parallels_co_read().  The lock there isn't
>>> necessary since the block driver is read-only and has no mutable state -
>>> there is no shared state that needs lock protection.
>> do you propose to drop the lock here and add it later with write
>> support (patch 08)? I'd prefer to stay on the safe side. Yes, the
>> lock is not mandatory at the moment but in 2 patches it will be
>> mandatory.
> This locking approach is very conservative.  At the end of the series,
> the only region that needs a lock is allocate_clusters() because
> bdrv_write_zeroes() may yield before s->bat_bitmap[] is assigned - we
> need to prevent simultaneous allocate_clusters() calls to the same
> clusters.
>
> I'm fine with the conservative approach, but please add a doc comment to
> BDRVParallelsState.lock explaining what this lock protects.  This way
> people reading the code will understand your philosophy and be able to
> follow it when modifying the code.
>
> Stefan
ok. sounds good for me.

I would like to implement the code as conservative as possible at the
beginning and place optimizations later step-by-step to have a
clear path to bisect the introductory patch.

At the moment I do not think that this locking scheme will affect
the performance but I can be wrong. There are a lot of stuff to
be implemented from the functional point of view before this will
be needed to tweak from my point of view.