Re: [PATCH v2(RFC) 0/3] qcow2: fix parallel rewrite and discard

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

12.03.2021 18:24, Kevin Wolf wrote:
> Am 25.02.2021 um 12:52 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Hi all! It occurs that nothing prevents discarding and reallocating host
>> cluster during data writing. This way data writing will pollute another
>> newly allocated cluster of data or metadata.
>>
>> OK, v2 is a try to solve the problem with CoRwlock.. And it is marked
>> RFC, because of a lot of iotest failures.. Some of problems with v2:
>>
>> 1. It's a more complicated to make a test, as everything is blocking
>> and I can't just break write and do discard.. I have to implement
>> aio_discard in qemu-io and rewrite test into several portions of io
>> commands splitted by "sleep 1".. OK, it's not a big problem, and I've
>> solved it.
> 
> Right, this just demonstrates that it's doing what it's supposed to.
> 
>> 2. iotest 7 fails with several leaked clusters. Seems, that it depend on
>> the fact that discard may be done in parallel with writes. Iotest 7 does
>> snapshots, so I think l1 table is updated to the moment when discard is
>> finally unlocked.. But I didn't dig into it, it's all my assumptions.
> 
> This one looks a bit odd, but it may be related to the bug in your code
> that you forgot that qcow2_cluster_discard() is not a coroutine_fn.
> Later tests fail during the unlock:
> 
> qemu-img: ../util/qemu-coroutine-lock.c:358: qemu_co_rwlock_unlock: Assertion `qemu_in_coroutine()' failed.
> 
> #0  0x00007ff33f7d89d5 in raise () from /lib64/libc.so.6
> #1  0x00007ff33f7c18a4 in abort () from /lib64/libc.so.6
> #2  0x00007ff33f7c1789 in __assert_fail_base.cold () from /lib64/libc.so.6
> #3  0x00007ff33f7d1026 in __assert_fail () from /lib64/libc.so.6
> #4  0x0000556f4ffd3c94 in qemu_co_rwlock_unlock (lock=0x556f51f63ca0) at ../util/qemu-coroutine-lock.c:358
> #5  0x0000556f4fef5e09 in qcow2_cluster_discard (bs=0x556f51f56a80, offset=37748736, bytes=0, type=QCOW2_DISCARD_NEVER, full_discard=false) at ../block/qcow2-cluster.c:1970
> #6  0x0000556f4ff46c23 in qcow2_snapshot_create (bs=0x556f51f56a80, sn_info=0x7fff89fb9a30) at ../block/qcow2-snapshot.c:736
> #7  0x0000556f4ff0d7b6 in bdrv_snapshot_create (bs=0x556f51f56a80, sn_info=0x7fff89fb9a30) at ../block/snapshot.c:227
> #8  0x0000556f4fe85526 in img_snapshot (argc=4, argv=0x7fff89fb9d30) at ../qemu-img.c:3337
> #9  0x0000556f4fe8a227 in main (argc=4, argv=0x7fff89fb9d30) at ../qemu-img.c:5375
> 
>> 3. iotest 13 (and I think a lot more iotests) crashes on
>> assert(!to->locks_held); .. So with this assertion we can't keep rwlock
>> locked during data writing...
>>
>>    #3  in __assert_fail () from /lib64/libc.so.6
>>    #4  in qemu_aio_coroutine_enter (ctx=0x55762120b700, co=0x55762121d700)
>>        at ../util/qemu-coroutine.c:158
>>    #5  in aio_co_enter (ctx=0x55762120b700, co=0x55762121d700) at ../util/async.c:628
>>    #6  in aio_co_wake (co=0x55762121d700) at ../util/async.c:612
>>    #7  in thread_pool_co_cb (opaque=0x7f17950daab0, ret=0) at ../util/thread-pool.c:279
>>    #8  in thread_pool_completion_bh (opaque=0x5576211e5070) at ../util/thread-pool.c:188
>>    #9  in aio_bh_call (bh=0x557621205df0) at ../util/async.c:136
>>    #10 in aio_bh_poll (ctx=0x55762120b700) at ../util/async.c:164
>>    #11 in aio_poll (ctx=0x55762120b700, blocking=true) at ../util/aio-posix.c:659
>>    #12 in blk_prw (blk=0x557621205790, offset=4303351808,
>>        buf=0x55762123e000 '\364' <repeats 199 times>, <incomplete sequence \364>..., bytes=12288,
>>        co_entry=0x557620d9dc97 <blk_write_entry>, flags=0) at ../block/block-backend.c:1335
>>    #13 in blk_pwrite (blk=0x557621205790, offset=4303351808, buf=0x55762123e000,
>>        count=12288, flags=0) at ../block/block-backend.c:1501
> 
> This is another bug in your code: A taken lock belongs to its coroutine.
> You can't lock in one coroutine and unlock in another.
> 
> The changes you made to the write code seem unnecessarily complicated
> anyway: Why not just qemu_co_rwlock_rdlock() right at the start of
> qcow2_co_pwritev_part() and unlock at its end, without taking and
> dropping the lock repeatedly? It makes both the locking more obviously
> correct and also fixes the bug (013 passes with this change).
> 
>> So now I think that v1 is simpler.. It's more complicated (but not too
>> much) in code. But it keeps discards and data writes non-blocking each
>> other and avoids yields in critical sections.
> 
> I think an approach with additional data structures is almost certainly
> more complex and harder to maintain (and as the review from Max showed,
> also to understand). I wouldn't give up yet on the CoRwlock based
> approach, it's almost trivial code in comparison.

Sure

> 
> True, making qcow2_cluster_discard() a coroutine_fn requires a
> preparational patch that is less trivial, but at least this can be seen
> as something that we would want to do sooner or later anyway.
> 

Thanks for help, I'll try your suggestions and resend.

-- 
Best regards,
Vladimir