Re: [PATCH 11/17] mm/filemap: Add filemap_range_uptodate

[Matthew Wilcox <willy@xxxxxxxxxxxxx>]

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.