Re: [PATCH] mm/filemap.c: Always read one page in do_sync_mmap_readahead()

[Alistair Popple <apopple@nvidia.com>]

Matthew Wilcox <willy@infradead.org> writes:

> On Tue, Jun 07, 2022 at 06:37:14PM +1000, Alistair Popple wrote:
>> ---
>>  include/linux/pagemap.h |  7 +++---
>>  mm/filemap.c            | 47 +++++++++++++----------------------------
>>  2 files changed, 18 insertions(+), 36 deletions(-)
>
> Love the diffstat ;-)
>
>> @@ -3011,14 +3001,8 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
>>  	}
>>  #endif
>>
>> -	/* If we don't want any read-ahead, don't bother */
>> -	if (vmf->vma->vm_flags & VM_RAND_READ)
>> -		return fpin;
>> -	if (!ra->ra_pages)
>> -		return fpin;
>> -
>> +	fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>>  	if (vmf->vma->vm_flags & VM_SEQ_READ) {
>> -		fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>>  		page_cache_sync_ra(&ractl, ra->ra_pages);
>>  		return fpin;
>>  	}
>
> Good.  Could even pull the maybe_unlock_mmap_for_io() all the way to the
> top of the file and remove it from the VM_HUGEPAGE case?

Good idea. Also while I'm here is there a reason we don't update
ra->start or mmap_miss for the VM_HUGEPAGE case?

>> @@ -3029,19 +3013,20 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
>>  		WRITE_ONCE(ra->mmap_miss, ++mmap_miss);
>>
>>  	/*
>> -	 * Do we miss much more than hit in this file? If so,
>> -	 * stop bothering with read-ahead. It will only hurt.
>> +	 * mmap read-around. If we don't want any read-ahead or if we miss more
>> +	 * than we hit don't bother with read-ahead and just read a single page.
>>  	 */
>> -	if (mmap_miss > MMAP_LOTSAMISS)
>> -		return fpin;
>> +	if ((vmf->vma->vm_flags & VM_RAND_READ) ||
>> +	    !ra->ra_pages || mmap_miss > MMAP_LOTSAMISS) {
>> +		ra->start = vmf->pgoff;
>> +		ra->size = 1;
>> +		ra->async_size = 0;
>> +	} else {
>
> I'd put the:
> 		/* mmap read-around */
> here
>
>> +		ra->start = max_t(long, 0, vmf->pgoff - ra->ra_pages / 2);
>> +		ra->size = ra->ra_pages;
>> +		ra->async_size = ra->ra_pages / 4;
>> +	}
>>
>> -	/*
>> -	 * mmap read-around
>> -	 */
>> -	fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>> -	ra->start = max_t(long, 0, vmf->pgoff - ra->ra_pages / 2);
>> -	ra->size = ra->ra_pages;
>> -	ra->async_size = ra->ra_pages / 4;
>>  	ractl._index = ra->start;
>>  	page_cache_ra_order(&ractl, ra, 0);
>>  	return fpin;
>> @@ -3145,9 +3130,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>>  			filemap_invalidate_lock_shared(mapping);
>>  			mapping_locked = true;
>>  		}
>> -		folio = __filemap_get_folio(mapping, index,
>> -					  FGP_CREAT|FGP_FOR_MMAP,
>> -					  vmf->gfp_mask);
>> +		folio = filemap_get_folio(mapping, index);
>>  		if (!folio) {
>>  			if (fpin)
>>  				goto out_retry;
>
> I think we also should remove the filemap_invalidate_lock_shared()
> here, no?

Right, afaik filemap_invalidate_lock_shared() is needed when
instantiating pages in the page cache during fault, which this patch
does via page_cache_ra_order() in do_sync_mmap_readahead() so I think
you're right about removing it for filemap_get_folio().

However do_sync_mmap_readahead() is the way normal (ie. !VM_RAND_READ)
pages would get instantiated today. So shouldn't
filemap_invalidate_lock_shared() be called before
do_sync_mmap_readahead() anyway? Or am I missing something?

> We also need to handle the !folio case differently.  Before, if it was
> gone, that was definitely an OOM.  Now if it's gone it might have been
> truncated, or removed due to memory pressure, or it might be an OOM
> situation where readahead didn't manage to create the folio.

Good point, thanks for catching that.