| Message ID | 20221126025550.967914-3-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Minor fixes | expand |
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. So I don't think this should be changed.
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 ?
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.
diff --git a/block/fops.c b/block/fops.c index 50d245e8c913..5a4f57726828 100644 --- a/block/fops.c +++ b/block/fops.c @@ -34,7 +34,12 @@ static int blkdev_get_block(struct inode *inode, sector_t iblock, static blk_opf_t dio_bio_write_op(struct kiocb *iocb) { - blk_opf_t opf = REQ_OP_WRITE | REQ_SYNC | REQ_IDLE; + blk_opf_t opf = REQ_OP_WRITE; + + if (is_sync_kiocb(iocb)) + opf |= REQ_SYNC; + else + opf |= REQ_IDLE; /* avoid the need for a I/O completion work item */ if (iocb_is_dsync(iocb))
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. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- block/fops.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)