Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Mon, 2 Oct 2023 11:55:23 +0200
From:   Jan Kara <jack@suse.cz>
To:     Lorenzo Stoakes <lstoakes@gmail.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Matthew Wilcox <willy@infradead.org>, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH] mm/filemap: clarify filemap_fault() comments for not
 uptodate case
Message-ID: <20231002095523.6gttx365tvfaeexb@quack3>
References: <20230930231029.88196-1-lstoakes@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230930231029.88196-1-lstoakes@gmail.com>
Precedence: bulk

On Sun 01-10-23 00:10:29, Lorenzo Stoakes wrote:
> The existing comments in filemap_fault() suggest that, after either a minor
> fault has occurred and filemap_get_folio() found a folio in the page cache,
> or a major fault arose and __filemap_get_folio(FGP_CREATE...) did the job
> (having relied on do_sync_mmap_readahead() or filemap_read_folio() to read
> in the folio), the only possible reason it could not be uptodate is because
> of an error.
> 
> This is not so, as if, for instance, the fault occurred within a VMA which
> had the VM_RAND_READ flag set (via madvise() with the MADV_RANDOM flag
> specified), this would cause even synchronous readahead to fail to read in
> the folio.
> 
> I confirmed this by dropping page caches and faulting in memory madvise()'d
> this way, observing that this code path was reached on each occasion.
> 
> Clarify the comments to include this case, and additionally update the
> comment recently added around the invalidate lock logic to make it clear
> the comment explicitly refers to the minor fault case.
> 
> In addition, while we're here, refer to folios rather than pages.
> 
> Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>

After the alignment fixup the patch looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/filemap.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index d285ec5f9162..959694a2ade7 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3308,21 +3308,28 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>  	VM_BUG_ON_FOLIO(!folio_contains(folio, index), folio);
>  
>  	/*
> -	 * We have a locked page in the page cache, now we need to check
> -	 * that it's up-to-date. If not, it is going to be due to an error.
> +	 * We have a locked folio in the page cache, now we need to check
> +	 * that it's up-to-date. If not, it is going to be due to an error,
> +	 * or because readahead was otherwise unable to retrieve it.
>  	 */
>  	if (unlikely(!folio_test_uptodate(folio))) {
>  		/*
> -		 * The page was in cache and uptodate and now it is not.
> -		 * Strange but possible since we didn't hold the page lock all
> -		 * the time. Let's drop everything get the invalidate lock and
> -		 * try again.
> +		 * If the invalidate lock is not held, the folio was in cache and
> +		 * uptodate and now it is not. Strange but possible since we
> +		 * didn't hold the page lock all the time. Let's drop everything,
> +		 * get the invalidate lock and try again.
>  		 */
>  		if (!mapping_locked) {
>  			folio_unlock(folio);
>  			folio_put(folio);
>  			goto retry_find;
>  		}
> +
> +		/*
> +		 * OK, the folio is really not uptodate. This can be because the
> +		 * VMA has the VM_RAND_READ flag set, or because an error
> +		 * arose. Let's read it in directly.
> +		 */
>  		goto page_not_uptodate;
>  	}
>  
> -- 
> 2.42.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR