linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] mm: mark async iocb read as NOWAIT once some data has been, copied
@ 2020-10-17 20:07 Jens Axboe
  2020-10-20 19:03 ` Johannes Weiner
  0 siblings, 1 reply; 2+ messages in thread
From: Jens Axboe @ 2020-10-17 20:07 UTC (permalink / raw)
  To: linux-mm, Andrew Morton, Johannes Weiner, Kent Overstreet,
	Matthew Wilcox

Once we've copied some data for an iocb that is marked with IOCB_WAITQ,
we should no longer attempt to async lock a new page. Instead make sure
we return the copied amount, and let the caller retry, instead of
returning -EIOCBQUEUED for a new page.

This should only be possible with read-ahead disabled on the below
device, and multiple threads racing on the same file. Haven't been able
to reproduce on anything else.

Cc: stable@vger.kernel.org # v5.9
Fixes: 1a0a7853b901 ("mm: support async buffered reads in generic_file_buffered_read()")
Reported-by: Kent Overstreet <kent.overstreet@gmail.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---

V3
- Place the 'written' check up top so we catch cases where 'written' is
  already passed in as non-zero (Kent)

 mm/filemap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mm/filemap.c b/mm/filemap.c
index 1a6beaf69f49..e4101b5bfa82 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2199,6 +2199,14 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
 	last_index = (*ppos + iter->count + PAGE_SIZE-1) >> PAGE_SHIFT;
 	offset = *ppos & ~PAGE_MASK;
 
+	/*
+	 * If we've already successfully copied some data, then we
+	 * can no longer safely return -EIOCBQUEUED. Hence mark
+	 * an async read NOWAIT at that point.
+	 */
+	if (written && (iocb->ki_flags & IOCB_WAITQ))
+		iocb->ki_flags |= IOCB_NOWAIT;
+
 	for (;;) {
 		struct page *page;
 		pgoff_t end_index;
-- 
2.28.0

-- 
Jens Axboe



^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v3] mm: mark async iocb read as NOWAIT once some data has been, copied
  2020-10-17 20:07 [PATCH v3] mm: mark async iocb read as NOWAIT once some data has been, copied Jens Axboe
@ 2020-10-20 19:03 ` Johannes Weiner
  0 siblings, 0 replies; 2+ messages in thread
From: Johannes Weiner @ 2020-10-20 19:03 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-mm, Andrew Morton, Kent Overstreet, Matthew Wilcox

On Sat, Oct 17, 2020 at 02:07:26PM -0600, Jens Axboe wrote:
> Once we've copied some data for an iocb that is marked with IOCB_WAITQ,
> we should no longer attempt to async lock a new page.

It could be useful to elaborate on the (user-visible) failure scenario
here a bit, as I don't think it's obvious.

> Instead make sure we return the copied amount, and let the caller
> retry, instead of returning -EIOCBQUEUED for a new page.

We *wouldn't* return -EIOCBQUEUED, though, would we? We'd do the async
path, put the caller on a waitqueue, but then return `written' instead
of letting it know.

> @@ -2199,6 +2199,14 @@ ssize_t generic_file_buffered_read(struct kiocb *iocb,
>  	last_index = (*ppos + iter->count + PAGE_SIZE-1) >> PAGE_SHIFT;
>  	offset = *ppos & ~PAGE_MASK;
>  
> +	/*
> +	 * If we've already successfully copied some data, then we
> +	 * can no longer safely return -EIOCBQUEUED. Hence mark
> +	 * an async read NOWAIT at that point.
> +	 */
> +	if (written && (iocb->ki_flags & IOCB_WAITQ))
> +		iocb->ki_flags |= IOCB_NOWAIT;

That looks correct to me, FWIW. It took a second to verify with all
spaghetti in this function :-) But the ra/!uptodate path already has
its own guard, and this is needed for the readpage fallback.


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2020-10-20 19:05 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-17 20:07 [PATCH v3] mm: mark async iocb read as NOWAIT once some data has been, copied Jens Axboe
2020-10-20 19:03 ` Johannes Weiner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).