| Message ID | 20190912161624.40886-1-slp@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | blockdev: avoid acquiring AioContext lock twice at do_drive_backup() | expand |
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 <slp@redhat.com> > --- > 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); > } >
Max Reitz <mreitz@redhat.com> writes: > 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. Redundant calls rarely hurt, they're just redundant ;-) > 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. Is this documented somewhere? I see nothing in the function declaration nor definition. I'm starting to get the feeling that the block layer is riddled with unwritten rules and assumptions that makes every change a lot harder than it should be. > 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). OK. I see this also happens in external_snapshot_prepare() and qmp_drive_mirror() too. I guess we should fix these too. In qmp_drive_mirror(), would it be safe to delay the acquisition of any context until just before the blockdev_mirror_common()? > Max > >> Signed-off-by: Sergio Lopez <slp@redhat.com> >> --- >> 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); >> } >>
On 13.09.19 11:37, Sergio Lopez wrote: > > Max Reitz <mreitz@redhat.com> writes: > >> 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. > > Redundant calls rarely hurt, they're just redundant ;-) If they’re expensive and in a hot path, they hurt. >> 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. > > Is this documented somewhere? I see nothing in the function declaration > nor definition. > > I'm starting to get the feeling that the block layer is riddled with > unwritten rules and assumptions that makes every change a lot harder > than it should be. It is written, it’s just that it’s written in bdrv_set_aio_context_ignore()’s definition. Yes, we should document it directly for bdrv_try_set_aio_context(), too, because that’s what external callers are much more likely to use. >> 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). > > OK. I see this also happens in external_snapshot_prepare() and > qmp_drive_mirror() too. I guess we should fix these too. > > In qmp_drive_mirror(), would it be safe to delay the acquisition of any > context until just before the blockdev_mirror_common()? From mirror’s perspective I think so, but I don’t think it’s safe to access any of a BDS’s fields without having acquired its AioContext. (In fact, I wonder whether we should acquire the context even before bdrv_op_is_blocked()...) Max
Max Reitz <mreitz@redhat.com> writes: > On 13.09.19 11:37, Sergio Lopez wrote: >> >> Max Reitz <mreitz@redhat.com> writes: >> >>> 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. >> >> Redundant calls rarely hurt, they're just redundant ;-) > > If they’re expensive and in a hot path, they hurt. > >>> 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. >> >> Is this documented somewhere? I see nothing in the function declaration >> nor definition. >> >> I'm starting to get the feeling that the block layer is riddled with >> unwritten rules and assumptions that makes every change a lot harder >> than it should be. > > It is written, it’s just that it’s written in > bdrv_set_aio_context_ignore()’s definition. > > Yes, we should document it directly for bdrv_try_set_aio_context(), too, > because that’s what external callers are much more likely to use. > >>> 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). >> >> OK. I see this also happens in external_snapshot_prepare() and >> qmp_drive_mirror() too. I guess we should fix these too. >> >> In qmp_drive_mirror(), would it be safe to delay the acquisition of any >> context until just before the blockdev_mirror_common()? > > From mirror’s perspective I think so, but I don’t think it’s safe to > access any of a BDS’s fields without having acquired its AioContext. > (In fact, I wonder whether we should acquire the context even before > bdrv_op_is_blocked()...) In that case, I wonder if we can safely release the context to honor bdrv_try_set_aio_context() requirements, knowing we aren't in a drained section. Sergio.
On 13.09.19 12:15, Sergio Lopez wrote: > > Max Reitz <mreitz@redhat.com> writes: > >> On 13.09.19 11:37, Sergio Lopez wrote: >>> >>> Max Reitz <mreitz@redhat.com> writes: >>> >>>> 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. >>> >>> Redundant calls rarely hurt, they're just redundant ;-) >> >> If they’re expensive and in a hot path, they hurt. >> >>>> 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. >>> >>> Is this documented somewhere? I see nothing in the function declaration >>> nor definition. >>> >>> I'm starting to get the feeling that the block layer is riddled with >>> unwritten rules and assumptions that makes every change a lot harder >>> than it should be. >> >> It is written, it’s just that it’s written in >> bdrv_set_aio_context_ignore()’s definition. >> >> Yes, we should document it directly for bdrv_try_set_aio_context(), too, >> because that’s what external callers are much more likely to use. >> >>>> 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). >>> >>> OK. I see this also happens in external_snapshot_prepare() and >>> qmp_drive_mirror() too. I guess we should fix these too. >>> >>> In qmp_drive_mirror(), would it be safe to delay the acquisition of any >>> context until just before the blockdev_mirror_common()? >> >> From mirror’s perspective I think so, but I don’t think it’s safe to >> access any of a BDS’s fields without having acquired its AioContext. >> (In fact, I wonder whether we should acquire the context even before >> bdrv_op_is_blocked()...) > > In that case, I wonder if we can safely release the context to honor > bdrv_try_set_aio_context() requirements, knowing we aren't in a drained > section. Hm. I suppose it depends. From the main context, it is OK to access all fields that only the main context accesses. So bdrv_try_set_aio_context() should be safe because they do that. I suppose the same goes for bdrv_op_is_blocked(), because op blockers are only set up or removed in the main context. So it should be safe as it is after all. Max
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); }
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. Signed-off-by: Sergio Lopez <slp@redhat.com> --- blockdev.c | 54 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 16 deletions(-)