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
>>