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

[Barry Song]

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?
> >
>
> So I think you're right about the sleeping aspect, but the problem is
> that this still gets called from interrupt context. folio_unmap_invalidate()
> grabs a number of locks in a non-IRQ safe way. Just at a glance, the
> spin_lock(&mapping->host->i_lock) call could lead to a deadlock, since other
> paths don't call it with interrupts disabled. And xa_unlock_irq() enables
> interrupts unconditionally, which would be incorrect from hardirq I believe.

That makes sense to me. If that's the case, it seems the
comment above

`if (in_task() && folio_trylock(folio))`

is no longer relevant at all.

BTW, if we use `spin_lock_irqsave(&mapping->host->i_lock)` in
other places and also restore IRQs when unlocking the xarray, then
we shouldn't need the `in_task()` check anymore, should we?

Best Regards
Barry