Re: [PATCH 04/17] mm/filemap: Support readpage splitting a page

[Kent Overstreet <kent.overstreet@gmail.com>]

On Mon, Nov 02, 2020 at 06:42:59PM +0000, Matthew Wilcox (Oracle) wrote:
> For page splitting to succeed, the thread asking to split the
> page has to be the only one with a reference to the page.  Calling
> wait_on_page_locked() while holding a reference to the page will
> effectively prevent this from happening with sufficient threads waiting
> on the same page.  Use put_and_wait_on_page_locked() to sleep without
> holding a reference to the page, then retry the page lookup after the
> page is unlocked.
> 
> Since we now get the page lock a little earlier in filemap_update_page(),
> we can eliminate a number of duplicate checks.  The original intent
> (commit ebded02788b5 ("avoid unnecessary calls to lock_page when waiting
> for IO to complete during a read")) behind getting the page lock later
> was to avoid re-locking the page after it has been brought uptodate by
> another thread.  We still avoid that because we go through the normal
> lookup path again after the winning thread has brought the page uptodate.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/filemap.c | 76 ++++++++++++++++------------------------------------
>  1 file changed, 23 insertions(+), 53 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 709774a60379..550e023abb52 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1347,14 +1347,6 @@ static int __wait_on_page_locked_async(struct page *page,
>  	return ret;
>  }
>  
> -static int wait_on_page_locked_async(struct page *page,
> -				     struct wait_page_queue *wait)
> -{
> -	if (!PageLocked(page))
> -		return 0;
> -	return __wait_on_page_locked_async(compound_head(page), wait, false);
> -}
> -
>  /**
>   * put_and_wait_on_page_locked - Drop a reference and wait for it to be unlocked
>   * @page: The page to wait for.
> @@ -2281,64 +2273,42 @@ static struct page *filemap_update_page(struct kiocb *iocb, struct file *filp,
>  	struct inode *inode = mapping->host;
>  	int error;
>  
> -	/*
> -	 * See comment in do_read_cache_page on why
> -	 * wait_on_page_locked is used to avoid unnecessarily
> -	 * serialisations and why it's safe.
> -	 */
>  	if (iocb->ki_flags & IOCB_WAITQ) {
> -		error = wait_on_page_locked_async(page,
> -						iocb->ki_waitq);
> +		error = lock_page_async(page, iocb->ki_waitq);
> +		if (error) {
> +			put_page(page);
> +			return ERR_PTR(error);
> +		}
>  	} else {
> -		error = wait_on_page_locked_killable(page);
> -	}
> -	if (unlikely(error)) {
> -		put_page(page);
> -		return ERR_PTR(error);
> +		if (!trylock_page(page)) {
> +			put_and_wait_on_page_locked(page, TASK_KILLABLE);
> +			return NULL;
> +		}
>  	}
> -	if (PageUptodate(page))
> -		return page;
>  
> +	if (!page->mapping)
> +		goto truncated;

Since we're dropping our ref to the page, it could potentially be truncated and
then reused, no? So we should be checking page->mapping == mapping &&
page->index == index (and stashing page->index before dropping our ref, or
passing it in).

> +	if (PageUptodate(page))
> +		goto uptodate;
>  	if (inode->i_blkbits == PAGE_SHIFT ||
>  			!mapping->a_ops->is_partially_uptodate)
> -		goto page_not_up_to_date;
> +		goto readpage;
>  	/* pipes can't handle partially uptodate pages */
>  	if (unlikely(iov_iter_is_pipe(iter)))
> -		goto page_not_up_to_date;
> -	if (!trylock_page(page))
> -		goto page_not_up_to_date;
> -	/* Did it get truncated before we got the lock? */
> -	if (!page->mapping)
> -		goto page_not_up_to_date_locked;
> +		goto readpage;
>  	if (!mapping->a_ops->is_partially_uptodate(page,
> -				pos & ~PAGE_MASK, count))
> -		goto page_not_up_to_date_locked;
> +				pos & (thp_size(page) - 1), count))
> +		goto readpage;
> +uptodate:
>  	unlock_page(page);
>  	return page;
>  
> -page_not_up_to_date:
> -	/* Get exclusive access to the page ... */
> -	error = lock_page_for_iocb(iocb, page);
> -	if (unlikely(error)) {
> -		put_page(page);
> -		return ERR_PTR(error);
> -	}
> -
> -page_not_up_to_date_locked:
> -	/* Did it get truncated before we got the lock? */
> -	if (!page->mapping) {
> -		unlock_page(page);
> -		put_page(page);
> -		return NULL;
> -	}
> -
> -	/* Did somebody else fill it already? */
> -	if (PageUptodate(page)) {
> -		unlock_page(page);
> -		return page;
> -	}
> -
> +readpage:
>  	return filemap_read_page(iocb, filp, mapping, page);
> +truncated:
> +	unlock_page(page);
> +	put_page(page);
> +	return NULL;
>  }
>  
>  static struct page *filemap_create_page(struct kiocb *iocb,
> -- 
> 2.28.0
>