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

[Stefan Hajnoczi]

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

pgp8wZSIa1OqD.pgp
Description: PGP signature