On Tue, Nov 03, 2020 at 08:49:44AM +0100, Christoph Hellwig wrote:
> On Mon, Nov 02, 2020 at 06:43:06PM +0000, Matthew Wilcox (Oracle) wrote:
> > Move the complicated condition and the calculations out of
> > filemap_update_page() into its own function.
>
> The logic looks good, but the flow inside of filemap_update_page looks
> odd. The patch below relative to this commit shows how I'd do it:
I have a simplification in mind that gets rid of the awkward 'first'
parameter. In filemap_get_pages(), do:
if ((iocb->ki_flags & IOCB_WAITQ) && (pagevec_count(pvec) > 1))
iocb->ki_flags |= IOCB_NOWAIT;
before calling filemap_update_page(). That matches what Kent did in
filemap_read():
if ((iocb->ki_flags & IOCB_WAITQ) && already_read)
iocb->ki_flags |= IOCB_NOWAIT;
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 81b569d818a3f8..304180c022d38a 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2266,40 +2266,39 @@ static int filemap_update_page(struct kiocb *iocb,
> if (!trylock_page(page)) {
> if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_NOIO))
> goto error;
> - if (iocb->ki_flags & IOCB_WAITQ) {
> - if (!first)
> - goto error;
> - error = __lock_page_async(page, iocb->ki_waitq);
> - if (error)
> - goto error;
> - } else {
> + if (!(iocb->ki_flags & IOCB_WAITQ)) {
> put_and_wait_on_page_locked(page, TASK_KILLABLE);
> return AOP_TRUNCATED_PAGE;
> }
> + if (!first)
> + goto error;
> + error = __lock_page_async(page, iocb->ki_waitq);
> + if (error)
> + goto error;
> }
I see where you're going there. OK.
>
> + error = AOP_TRUNCATED_PAGE;
> if (!page->mapping)
> - goto truncated;
> - if (filemap_range_uptodate(iocb, mapping, iter, page)) {
> - unlock_page(page);
> - return 0;
> - }
> + goto error_unlock_page;
>
> - if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
> - unlock_page(page);
> + if (!filemap_range_uptodate(iocb, mapping, iter, page)) {
> error = -EAGAIN;
> - } else {
> + if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ))
> + goto error_unlock_page;
> error = filemap_read_page(iocb->ki_filp, mapping, page);
> - if (!error)
> - return 0;
> + if (error)
> + goto error;
> + return 0; /* filemap_read_page unlocks the page */
> }
> +
> + unlock_page(page);
> + return 0;
> +
> +error_unlock_page:
> + unlock_page(page);
> error:
> put_page(page);
> return error;
> -truncated:
> - unlock_page(page);
> - put_page(page);
> - return AOP_TRUNCATED_PAGE;
> }
There's something niggling at me about this change ... I'll play with
it a bit.
