Matthew Wilcox <willy@xxxxxxxxxxxxx> 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.
