[Devel] [PATCH vz9] dm-qcow2: only use inline bvec if iteration is valid

[Alexander Atanasov]

On 30.12.24 10:49, Pavel Tikhomirov wrote:
> Looks good. See small comment inline:
> 
> On 12/17/24 18:34, Alexander Atanasov wrote:
>> In some cases qcow2 is passed requests without bvec set and
>> zero bi_size - for such requests qcow2 resorts to inline bvecs
>> which in that case are invalid.
>>
>> To address this:
>> - Check if bi_size is not zero, and only then use the inline bvecs.
>> - Properly count number of segments in a NULL bvec.
>> - Add warn on to catch if we have a requests without bvec and bi_size.
>>
>> https://virtuozzo.atlassian.net/browse/PSBM-159903
>> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
>> ---
>>   drivers/md/dm-qcow2-map.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/dm-qcow2-map.c b/drivers/md/dm-qcow2-map.c
>> index 6585f3fac6e7..a28f6332dc08 100644
>> --- a/drivers/md/dm-qcow2-map.c
>> +++ b/drivers/md/dm-qcow2-map.c
>> @@ -308,6 +308,9 @@ static unsigned int qio_nr_segs(struct qio *qio)
>>       struct bvec_iter iter;
>>       struct bio_vec bv;
>> +    if (!qio->bi_io_vec)
>> +        return nr_segs;
> 
> maybe just "return 0" (to improve readability) ?


I will let that to Kostya to decide. nr_segs is what the function is 
returning and it is initiallized to zero 4 lines before that return,
it is not visible in the diff thou. I find it better to use a consistent 
return inside a function if it is returning a variable, i.e. return the 
what we have computed so far. Not for particular case but in a larger 
function it is easy to miss to update such place when new calculation is 
performed, this is why i prefer it that way but i don't mind either.

> 
>> +
>>       qcow2_for_each_bvec(iter, bv, qio->bi_iter, qio->bi_io_vec)
>>           nr_segs++;
>> @@ -1248,6 +1251,7 @@ static void __submit_rw_mapped(struct qcow2 
>> *qcow2, struct qio *qio, u32 nr_segs
>>       bvec = __bvec_iter_bvec(qio->bi_io_vec, qio->bi_iter);
>>       pos = to_bytes(qio->bi_iter.bi_sector);
>> +    WARN_ON(qio->bi_iter.bi_size && !bvec);
>>       iov_iter_bvec(&iter, rw, bvec, nr_segs, qio->bi_iter.bi_size);
>>       iter.iov_offset = qio->bi_iter.bi_bvec_done;
>> @@ -3538,7 +3542,7 @@ static void prepare_one_embedded_qio(struct 
>> qcow2 *qcow2, struct qio *qio,
>>       } else {
>>           /* Single bio already provides bvec array */
>>           bvec = rq->bio->bi_io_vec;
>> -        if (!bvec) {
>> +        if (!bvec && rq->bio->bi_iter.bi_size) {
>>               bvec = rq->bio->bi_inline_vecs;
>>               if (unlikely(!bvec)) {
>>                   QC_ERR(qcow2->tgt->ti, "Expecting inline bvec");
> 

-- 
Regards,
Alexander Atanasov