On 7/19/23 10:59?PM, Christoph Hellwig wrote:
>> + if (dio->flags & IOMAP_DIO_DEFER_COMP) {
>> + /* only polled IO cares about private cleared */
>> + iocb->private = dio;
>
> FYI, I find this comment a bit weird as it comments on what we're
> not doing in a path where it is irreleant. I'd rather only clear
> the private data in the path where polling is applicable and have
> a comment there why it is cleared. That probably belongs into the
> first patch restructuring the function.
OK, that makes sense.
>> @@ -277,12 +308,15 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>> * data IO that doesn't require any metadata updates (including
>> * after IO completion such as unwritten extent conversion) and
>> * the underlying device supports FUA. This allows us to avoid
>> - * cache flushes on IO completion.
>> + * cache flushes on IO completion. If we can't use FUA and
>> + * need to sync, disable in-task completions.
>
> ... because iomap_dio_complete will have to call generic_write_sync to
> do a blocking ->fsync call.
Will add to that comment.
>> @@ -308,6 +342,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>> pad = pos & (fs_block_size - 1);
>> if (pad)
>> iomap_dio_zero(iter, dio, pos - pad, pad);
>> +
>> + dio->flags &= ~IOMAP_DIO_DEFER_COMP;
>
> Why does zeroing disable the deferred completions? I can't really
> think of why, which is probably a strong indicator why this needs a
> comment.
If zerooout is set, then it's a new or unwritten extent and we need
further processing at write time which may trigger more IO. I'll add a
comment.
--
Jens Axboe
