On 11/29/22 08:35, Damien Le Moal wrote: > On 11/29/22 00:25, Christoph Hellwig wrote: >> On Sat, Nov 26, 2022 at 11:55:50AM +0900, Damien Le Moal wrote: >>> Taking a blktrace of a simple fio run on a block device file using >>> libaio and iodepth > 1 reveals that asynchronous writes are executed as >>> sync writes, that is, REQ_SYNC is set for the write BIOs. >>> >>> Fix this by modifying dio_bio_write_op() to set REQ_SYNC only for IOs >>> that are indeed synchronous ones and set REQ_IDLE only for asynchronous >>> IOs. >> >> Well, REQ_SYNC is used for I/O that some is actively waiting for, >> which includs aio/io_uring I/O unlike buffered writback. > > OK. But then the semantic of REQ_SYNC should really be more clearly > defined. Always setting it for bdev direct write IOs regardless of the > iocb type is telling me that we should not need it at all, especially > considering that for bdev direct IO reads, it is the reverse: REQ_SYNC > is never set. > >> So I don't think this should be changed. > > OK. But then what ? Looking again at the block layer use of REQ_SYNC, I > do not see anything super relevant. wbt_should_throttle() use it to > detect direct writes. Some other places set that flag but it does not > seem to actually be used/tested anywhere. What am I missing ? I missed that most of the "magic" is happening using tests done with op_is_sync(), which assumes that all reads are sync too. Got it. Back to the problem at hand though, bfq uses op_is_sync() to differentiate sync and async IOs to apply them to different queues. So always setting REQ_SYNC for direct writes seems wrong. -- Damien Le Moal Western Digital Research
