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

[Stefan Hajnoczi]

On Thu, Apr 23, 2015 at 12:23:43PM +0300, Denis V. Lunev wrote:
> 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.

Okay.

Stefan

pgp_HklmHiAsJ.pgp
Description: PGP signature