Re: [PATCH v3 02/12] block/io.c: drop assertion on double waiting for request serialisation

[Paolo Bonzini]

On 19/08/20 17:28, Stefan Hajnoczi wrote:
> On Mon, Aug 17, 2020 at 12:15:43PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> The comments states, that on misaligned request we should have already
>> been waiting. But for bdrv_padding_rmw_read, we called
>> bdrv_mark_request_serialising with align = request_alignment, and now
>> we serialise with align = cluster_size. So we may have to wait again
>> with larger alignment.
>>
>> Note, that the only user of BDRV_REQ_SERIALISING is backup which issues
>> cluster-aligned requests, so seems the assertion should not fire for
>> now. But it's wrong anyway.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  block/io.c | 11 +----------
>>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> This code was added by Paolo, CCing him.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

bdrv_mark_request_serialising already calls
bdrv_wait_serialising_requests_locked so it is okay.

Thanks,

Paolo

>> diff --git a/block/io.c b/block/io.c
>> index ad3a51ed53..b18680a842 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1881,7 +1881,6 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t 
>> offset, uint64_t bytes,
>>                            BdrvTrackedRequest *req, int flags)
>>  {
>>      BlockDriverState *bs = child->bs;
>> -    bool waited;
>>      int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
>>  
>>      if (bs->read_only) {
>> @@ -1893,15 +1892,7 @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t 
>> offset, uint64_t bytes,
>>      assert(!(flags & ~BDRV_REQ_MASK));
>>  
>>      if (flags & BDRV_REQ_SERIALISING) {
>> -        waited = bdrv_mark_request_serialising(req, 
>> bdrv_get_cluster_size(bs));
>> -        /*
>> -         * For a misaligned request we should have already waited earlier,
>> -         * because we come after bdrv_padding_rmw_read which must be called
>> -         * with the request already marked as serialising.
>> -         */
>> -        assert(!waited ||
>> -               (req->offset == req->overlap_offset &&
>> -                req->bytes == req->overlap_bytes));
>> +        bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
>>      } else {
>>          bdrv_wait_serialising_requests(req);
>>      }
>> -- 
>> 2.18.0
>>

signature.asc
Description: OpenPGP digital signature