Re: [PATCH v4] mm: filemap: retain mapped dropbehind folios

[Jan Kara]

On Mon 14-09-26 17:42:59, Barry Song wrote:
> On Mon, Sep 14, 2026 at 5:33 PM Jan Kara <jack@xxxxxxx> wrote:
> >
> > On Mon 14-09-26 09:09:32, Barry Song wrote:
> > > On Mon, Sep 14, 2026 at 7:34 AM Tal Zussman <tz2294@xxxxxxxxxxxx> wrote:
> > > >
> > > > On 9/13/26 6:51 PM, Barry Song wrote:
> > > > > On Fri, Sep 4, 2026 at 6:36 AM Tal Zussman <tz2294@xxxxxxxxxxxx> wrote:
> > > > >>
> > > > >> On 9/4/26 12:28 AM, Barry Song wrote:
> > > > >> > On Sun, Aug 30, 2026 at 7:59 PM Tal Zussman <tz2294@xxxxxxxxxxxx> wrote:
> > > > >> >>
> > > > >> >> On 8/30/26 6:25 AM, Wenjie Qi wrote:
> > > > >> >> > From: Wenjie Qi <qiwenjie@xxxxxxxxxx>
> > > > >> >> >
> > > > >> >> > Fault-around can map ready dropbehind folios without going through the
> > > > >> >> > normal page-cache lookup that clears dropbehind. A mapping represents a
> > > > >> >> > competing cached user, but writeback completion can currently unmap that
> > > > >> >> > folio. A later mmap access must then fault it back in.
> > > > >> >> >
> > > > >> >> > Retain mapped folios instead. For a mapped folio,
> > > > >> >> > folio_unmap_invalidate() can call unmap_mapping_folio(), which takes
> > > > >> >> > i_mmap_rwsem and may sleep. Retaining the folio also avoids this path when
> > > > >> >> > folio_end_dropbehind() runs in non-preemptible task context.
> > > > >> >> >
> > > > >> >> > Unmapped dropbehind folios continue through the existing invalidation path.
> > > > >> >> >
> > > > >> >> > Fixes: fb7d3bc41493 ("mm/filemap: drop streaming/uncached pages when writeback completes")
> > > > >> >> > Cc: stable@xxxxxxxxxxxxxxx
> > > > >> >> > Signed-off-by: Wenjie Qi <qiwenjie@xxxxxxxxxx>
> > > > >> >> > Reviewed-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx>
> > > > >> >> > Reviewed-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
> > > > >> >> > ---
> > > > >> >> > The mapped-plus-dropbehind state was reproduced in QEMU. The patched
> > > > >> >> > kernel retained the mapped folio and continued to evict the unmapped
> > > > >> >> > dontcache folio. I did not reproduce the sleeping-in-atomic warning and
> > > > >> >> > am not aware of an existing report.
> > > > >> >> >
> > > > >> >>
> > > > >> >> I was able to trigger the sleeping-while-atomic BUG on current mainline with
> > > > >> >> some help from Claude. The reproducer itself is straightforward enough, but the
> > > > >> >> configuration was a little finicky. The atomic context comes from using
> > > > >> >> threadirqs so that virtblk_done() completes requests from the irq thread under
> > > > >> >> local_bh_disable() and the vq spinlock with irqs off, so in_task() is true but
> > > > >> >> the i_mmap_rwsem down_read() sleeps. It also needed 1 vCPU (so completion stays
> > > > >> >> in the irq thread rather than softirq) and ext4 -o dioread_lock (so end_io runs
> > > > >> >> inline instead of on a workqueue). With v4 applied the bug is gone, so:
> > > > >> >>
> > > > >> >
> > > > >> > Thanks, Tal. Then I'm getting quite confused. Since with Wenjie's
> > > > >> > patch, we are no longer going to sleep for dropbehind in `filemap.c`,
> > > > >> > do we still need all the complex logic in the block device and file
> > > > >> > system layers to move dropbehind to a workqueue task context?
> > > > >> >
> > > > >>
> > > > >> Hi Barry,
> > > > >>
> > > > >> Let me clarify. Wenjie's patch prevents sleeping in a very specific case.
> > > > >> The existing in_task() check covered most other cases, in that it just
> > > > >> doesn't do dropbehind invalidation if we're not executing in task context
> > > > >> (for example, block device writeback completing in interrupt context).
> > > > >>
> > > > >> However, dropping the invalidation defeats the point of dropbehind, as it
> > > > >> leaves the pages in the page cache. If we get rid of the task-context
> > > > >> deferral mechanism, we wouldn't sleep, but we also wouldn't invalidate.
> > > > >> Wenjie's change is largely unrelated to this.
> > > > >
> > > > > Hi Tal,
> > > > >
> > > > > Thanks for the clarification. However, I’m still not convinced.
> > > > >
> > > > > If `unmap_mapping_folio()` is the only place where we might
> > > > > sleep — where we might take `mapping->i_mmap_rwsem` through
> > > > > `i_mmap_lock_read()`, as Wenjian pointed out in [1] — we have now
> > > > > prevented that from happening with `if (!folio_mapped(folio))`.
> > > > >
> > > > > So no mapped folio should call `folio_unmap_invalidate()`:
> > > > >
> > > > > static void filemap_end_dropbehind(struct folio *folio)
> > > > > {
> > > > >          struct address_space *mapping = folio->mapping;
> > > > >
> > > > >          VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> > > > >
> > > > >          if (folio_test_writeback(folio) || folio_test_dirty(folio))
> > > > >                  return;
> > > > >          if (!folio_test_clear_dropbehind(folio))
> > > > >                  return;
> > > > >          if (mapping && !folio_mapped(folio))
> > > > >                  folio_unmap_invalidate(mapping, folio, 0);
> > > > > }
> > > > >
> > > > > Why do we still need the `in_task()` check in
> > > > > `folio_end_dropbehind()`?
> > > > >
> > > > > void folio_end_dropbehind(struct folio *folio)
> > > > > {
> > > > >          if (!folio_test_dropbehind(folio))
> > > > >                  return;
> > > > >
> > > > >          /*
> > > > >           * Hitting !in_task() should not happen off RWF_DONTCACHE writeback,
> > > > >           * but can happen if normal writeback just happens to find dirty folios
> > > > >           * that were created as part of uncached writeback, and that writeback
> > > > >           * would otherwise not need non-IRQ handling. Just skip the
> > > > >           * invalidation in that case.
> > > > >           */
> > > > >          if (in_task() && folio_trylock(folio)) {
> > > > >                  filemap_end_dropbehind(folio);
> > > > >                  folio_unlock(folio);
> > > > >          }
> > > > > }
> > > > >
> > > > > Could any other operation in `folio_unmap_invalidate()` sleep for
> > > > > unmapped folios? Otherwise, it seems we don't need `in_task()` at
> > > > > all?
> >
> > We do need it. folio_unmap_invalidate() calls filemap_release_folio() which
> > calls into ->release_folio() handler and generally filesystems are allowed
> > to sleep in this handler. Now usually they don't actually sleep there but
> > they definitely grab a lot of irq unsafe locks there as Tal writes below.
> 
> Hi Jan,
> 
> Thanks very much for your explanation. That makes a lot of
> sense to me.
> 
> Then, based on the above, I am getting into another confusion and
> have another question. If `release_folio()` is actually allowed
> to sleep, and some filesystem might sleep there in the future,
> wouldn't Wenjie's patch [1] make sense in that case, as it
> guarantees that `->release_folio()` is always called in a
> sleepable context, while `in_task()` does not provide such a
> guarantee?
> 
> https://lore.kernel.org/linux-mm/20260820142956.1414337-1-qiwenjie@xxxxxxxxxx/

Well, adding the folio_mapped() check makes sense as well as Matthew
explained - we shouldn't be dropping mapped folios(). But yes, now that
you've made me look into this I don't think dropping the in_task() check
here is correct and we still need to make sure that *at least* we are in
non-interrupt context and preferably to keep fs maintainer's sanity in a
sleepable context. Matthew?

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR