Re: [PATCH 1/3] block: eliminate BDRV_REQ_NO_SERIALISING

[Paolo Bonzini]

On 18/12/19 17:37, Kevin Wolf wrote:
>>       * passthrough flags.  */
>> -    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
>> -                       BDRV_REQ_PREFETCH)));
>> +    assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));
>>  
>>      /* Handle Copy on Read and associated serialisation */
>>      if (flags & BDRV_REQ_COPY_ON_READ) {
>> @@ -1458,12 +1457,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
>> *child,
>>          bdrv_mark_request_serialising(req, bdrv_get_cluster_size(bs));
>>      }
>>  
>> -    /* BDRV_REQ_SERIALISING is only for write operation */
>> -    assert(!(flags & BDRV_REQ_SERIALISING));
> I think we shoud still keep this assertion as long as read requests
> don't mark themselves as serialising when BDRV_REQ_SERIALISING is given.
> Otherwise, someone might add the flag to a read request and will later
> be surprised that it didn't work.

I'm removing it because it's anyway tested by the earlier

    assert(!(flags & ~(BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH)));

>> @@ -3222,9 +3214,7 @@ static int coroutine_fn bdrv_co_copy_range_internal(
>>  
>>          /* BDRV_REQ_SERIALISING is only for write operation */
>>          assert(!(read_flags & BDRV_REQ_SERIALISING));
> Here you kept the assertion, so apart from making sense anyway, it would
> also be more consistent to keep it above, too. :-)

... which is not present here.

Paolo