Hi, On Wed, Apr 26, 2023 at 2:27 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > 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@xxxxxxxxxxxxx> 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. Agreed, it's just better than nothing... > > > 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. Sure, I'm happy to do it like you say. Do you have any suggestions for the similar lock_buffer() case, or are you OK w/ the timeout there? Mel: do you have any comments? In your previous response [1] you seemed to indicate that you thought that short waits for read were a good idea. [1] https://lore.kernel.org/r/20230420102304.7wdquge2b7r3xerj@xxxxxxxxxxxxxxxxxxx -Doug
