Hi,
For v1, see:
https://lore.kernel.org/linux-fsdevel/20210208221829.17247-1-axboe@kernel.dk/
tldr; don't -EAGAIN IOCB_NOWAIT dio reads just because we have page cache
entries for the given range. This causes unnecessary work from the callers
side, when the IO could have been issued totally fine without blocking on
writeback when there is none.
fs/iomap/direct-io.c | 23 ++++++++++++++--------
include/linux/fs.h | 2 ++
mm/filemap.c | 47 ++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 62 insertions(+), 10 deletions(-)
Since v1:
- Simplify the filemap_range_needs_writeback() loop (Willy)
- Drop the write side (Chinner)
--
Jens Axboe
For O_DIRECT reads/writes, we check if we need to issue a call to
filemap_write_and_wait_range() to issue and/or wait for writeback for any
page in the given range. The existing mechanism just checks for a page in
the range, which is suboptimal for IOCB_NOWAIT as we'll fallback to the
slow path (and needing retry) if there's just a clean page cache page in
the range.
Provide filemap_range_needs_writeback() which tries a little harder to
check if we actually need to issue and/or wait for writeback in the
range.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
include/linux/fs.h | 2 ++
mm/filemap.c | 43 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
For the generic page cache read helper, use the better variant of checking
for the need to call filemap_write_and_wait_range() when doing O_DIRECT
reads. This avoids falling back to the slow path for IOCB_NOWAIT, if there
are no pages to wait for (or write out).
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
mm/filemap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Christoph Hellwig <hch@infradead.org> Date: 2021-02-09 07:49:25
On Mon, Feb 08, 2021 at 07:30:07PM -0700, Jens Axboe wrote:
quoted hunk
For the generic page cache read helper, use the better variant of checking
for the need to call filemap_write_and_wait_range() when doing O_DIRECT
reads. This avoids falling back to the slow path for IOCB_NOWAIT, if there
are no pages to wait for (or write out).
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
mm/filemap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
On Mon, Feb 08, 2021 at 07:30:07PM -0700, Jens Axboe wrote:
quoted
For the generic page cache read helper, use the better variant of checking
for the need to call filemap_write_and_wait_range() when doing O_DIRECT
reads. This avoids falling back to the slow path for IOCB_NOWAIT, if there
are no pages to wait for (or write out).
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
mm/filemap.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
For reads, use the better variant of checking for the need to call
filemap_write_and_wait_range() when doing O_DIRECT. This avoids falling
back to the slow path for IOCB_NOWAIT, if there are no pages to wait for
(or write out).
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/iomap/direct-io.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
looking at this I really hate the scheme with the potential racyness
and duplicated page looksups.
Why can't we pass a nonblock flag to filemap_write_and_wait_range
and invalidate_inode_pages2_range that makes them return -EAGAIN
when they would block to clean this whole mess up?
looking at this I really hate the scheme with the potential racyness
and duplicated page looksups.
Me too
Why can't we pass a nonblock flag to filemap_write_and_wait_range
and invalidate_inode_pages2_range that makes them return -EAGAIN
when they would block to clean this whole mess up?
We could, but that's a _lot_ of surgery. I'd rather live with the
slight race for now instead of teaching writepages, page laundering,
etc about IOCB_NOWAIT.
I do think that's a worthy long term goal, but we dio read situation
is bad enough that it warrants a quicker fix.
--
Jens Axboe
From: Andrew Morton <akpm@linux-foundation.org> Date: 2021-02-09 21:21:45
On Mon, 8 Feb 2021 19:30:05 -0700 Jens Axboe [off-list ref] wrote:
Hi,
For v1, see:
https://lore.kernel.org/linux-fsdevel/20210208221829.17247-1-axboe@kernel.dk/
tldr; don't -EAGAIN IOCB_NOWAIT dio reads just because we have page cache
entries for the given range. This causes unnecessary work from the callers
side, when the IO could have been issued totally fine without blocking on
writeback when there is none.
Seems a good idea. Obviously we'll do more work in the case where some
writeback needs doing, but we'll be doing synchronous writeout in that
case anyway so who cares.
Please remind me what prevents pages from becoming dirty during or
immediately after the filemap_range_needs_writeback() check? Perhaps
filemap_range_needs_writeback() could have a comment explaining what it
is that keeps its return value true after it has returned it!
On Mon, 8 Feb 2021 19:30:05 -0700 Jens Axboe [off-list ref] wrote:
quoted
Hi,
For v1, see:
https://lore.kernel.org/linux-fsdevel/20210208221829.17247-1-axboe@kernel.dk/
tldr; don't -EAGAIN IOCB_NOWAIT dio reads just because we have page cache
entries for the given range. This causes unnecessary work from the callers
side, when the IO could have been issued totally fine without blocking on
writeback when there is none.
Seems a good idea. Obviously we'll do more work in the case where some
writeback needs doing, but we'll be doing synchronous writeout in that
case anyway so who cares.
Right, I think that'll be a round two on top of this, so we can make the
write side happier too. That's a bit more involved...
Please remind me what prevents pages from becoming dirty during or
immediately after the filemap_range_needs_writeback() check? Perhaps
filemap_range_needs_writeback() could have a comment explaining what it
is that keeps its return value true after it has returned it!
It's inherently racy, just like it is now. There's really no difference
there, and I don't think there's a way to close that. Even if you
modified filemap_write_and_wait_range() to be non-block friendly,
there's nothing stopping anyone from adding dirty page cache right after
that call.
--
Jens Axboe
On Tue, Feb 9, 2021 at 10:25 PM Jens Axboe [off-list ref] wrote:
On 2/9/21 12:55 PM, Andrew Morton wrote:
quoted
On Mon, 8 Feb 2021 19:30:05 -0700 Jens Axboe [off-list ref] wrote:
quoted
Hi,
For v1, see:
https://lore.kernel.org/linux-fsdevel/20210208221829.17247-1-axboe@kernel.dk/
tldr; don't -EAGAIN IOCB_NOWAIT dio reads just because we have page cache
entries for the given range. This causes unnecessary work from the callers
side, when the IO could have been issued totally fine without blocking on
writeback when there is none.
Seems a good idea. Obviously we'll do more work in the case where some
writeback needs doing, but we'll be doing synchronous writeout in that
case anyway so who cares.
Right, I think that'll be a round two on top of this, so we can make the
write side happier too. That's a bit more involved...
quoted
Please remind me what prevents pages from becoming dirty during or
immediately after the filemap_range_needs_writeback() check? Perhaps
filemap_range_needs_writeback() could have a comment explaining what it
is that keeps its return value true after it has returned it!
It's inherently racy, just like it is now. There's really no difference
there, and I don't think there's a way to close that. Even if you
modified filemap_write_and_wait_range() to be non-block friendly,
there's nothing stopping anyone from adding dirty page cache right after
that call.
Jens, do you have some numbers before and after your patchset is applied?
And kindly a test "profile" for FIO :-)?
Thanks.
- Sedat -
On Tue, Feb 9, 2021 at 10:25 PM Jens Axboe [off-list ref] wrote:
quoted
On 2/9/21 12:55 PM, Andrew Morton wrote:
quoted
On Mon, 8 Feb 2021 19:30:05 -0700 Jens Axboe [off-list ref] wrote:
quoted
Hi,
For v1, see:
https://lore.kernel.org/linux-fsdevel/20210208221829.17247-1-axboe@kernel.dk/
tldr; don't -EAGAIN IOCB_NOWAIT dio reads just because we have page cache
entries for the given range. This causes unnecessary work from the callers
side, when the IO could have been issued totally fine without blocking on
writeback when there is none.
Seems a good idea. Obviously we'll do more work in the case where some
writeback needs doing, but we'll be doing synchronous writeout in that
case anyway so who cares.
Right, I think that'll be a round two on top of this, so we can make the
write side happier too. That's a bit more involved...
quoted
Please remind me what prevents pages from becoming dirty during or
immediately after the filemap_range_needs_writeback() check? Perhaps
filemap_range_needs_writeback() could have a comment explaining what it
is that keeps its return value true after it has returned it!
It's inherently racy, just like it is now. There's really no difference
there, and I don't think there's a way to close that. Even if you
modified filemap_write_and_wait_range() to be non-block friendly,
there's nothing stopping anyone from adding dirty page cache right after
that call.
Jens, do you have some numbers before and after your patchset is applied?
I don't, the load was pretty light for the test case - it was just doing
33-34K of O_DIRECT 4k random reads in a pretty small range of the device.
When you end up having page cache in that range, that means you end up
punting a LOT of requests to the async worker. So it wasn't as much a
performance win for this particular case, but an efficiency win. You get
rid of a worker using 40% CPU, and reduce the latencies.
And kindly a test "profile" for FIO :-)?
To reproduce this, have a small range dio rand reads and then have
something else that does a few buffered reads from the same range.
--
Jens Axboe