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

[Jan Kara]

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.

								Honza

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