Re: [PATCH v2 1/4] mm/filemap: Add folio_lock_timeout()

[Matthew Wilcox <willy@infradead.org>]

On Wed, Apr 26, 2023 at 01:46:58PM -0700, Doug Anderson wrote:
> On Wed, Apr 26, 2023 at 8:14 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > I'm not generally a fan of lock-with-timeout approaches.  I think the
> > rationale for this one makes sense, but we're going to see some people
> > try to use this for situations where it doesn't make sense.
> 
> Although it won't completely prevent the issue, I could add a comment

People don't read comments.

> > Hm.  If the problem is that we want to wait for the lock unless the
> > lock is being held for I/O, we can actually tell that in the caller.
> >
> >         if (folio_test_uptodate(folio))
> >                 folio_lock(folio);
> >         else
> >                 folio_trylock(folio);
> >
> > (the folio lock isn't held for writeback, just taken and released;
> > if the folio is uptodate, the folio lock should only be taken for a
> > short time; if it's !uptodate then it's probably being read)
> 
> The current place in patch #3 where I'm using folio_lock_timeout()
> only calls it if a folio_trylock() already failed [2]. So I guess the
> idea would be that if the trylock failed and folio_test_uptodate()
> returns 0 then we immediately fail, otherwise we call the unbounded
> folio_trylock()?

Looking at the actual code, here's what I'd do:

+++ b/mm/migrate.c
@@ -1156,6 +1156,14 @@ static int migrate_folio_unmap(new_page_t get_new_page, free_page_t put_new_page
                if (current->flags & PF_MEMALLOC)
                        goto out;

+               /*
+                * In "light" mode, we can wait for transient locks (eg
+                * inserting a page into the page table), but it's not
+                * worth waiting for I/O.
+                */
+               if (mode == MIGRATE_SYNC_LIGHT && !folio_test_uptodate(folio))
+                       goto out;
+
                folio_lock(src);
        }
        locked = true;

> I put some traces in and ran my test and it turns out that in every
> case (except one) where the tre initial folio_trylock() failed I saw
> folio_test_uptodate() return 0. Assuming my test case is typical, I
> think that means that coding it with folio_test_uptodate() is roughly
> the same as just never waiting at all for the folio lock in the
> SYNC_LIGHT case. In the original discussion of my v1 patch people
> didn't like that idea. ...so I think that for now I'm going to keep it
> with the timeout flow.

I think that means that your specific test is generally going to
exercise the case where the lock is held because we're waiting for I/O.
That's exactly what you set it up to produce, after all!  But it won't
affect the cases where the folio lock is being held for other reasons,
which your testcase is incredibly unlikely to produce.