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

[Sergio Lopez]

Max Reitz <address@hidden> writes:

> On 13.09.19 11:37, Sergio Lopez wrote:
>> 
>> Max Reitz <address@hidden> 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.

signature.asc
Description: PGP signature