Re: [PATCH 1/2] block: fail a short atomic bounce in bio_iov_iter_bounce_write()

[Tal Zussman]

On 11/8/26 12:13 PM, John Garry wrote:
> On 11/8/26 04:24, Tal Zussman wrote:
>> bio_iov_iter_get_pages() fails a REQ_ATOMIC bio that doesn't cover the
>> whole iterator, since an atomic write is submitted as a single bio, and
>> submitting only part of it would tear the write. In contrast,
>> bio_iov_iter_bounce_write() doesn't. If a folio allocation fails partway
>> through, it returns a short bio, which for an atomic write triggers a
>> WARN_ON_ONCE() in iomap_dio_bio_iter_one().
>> 
>> Make bio_iov_iter_bounce_write() also check that a REQ_ATOMIC bio covers
>> the whole iterator, and clean up and return -EINVAL if it doesn't.
>> 
>> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
> 
> This looks reasonable, as atomic stable writes are implied to be
> supported from 3373503df025a ("xfs: use bounce buffering direct I/O when
> the device requires stable pages"). I am wondering how to test and prove
> this (change is good and maintained)?
> 

I tested this with XFS on scsi_debug (dif=1 dix=1 atomic_wr=1), doing
1M RWF_ATOMIC direct writes and using fail_page_alloc to make the
folio allocations in bio_alloc_bounce_folios() fail partway through.
I had an LLM make it pretty precise and target bio_alloc_bounce_folios()
using the stack filtering mechanism, so I don't think this necessarily
makes sense as an xfstest if that's what you were thinking...

Without this patch, the write returns -EINVAL and hits the
WARN_ON_ONCE() in iomap_dio_bio_iter_one(). With it, the write returns
-EINVAL without the warning.

This also happened to turn up a scsi_debug bug: with dif=1 dix=1 it
doesn't store the protection information for WRITE ATOMIC(16), so
reading back an atomic write fails. I'll send a separate patch for that.

Does that answer your question/is that what you were getting at?

Thanks,
Tal

>> ---
>>   block/bio.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>> 
>> diff --git a/block/bio.c b/block/bio.c
>> index b0fcc69de653..61a3812520a2 100644
>> --- a/block/bio.c
>> +++ b/block/bio.c
>> @@ -1523,6 +1523,16 @@ int bio_iov_iter_bounce_write(struct bio *bio, struct iov_iter *iter,
>>   		}
>>   	}
>>   
>> +	/*
>> +	 * An atomic write is submitted as a single bio, so it has to cover
>> +	 * the whole iterator or it would be torn.
>> +	 */
>> +	if ((bio->bi_opf & REQ_ATOMIC) && iov_iter_count(iter)) {
>> +		iov_iter_revert(iter, total_copied);
>> +		bio_free_folios(bio);
>> +		return -EINVAL;
>> +	}
>> +
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(bio_iov_iter_bounce_write);
>> 
>