mm: mark async iocb read as NOWAIT once some data has been, copied
Subsystems (2)
PATCH v3 1/1 mm: mark async iocb read as NOWAIT once some data has been, copied
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.
---
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(+)
mm/filemap.c +8 -0
@@ -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;
Signature · 4 lines
2.28.0 -- Jens Axboe
Quoted 2 lines · Jens Axboe
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.
Quoted 2 lines
Instead make sure we return the copied amount, and let the caller retry, instead of returning
-EIOCBQUEUEDfor 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.
mm/filemap.c ssize_t generic_file_buffered_read(struct kiocb *iocb, · line 2208
Quoted 11 lines
mm/filemap.c +7
-0@@ -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.