Re: [Qemu-devel] [PATCH v9 03/13] block/dirty-bitmap: add _locked version of bdrv_reclaim_dirty_bitmap

[John Snow <jsnow@redhat.com>]

On 01/22/2018 07:14 AM, Vladimir Sementsov-Ogievskiy wrote:
> 19.01.2018 17:12, Vladimir Sementsov-Ogievskiy wrote:
>> 18.01.2018 13:09, Paolo Bonzini wrote:
>>> On 18/01/2018 10:55, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Most functions that looks at the list are "called with BQL taken".
>>>>> Functions that write to the list are "called with BQL taken" and call
>>>>> bdrv_dirty_bitmaps_lock/bdrv_dirty_bitmaps_unlock themselves.
>>>> Paolo, could you please explain about bitmap locking in more details?
>>>> Why do we need mutexes?
>>> We have three cases:
>>>
>>> 1) monitor creates and destroy bitmaps.
>>>
>>> 2) monitor also has to read the list.  We know it operates with BQL.
>>>
>>> 3) users such as mirror.c create a dirty bitmap in the monitor command
>>> (under BQL), but they can operate without BQL in a separate iothread so
>>> we create a separate lock (bitmap->mutex).
>>>
>>> While in the second and third case, bitmaps cannot disappear. So in the
>>> first case you operate with BQL+dirty bitmap mutex.  The result is that
>>> you lock out both the second and the third case while creating and
>>> destroying bitmaps.
>>>
>>>> Why do we do not need them
>>>> on read from the bitmap, only on write?
>>> Indeed, reading the bitmap also requires taking the lock.  So
>>> s/Modifying/Accessing/ in that comment.
>>>
>>> Paolo
>>
>> so,
>>
>>     /* Writing to the list requires the BQL_and_  the dirty_bitmap_mutex.
>>      * Reading from the list can be done with either the BQL or the
>>      * dirty_bitmap_mutex.  Accessing a bitmap only requires
>>      * dirty_bitmap_mutex. */
>>     QemuMutex dirty_bitmap_mutex;
>>
>>
>>
>> so, accessing the bitmap needs mutex lock?
>>
>> Then what do you mean under accessing the bitmap? Any touch of
>> BdrvDirtyBitmap fields? Then "reading the list" will require bitmap
>> mutex too.
>> Or accessing the bitmap is accessing any field except
>> BdrvDirtyBitmap.list? Then in (2), what do you mean? For example
>> query-block will go through
>> the list, but it touches other fields too, so it should lock mutex.
>>
> 
> and one more question:
> 
> What about qmp transactions? Should we lock mutex during the whole
> transaction?
> 

For bitmaps?

hmm...

at the moment, Transactions still do the tepid bdrv_drain_all() prior to
the transaction, and then I suspect they rely on the QMP context holding
the big lock to prevent any new IO occurring.

It should be a quiescent point, I think, but I've lost track of how
exactly it behaves presently. Didn't we need a
bdrv_drained_all_begin/_end pair here? Did we not do that? I forget why...

(...is it related to how we don't know how to implement this in a
context where graph changes might occur, which can happen during a
transaction?)

it might not be possible to grab the bitmap locks during .prepare and
release them in .cleanup, because we might want to add two actions to
the same transaction that operate on the same bitmap (full backup +
clear, for instance?) and they'll deadlock against each other.

It might be sufficient to just lock and release per each action, until
the deeper issues with transactions are resolved. If the transaction is
properly quiescent, you shouldn't run into any bitmap inconsistency
problems anyway.

Hoping Kevin and Paolo can chime in to remind me of the details, here.