Re: [PATCH] fs/gfs2: prevent premature evict in prune_icache_sb

[Jan Kara]

On Wed 23-09-26 23:01:52, Andreas Gruenbacher wrote:
> On Tue, Sep 22, 2026 at 1:22 PM Jan Kara <jack@xxxxxxx> wrote:
> > Hello Andreas!
> >
> > On Mon 21-09-26 23:17:11, Andreas Gruenbacher wrote:
> > > Introduce a new I_NOPRUNE i_state flag for preventing prune_icache_sb()
> > > from pruning specific inodes.
> > >
> > > prune_icache_sb() prunes clean inodes under memory pressure ("direct
> > > reclaim").  An inode is considered clean when none of the i_state flags
> > > are set; the assumption is that evicting inodes that don't have any
> > > i_state flags set will be fairly cheap.
> > >
> > > Unfortunately, on gfs2, inodes can be clean in the sense that they won't
> > > require writing back to disk, but they may still have outstanding
> > > revokes (as indicated by the GLF_LFLUSH inode glock flag).  To evict one
> > > of those inodes, those outstanding revokes need to be written out first.
> > >
> > > This requires flushing the log, which is already an expensive operation.
> > > When in data=ordered mode, all the ordered data needs to be written out
> > > before the log can be flushed, which makes things even worse.
> > >
> > > As previously discussed [*], we are currently also running into the
> > > following warning in iomap_writepages() when flushing ordered data:
> > >
> > >     /*
> > >      * Writeback from reclaim context should never happen except in the case
> > >      * of a VM regression so warn about it and refuse to write the data.
> > >      */
> > >     if (WARN_ON_ONCE((current->flags & (PF_MEMALLOC | PF_KSWAPD)) ==
> > >                     PF_MEMALLOC))
> > >             return -EIO;
> > >
> > > So we need to prevent prune_icache_sb() from evicting inodes that have
> > > any outstanding revokes.  This patch achieves that by introducing a new
> > > I_NOPRUNE i_state flag.  When that new flag (or any other flag) is set,
> > > prune_icache_sb() will skip those inodes.
> > >
> > > In the filesystem code, we set and clear the I_NOPRUNE flag in sync with
> > > the GLF_LFLUSH inode glock flag.
> > >
> > > An alternative approach might be to allow filesystems to refuse evicting
> > > inodes that prune_icache_sb() has already selected.  This could be
> > > achieved by changing the ->evict_inode super operation to return a
> > > lru_status code or similar.  prune_icache_sb() would then have to
> > > resurrect inodes that were already marked I_FREEING and put them back
> > > onto the lru list.  This approach doesn't seem obviously better than
> > > introducing I_NOPRUNE, so I haven't pursued this any further.
> >
> > So for situations like this I've written a patch set to allow filesystem
> > to mark inode for deferred reclaim (from a workqueue) [1]. Would that work
> > for you? It will definitely solve your problems with warnings in memory
> > allocator.
> >
> > I don't want to have two different mechanisms for the same issue so if my
> > solution doesn't quite work for you, let's discuss how to make it better
> > :).
> 
> thanks, this would solve some of my problems. In the gfs2 case, inodes
> are only in "deferred reclaim" state for a while (while they have
> outstanding revokes) before becoming directly reclaimable again. So
> I'd also need a way to clear the I_DEFER_RECLAIM flag again.

Well, that's kind of the case for other filesystems as well. But since
deferred reclaim is relatively cheap my current thinking was that if
somebody did something expensive to the inode like allocating blocks
(that's what triggers this for ext4, not sure what exactly triggers the
state for GFS2), then we can afford also the additional small cost of
offloading the reclaim to a workqueue (regardless whether it is really
needed at the time reclaim actually happens). We just don't want to
unconditionally offload because if someone is just statting millions of
inodes on some filesystem, he's basically bound by the speed of reclaim on
sufficiently fast storage and the additional delay of reclaim and the cost
of offloading would become visible.

> The I_DEFER_RECLAIM flag setting and clearing code would be racy and
> ugly though, so I'm wondering if we can change the ->evict_inode()
> super operation to return a DIRECT / DEFERRED status code instead. The
> filesystem could then look at the filesystem-internal state and return
> how to proceed with the inode. For example, fs2_evict_inode() would
> probably request a DEFERED evict when GLF_LFLUSH is set, PF_MEMALLOC
> is set, and PF_KSWAPD is not set.

Right, but I really want filesystems to mess with process reclaim flags as
little as possible because that smells with layering violation and
generally tends to be fragile. In some code paths such as iput_final()
or evict_inodes() I really prefer to be sure that the filesystem just frees
the inode without having to check details of all the implementations making
sure they don't do something stupid and offload when they shouldn't.

So if we do something like this, I'd probably prefer explicit flag to
->evict() telling that offloading is possible and in that case filesystem
can return special value meaning it requests offloading and prune_icache()
would handle that.

Another problem I see with this scheme is that VFS is completely oblivious
to the number of inodes for which reclaim will be deferred and so there's
no chance for throttling places that create such inodes when the reclaim is
getting overloaded.

So overall I'd prefer we avoid all this complexity at least until we have a
real-world case where the performance impact of unnecessary offloading is
visible.

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