Re: [PATCH 1/3] mm: handle swap page faults under VMA lock if page is uncontended

[Matthew Wilcox]

On Mon, May 01, 2023 at 10:04:56PM -0700, Suren Baghdasaryan wrote:
> On Mon, May 1, 2023 at 8:22 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> >
> > On Mon, May 01, 2023 at 07:30:13PM -0700, Suren Baghdasaryan wrote:
> > > On Mon, May 1, 2023 at 7:02 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, May 01, 2023 at 10:50:23AM -0700, Suren Baghdasaryan wrote:
> > > > > +++ b/mm/memory.c
> > > > > @@ -3711,11 +3711,6 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> > > > >       if (!pte_unmap_same(vmf))
> > > > >               goto out;
> > > > >
> > > > > -     if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
> > > > > -             ret = VM_FAULT_RETRY;
> > > > > -             goto out;
> > > > > -     }
> > > > > -
> > > > >       entry = pte_to_swp_entry(vmf->orig_pte);
> > > > >       if (unlikely(non_swap_entry(entry))) {
> > > > >               if (is_migration_entry(entry)) {
> > > >
> > > > You're missing the necessary fallback in the (!folio) case.
> > > > swap_readpage() is synchronous and will sleep.
> > >
> > > True, but is it unsafe to do that under VMA lock and has to be done
> > > under mmap_lock?
> >
> > ... you were the one arguing that we didn't want to wait for I/O with
> > the VMA lock held?
> 
> Well, that discussion was about waiting in folio_lock_or_retry() with
> the lock being held. I argued against it because currently we drop
> mmap_lock lock before waiting, so if we don't drop VMA lock we would
> be changing the current behavior which might introduce new
> regressions. In the case of swap_readpage and swapin_readahead we
> already wait with mmap_lock held, so waiting with VMA lock held does
> not introduce new problems (unless there is a need to hold mmap_lock).
> 
> That said, you are absolutely correct that this situation can be
> improved by dropping the lock in these cases too. I just didn't want
> to attack everything at once. I believe after we agree on the approach
> implemented in https://lore.kernel.org/all/20230501175025.36233-3-surenb@xxxxxxxxxx
> for dropping the VMA lock before waiting, these cases can be added
> easier. Does that make sense?

OK, I looked at this path some more, and I think we're fine.  This
patch is only called for SWP_SYNCHRONOUS_IO which is only set for
QUEUE_FLAG_SYNCHRONOUS devices, which are brd, zram and nvdimms
(both btt and pmem).  So the answer is that we don't sleep in this
path, and there's no need to drop the lock.