On Mon, 2020-02-10 at 21:05 +0300, Kirill A. Shutemov wrote:
> On Mon, Feb 10, 2020 at 09:25:11AM -0800, Matthew Wilcox wrote:
> > On Mon, Feb 10, 2020 at 12:00:29PM -0500, Qian Cai wrote:
> > > @@ -2622,7 +2622,7 @@ void filemap_map_pages(struct vm_fault *vmf,
> > > if (page->index >= max_idx)
> > > goto unlock;
> > >
> > > - if (file->f_ra.mmap_miss > 0)
> > > + if (data_race(file->f_ra.mmap_miss > 0))
> > > file->f_ra.mmap_miss--;
> >
> > How is this safe? Two threads can each see 1, and then both decrement the
> > in-memory copy, causing it to end up at -1.
>
> Right, it is bogus.
>
> Below is my completely untested attempt on fix this. It still allows
> races, but they will only lead to missed accounting, but not underflow.
Looks good to me. Do you plan to send out an official patch?
>
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 1784478270e1..1919d37c646a 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2365,6 +2365,7 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
> struct address_space *mapping = file->f_mapping;
> struct file *fpin = NULL;
> pgoff_t offset = vmf->pgoff;
> + unsigned mmap_miss;
>
> /* If we don't want any read-ahead, don't bother */
> if (vmf->vma->vm_flags & VM_RAND_READ)
> @@ -2380,14 +2381,15 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
> }
>
> /* Avoid banging the cache line if not needed */
> - if (ra->mmap_miss < MMAP_LOTSAMISS * 10)
> - ra->mmap_miss++;
> + mmap_miss = READ_ONCE(ra->mmap_miss);
> + if (mmap_miss < MMAP_LOTSAMISS * 10)
> + 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.
> */
> - if (ra->mmap_miss > MMAP_LOTSAMISS)
> + if (mmap_miss > MMAP_LOTSAMISS)
> return fpin;
>
> /*
> @@ -2413,13 +2415,15 @@ static struct file *do_async_mmap_readahead(struct vm_fault *vmf,
> struct file_ra_state *ra = &file->f_ra;
> struct address_space *mapping = file->f_mapping;
> struct file *fpin = NULL;
> + unsigned int mmap_miss;
> pgoff_t offset = vmf->pgoff;
>
> /* If we don't want any read-ahead, don't bother */
> if (vmf->vma->vm_flags & VM_RAND_READ)
> return fpin;
> - if (ra->mmap_miss > 0)
> - ra->mmap_miss--;
> + mmap_miss = READ_ONCE(ra->mmap_miss);
> + if (mmap_miss)
> + WRITE_ONCE(ra->mmap_miss, --mmap_miss);
> if (PageReadahead(page)) {
> fpin = maybe_unlock_mmap_for_io(vmf, fpin);
> page_cache_async_readahead(mapping, ra, file,
> @@ -2586,7 +2590,9 @@ void filemap_map_pages(struct vm_fault *vmf,
> unsigned long max_idx;
> XA_STATE(xas, &mapping->i_pages, start_pgoff);
> struct page *page;
> + unsigned long mmap_miss;
>
> + mmap_miss = READ_ONCE(file->f_ra.mmap_miss);
> rcu_read_lock();
> xas_for_each(&xas, page, end_pgoff) {
> if (xas_retry(&xas, page))
> @@ -2622,8 +2628,8 @@ void filemap_map_pages(struct vm_fault *vmf,
> if (page->index >= max_idx)
> goto unlock;
>
> - if (file->f_ra.mmap_miss > 0)
> - file->f_ra.mmap_miss--;
> + if (mmap_miss > 0)
> + mmap_miss--;
>
> vmf->address += (xas.xa_index - last_pgoff) << PAGE_SHIFT;
> if (vmf->pte)
> @@ -2643,6 +2649,7 @@ void filemap_map_pages(struct vm_fault *vmf,
> break;
> }
> rcu_read_unlock();
> + WRITE_ONCE(file->f_ra.mmap_miss, mmap_miss);
> }
> EXPORT_SYMBOL(filemap_map_pages);
>
