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

[Tal Zussman <tz2294@columbia.edu>]

On 9/13/26 6:51 PM, Barry Song wrote:
> On Fri, Sep 4, 2026 at 6:36 AM Tal Zussman <tz2294@columbia.edu> wrote:
>>
>> On 9/4/26 12:28 AM, Barry Song wrote:
>> > On Sun, Aug 30, 2026 at 7:59 PM Tal Zussman <tz2294@columbia.edu> wrote:
>> >>
>> >> On 8/30/26 6:25 AM, Wenjie Qi wrote:
>> >> > From: Wenjie Qi <qiwenjie@xiaomi.com>
>> >> >
>> >> > 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@vger.kernel.org
>> >> > Signed-off-by: Wenjie Qi <qiwenjie@xiaomi.com>
>> >> > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> >> > Reviewed-by: Tal Zussman <tz2294@columbia.edu>
>> >> > ---
>> >> > 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.

- Tal