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

[Denis V. Lunev]

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.