Re: [Qemu-devel] [PATCH 18/41] block-migration: add lock

[Orit Wasserman]

On 02/15/2013 07:46 PM, Paolo Bonzini wrote:
> Some state is shared between the block migration code and its AIO
> callbacks.  Once block migration will run outside the iothread,
> the block migration code and the AIO callbacks will be able to
> run concurrently.  Protect the critical sections with a separate
> lock.  Do the same for completed_sectors, which can be used from
> the monitor.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  block-migration.c     |   54 ++++++++++++++++++++++++++++++++++++++++++++++--
>  include/qemu/atomic.h |    1 +
>  2 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 21842b2..ea99163 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -54,7 +54,7 @@ typedef struct BlkMigDevState {
>      int64_t cur_sector;
>      int64_t cur_dirty;
>  
> -    /* Protected by iothread lock.  */
> +    /* Protected by block migration lock.  */
>      unsigned long *aio_bitmap;
>      int64_t completed_sectors;
>  } BlkMigDevState;
> @@ -69,7 +69,7 @@ typedef struct BlkMigBlock {
>      QEMUIOVector qiov;
>      BlockDriverAIOCB *aiocb;
>  
> -    /* Protected by iothread lock.  */
> +    /* Protected by block migration lock.  */
>      int ret;
>      QSIMPLEQ_ENTRY(BlkMigBlock) entry;
>  } BlkMigBlock;
> @@ -81,7 +81,7 @@ typedef struct BlkMigState {
>      QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list;
>      int64_t total_sector_sum;
>  
> -    /* Protected by iothread lock.  */
> +    /* Protected by lock.  */
>      QSIMPLEQ_HEAD(blk_list, BlkMigBlock) blk_list;
>      int submitted;
>      int read_done;
> @@ -90,10 +90,23 @@ typedef struct BlkMigState {
>      int transferred;
>      int prev_progress;
>      int bulk_completed;
> +
> +    /* Lock must be taken _inside_ the iothread lock.  */
> +    QemuMutex lock;
>  } BlkMigState;
>  
>  static BlkMigState block_mig_state;
>  
> +static void blk_mig_lock(void)
> +{
> +    qemu_mutex_lock(&block_mig_state.lock);
> +}
> +
> +static void blk_mig_unlock(void)
> +{
> +    qemu_mutex_unlock(&block_mig_state.lock);
> +}
> +
>  static void blk_send(QEMUFile *f, BlkMigBlock * blk)
>  {
>      int len;
> @@ -120,9 +133,11 @@ uint64_t blk_mig_bytes_transferred(void)
>      BlkMigDevState *bmds;
>      uint64_t sum = 0;
>  
> +    blk_mig_lock();
>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
>          sum += bmds->completed_sectors;
>      }
> +    blk_mig_unlock();
>      return sum << BDRV_SECTOR_BITS;
>  }
>  
> @@ -142,6 +157,9 @@ uint64_t blk_mig_bytes_total(void)
>      return sum << BDRV_SECTOR_BITS;
>  }
>  
> +
> +/* Called with migration lock held.  */
> +
>  static int bmds_aio_inflight(BlkMigDevState *bmds, int64_t sector)
>  {
>      int64_t chunk = sector / (int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK;
> @@ -154,6 +172,8 @@ static int bmds_aio_inflight(BlkMigDevState *bmds, 
> int64_t sector)
>      }
>  }
>  
> +/* Called with migration lock held.  */
> +
>  static void bmds_set_aio_inflight(BlkMigDevState *bmds, int64_t sector_num,
>                               int nb_sectors, int set)
>  {
> @@ -188,10 +208,13 @@ static void alloc_aio_bitmap(BlkMigDevState *bmds)
>      bmds->aio_bitmap = g_malloc0(bitmap_size);
>  }
>  
> +/* Never hold migration lock when yielding to the main loop!  */
> +
>  static void blk_mig_read_cb(void *opaque, int ret)
>  {
>      BlkMigBlock *blk = opaque;
>  
> +    blk_mig_lock();
>      blk->ret = ret;
>  
>      QSIMPLEQ_INSERT_TAIL(&block_mig_state.blk_list, blk, entry);
> @@ -199,6 +222,7 @@ static void blk_mig_read_cb(void *opaque, int ret)
>  
>      block_mig_state.submitted--;
>      block_mig_state.read_done++;
> +    blk_mig_unlock();
>      assert(block_mig_state.submitted >= 0);
Shouldn't the assert be under the lock too?

>  }
>  
> @@ -244,7 +268,9 @@ static int mig_save_device_bulk(QEMUFile *f, 
> BlkMigDevState *bmds)
>      blk->iov.iov_len = nr_sectors * BDRV_SECTOR_SIZE;
>      qemu_iovec_init_external(&blk->qiov, &blk->iov, 1);
>  
> +    blk_mig_lock();
>      block_mig_state.submitted++;
> +    blk_mig_unlock();
>  
>      blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
>                                  nr_sectors, blk_mig_read_cb, blk);
> @@ -366,8 +392,12 @@ static int mig_save_device_dirty(QEMUFile *f, 
> BlkMigDevState *bmds,
>      int ret = -EIO;
>  
>      for (sector = bmds->cur_dirty; sector < bmds->total_sectors;) {
> +        blk_mig_lock();
>          if (bmds_aio_inflight(bmds, sector)) {
> +            blk_mig_unlock();
>              bdrv_drain_all();
> +        } else {
> +            blk_mig_unlock();
>          }
>          if (bdrv_get_dirty(bmds->bs, sector)) {
>  
> @@ -389,8 +419,11 @@ static int mig_save_device_dirty(QEMUFile *f, 
> BlkMigDevState *bmds,
>  
>                  blk->aiocb = bdrv_aio_readv(bmds->bs, sector, &blk->qiov,
>                                              nr_sectors, blk_mig_read_cb, 
> blk);
> +
> +                blk_mig_lock();
>                  block_mig_state.submitted++;
>                  bmds_set_aio_inflight(bmds, sector, nr_sectors, 1);
> +                blk_mig_unlock();
>              } else {
>                  ret = bdrv_read(bmds->bs, sector, blk->buf, nr_sectors);
>                  if (ret < 0) {
> @@ -446,6 +479,7 @@ static int flush_blks(QEMUFile *f)
>              __FUNCTION__, block_mig_state.submitted, 
> block_mig_state.read_done,
>              block_mig_state.transferred);
>  
> +    blk_mig_lock();
>      while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) {
>          if (qemu_file_rate_limit(f)) {
>              break;
> @@ -456,7 +490,9 @@ static int flush_blks(QEMUFile *f)
>          }
>  
>          QSIMPLEQ_REMOVE_HEAD(&block_mig_state.blk_list, entry);
> +        blk_mig_unlock();
>          blk_send(f, blk);
> +        blk_mig_lock();
>  
>          g_free(blk->buf);
>          g_free(blk);
> @@ -465,6 +501,7 @@ static int flush_blks(QEMUFile *f)
>          block_mig_state.transferred++;
>          assert(block_mig_state.read_done >= 0);
>      }
> +    blk_mig_unlock();
>  
>      DPRINTF("%s Exit submitted %d read_done %d transferred %d\n", 
> __FUNCTION__,
>              block_mig_state.submitted, block_mig_state.read_done,
> @@ -493,6 +530,7 @@ static void blk_mig_cleanup(void)
>  
>      set_dirty_tracking(0);
>  
> +    blk_mig_lock();
>      while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
>          QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
>          bdrv_set_in_use(bmds->bs, 0);
> @@ -506,6 +544,7 @@ static void blk_mig_cleanup(void)
>          g_free(blk->buf);
>          g_free(blk);
>      }
> +    blk_mig_unlock();
>  }
>  
>  static void block_migration_cancel(void *opaque)
> @@ -548,9 +587,11 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>      blk_mig_reset_dirty_cursor();
>  
>      /* control the rate of transfer */
> +    blk_mig_lock();
>      while ((block_mig_state.submitted +
>              block_mig_state.read_done) * BLOCK_SIZE <
>             qemu_file_get_rate_limit(f)) {
> +        blk_mig_unlock();
>          if (block_mig_state.bulk_completed == 0) {
>              /* first finish the bulk phase */
>              if (blk_mig_save_bulked_block(f) == 0) {
> @@ -564,11 +605,13 @@ static int block_save_iterate(QEMUFile *f, void *opaque)
>          if (ret < 0) {
>              return ret;
>          }
> +        blk_mig_lock();
>          if (ret != 0) {
>              /* no more dirty blocks */
>              break;
>          }
>      }
> +    blk_mig_unlock();
>  
>      ret = flush_blks(f);
>      if (ret) {
> @@ -595,7 +638,9 @@ static int block_save_complete(QEMUFile *f, void *opaque)
>  
>      /* we know for sure that save bulk is completed and
>         all async read completed */
> +    blk_mig_lock();
>      assert(block_mig_state.submitted == 0);
> +    blk_mig_unlock();
>  
>      do {
>          ret = blk_mig_save_dirty_block(f, 0);
> @@ -620,6 +665,7 @@ static uint64_t block_save_pending(QEMUFile *f, void 
> *opaque, uint64_t max_size)
>      /* Estimate pending number of bytes to send */
>      uint64_t pending;
>  
> +    blk_mig_lock();
>      pending = get_remaining_dirty() +
>                         block_mig_state.submitted * BLOCK_SIZE +
>                         block_mig_state.read_done * BLOCK_SIZE;
> @@ -628,6 +674,7 @@ static uint64_t block_save_pending(QEMUFile *f, void 
> *opaque, uint64_t max_size)
>      if (pending == 0 && !block_mig_state.bulk_completed) {
>          pending = BLOCK_SIZE;
>      }
> +    blk_mig_unlock();
>  
>      DPRINTF("Enter save live pending  %" PRIu64 "\n", pending);
>      return pending;
> @@ -739,6 +786,7 @@ void blk_mig_init(void)
>  {
>      QSIMPLEQ_INIT(&block_mig_state.bmds_list);
>      QSIMPLEQ_INIT(&block_mig_state.blk_list);
> +    qemu_mutex_init(&block_mig_state.lock);
>  
>      register_savevm_live(NULL, "block", 0, 1, &savevm_block_handlers,
>                           &block_mig_state);
> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
> index 96a194b..10becb6 100644
> --- a/include/qemu/atomic.h
> +++ b/include/qemu/atomic.h
> @@ -16,6 +16,7 @@
>   */
>  #define smp_wmb()   barrier()
>  #define smp_rmb()   barrier()
> +
White space damage

Orit
>  /*
>   * We use GCC builtin if it's available, as that can use
>   * mfence on 32 bit as well, e.g. if built with -march=pentium-m.
>