Re: [Qemu-devel] [PATCH] blockdev: avoid acquiring AioContext lock twice at do_drive_backup()

[Max Reitz]

On 12.09.19 18:16, Sergio Lopez wrote:
> do_drive_backup() acquires the AioContext lock of the corresponding
> BlockDriverState. This is not a problem when it's called from
> qmp_drive_backup(), but drive_backup_prepare() also acquires the lock
> before calling it.
> 
> This change adds a BlockDriverState argument to do_drive_backup(),
> which is used to signal that the context lock is already acquired and
> to save a couple of redundant calls.

But those redundant calls don’t really hurt (it’s just bdrv_lookup_bs(),
as far as I can tell).  Wouldn’t it be simpler to just release the
context lock in drive_backup_prepare() before calling do_drive_backup()?
 The BDS is drained anyway.

On top of that, do_backup_common() calls bdrv_try_set_aio_context() to
bring the target into the source’s AioContext.  However, this function
must be called with the old AioContext held, and the new context not held.

Currently, it’s called exactly the other way around: With the new
context held, but the old one not held.

So I think it indeed actually makes more sense to release the AioContext
before calling do_drive_backup(), and to move the
bdrv_try_set_aio_context() call for target_bs to the callers of
do_backup_common() (where they have not yet taken the AioContext lock).

Max

> Signed-off-by: Sergio Lopez <address@hidden>
> ---
>  blockdev.c | 54 ++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index fbef6845c8..0cc6c69ceb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1762,8 +1762,10 @@ typedef struct DriveBackupState {
>      BlockJob *job;
>  } DriveBackupState;
>  
> -static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
> -                            Error **errp);
> +static BlockJob *do_drive_backup(DriveBackup *backup,
> +                                 BlockDriverState *backup_bs,
> +                                 JobTxn *txn,
> +                                 Error **errp);
>  
>  static void drive_backup_prepare(BlkActionState *common, Error **errp)
>  {
> @@ -1781,6 +1783,11 @@ static void drive_backup_prepare(BlkActionState 
> *common, Error **errp)
>          return;
>      }
>  
> +    if (!bs->drv) {
> +        error_setg(errp, "Device has no medium");
> +        return;
> +    }
> +
>      aio_context = bdrv_get_aio_context(bs);
>      aio_context_acquire(aio_context);
>  
> @@ -1789,7 +1796,9 @@ static void drive_backup_prepare(BlkActionState 
> *common, Error **errp)
>  
>      state->bs = bs;
>  
> -    state->job = do_drive_backup(backup, common->block_job_txn, &local_err);
> +    state->job = do_drive_backup(backup, bs,
> +                                 common->block_job_txn,
> +                                 &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          goto out;
> @@ -3607,7 +3616,9 @@ static BlockJob *do_backup_common(BackupCommon *backup,
>      return job;
>  }
>  
> -static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn,
> +static BlockJob *do_drive_backup(DriveBackup *backup,
> +                                 BlockDriverState *backup_bs,
> +                                 JobTxn *txn,
>                                   Error **errp)
>  {
>      BlockDriverState *bs;
> @@ -3625,18 +3636,27 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
> JobTxn *txn,
>          backup->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
>      }
>  
> -    bs = bdrv_lookup_bs(backup->device, backup->device, errp);
> -    if (!bs) {
> -        return NULL;
> -    }
> +    if (backup_bs) {
> +        bs = backup_bs;
> +        /*
> +         * If the caller passes us a BDS, we assume it has already
> +         * acquired the context lock.
> +         */
> +        aio_context = bdrv_get_aio_context(bs);
> +    } else {
> +        bs = bdrv_lookup_bs(backup->device, backup->device, errp);
> +        if (!bs) {
> +            return NULL;
> +        }
>  
> -    if (!bs->drv) {
> -        error_setg(errp, "Device has no medium");
> -        return NULL;
> -    }
> +        if (!bs->drv) {
> +            error_setg(errp, "Device has no medium");
> +            return NULL;
> +        }
>  
> -    aio_context = bdrv_get_aio_context(bs);
> -    aio_context_acquire(aio_context);
> +        aio_context = bdrv_get_aio_context(bs);
> +        aio_context_acquire(aio_context);
> +    }
>  
>      if (!backup->has_format) {
>          backup->format = backup->mode == NEW_IMAGE_MODE_EXISTING ?
> @@ -3713,7 +3733,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, 
> JobTxn *txn,
>  unref:
>      bdrv_unref(target_bs);
>  out:
> -    aio_context_release(aio_context);
> +    if (!backup_bs) {
> +        aio_context_release(aio_context);
> +    }
>      return job;
>  }
>  
> @@ -3721,7 +3743,7 @@ void qmp_drive_backup(DriveBackup *arg, Error **errp)
>  {
>  
>      BlockJob *job;
> -    job = do_drive_backup(arg, NULL, errp);
> +    job = do_drive_backup(arg, NULL, NULL, errp);
>      if (job) {
>          job_start(&job->job);
>      }
>

signature.asc
Description: OpenPGP digital signature