[RFC PATCH 0/2] Defer evicting inodes to a workqueue

[RFC PATCH 0/2] Defer evicting inodes to a workqueue

[Matthew Wilcox (Oracle)]

Evicting an inode is a complex process which may require allocating
memory, running a transaction, etc, etc.  Doing it as part of reclaim
is a bad idea and leads to hard-to-reproduce bug reports.  This pair of
patches defers it to a workqueue if we're in reclaim.

Bugs:
https://lore.kernel.org/all/CALm_T+3j+dyK02UgPiv9z0f1oj-HM63oxhsB0JF9gVAjeVfm1Q@mail.gmail.com/
https://lore.kernel.org/all/CALm_T+2cEDUJvjh6Lv+6Mg9QJxGBVAHu-CY+okQgh-emWa7-1A@mail.gmail.com/
https://lore.kernel.org/all/20250326105914.3803197-1-matt@readmodwrite.com/

I don't know if this is a good idea, to be honest.  We're kind of lying
to reclaim by pretending that we've freed N inodes when actually we've
just queued them for eviction.  On the other hand, XFS has been doing
it for years, so perhaps it's not important.

I think the real solution here is to convert the Linux VFS to use the
same inode lifecycle as IRIX, but I don't fully understand the downsides
of that approach.  One major pro of course is that XFS wouldn't have to
work around the Linux VFS any more.

I do wonder if a better approach might be:

+++ b/fs/inode.c
@@ -883,6 +883,10 @@ void evict_inodes(struct super_block *sb)
                        spin_unlock(&inode->i_lock);
                        continue;
                }
+               if (in_reclaim() && (inode->i_state & I_DIRTY_ALL)) {
+                       spin_unlock(&inode->i_lock);
+                       continue;
+               }
 
                inode->i_state |= I_FREEING;
                inode_lru_list_del(inode);

Thoughts?

Matthew Wilcox (Oracle) (2):
  Add in_reclaim()
  fs: Defer evicting inodes to a workqueue

 fs/inode.c               | 36 ++++++++++++++++++++++++++++++++++--
 include/linux/sched/mm.h | 11 +++++++++++
 mm/page_alloc.c          | 10 +++++-----
 3 files changed, 50 insertions(+), 7 deletions(-)

-- 
2.47.2

[RFC PATCH 1/2] Add in_reclaim()

[Matthew Wilcox (Oracle)]

This is more meaningful than checking PF_MEMALLOC.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/sched/mm.h | 11 +++++++++++
 mm/page_alloc.c          | 10 +++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
index 2201da0afecc..a9825ea7c331 100644
--- a/include/linux/sched/mm.h
+++ b/include/linux/sched/mm.h
@@ -468,6 +468,17 @@ static inline void memalloc_pin_restore(unsigned int flags)
 	memalloc_flags_restore(flags);
 }
 
+/**
+ * in_reclaim - Is the current task doing reclaim?
+ *
+ * This is true if the current task is kswapd or if we've entered
+ * direct reclaim.
+ */
+static inline bool in_reclaim(void)
+{
+	return current->flags & PF_MEMALLOC;
+}
+
 #ifdef CONFIG_MEMCG
 DECLARE_PER_CPU(struct mem_cgroup *, int_active_memcg);
 /**
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d1d037f97c5f..d27265df56b5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4220,7 +4220,7 @@ static bool __need_reclaim(gfp_t gfp_mask)
 		return false;
 
 	/* this guy won't enter reclaim */
-	if (current->flags & PF_MEMALLOC)
+	if (in_reclaim())
 		return false;
 
 	if (gfp_mask & __GFP_NOLOCKDEP)
@@ -4455,10 +4455,10 @@ static inline int __gfp_pfmemalloc_flags(gfp_t gfp_mask)
 		return 0;
 	if (gfp_mask & __GFP_MEMALLOC)
 		return ALLOC_NO_WATERMARKS;
-	if (in_serving_softirq() && (current->flags & PF_MEMALLOC))
+	if (in_serving_softirq() && in_reclaim())
 		return ALLOC_NO_WATERMARKS;
 	if (!in_interrupt()) {
-		if (current->flags & PF_MEMALLOC)
+		if (in_reclaim())
 			return ALLOC_NO_WATERMARKS;
 		else if (oom_reserves_allowed(current))
 			return ALLOC_OOM;
@@ -4627,7 +4627,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		 * because we cannot reclaim anything and only can loop waiting
 		 * for somebody to do a work for us.
 		 */
-		WARN_ON_ONCE(current->flags & PF_MEMALLOC);
+		WARN_ON_ONCE(in_reclaim());
 	}
 
 restart:
@@ -4774,7 +4774,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		goto nopage;
 
 	/* Avoid recursion of direct reclaim */
-	if (current->flags & PF_MEMALLOC)
+	if (in_reclaim())
 		goto nopage;
 
 	/* Try direct reclaim and then allocating */
-- 
2.47.2

Re: [RFC PATCH 1/2] Add in_reclaim()

[Jan Kara]

On Wed 24-09-25 10:09:56, Matthew Wilcox (Oracle) wrote:
> This is more meaningful than checking PF_MEMALLOC.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

This is IMO a good cleanup regardless of the other change. I always have to
lookup details in reclaim code when I need to deal with PF_MEMALLOC :).
Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/sched/mm.h | 11 +++++++++++
>  mm/page_alloc.c          | 10 +++++-----
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> index 2201da0afecc..a9825ea7c331 100644
> --- a/include/linux/sched/mm.h
> +++ b/include/linux/sched/mm.h
> @@ -468,6 +468,17 @@ static inline void memalloc_pin_restore(unsigned int flags)
>  	memalloc_flags_restore(flags);
>  }
>  
> +/**
> + * in_reclaim - Is the current task doing reclaim?
> + *
> + * This is true if the current task is kswapd or if we've entered
> + * direct reclaim.
> + */
> +static inline bool in_reclaim(void)
> +{
> +	return current->flags & PF_MEMALLOC;
> +}
> +
>  #ifdef CONFIG_MEMCG
>  DECLARE_PER_CPU(struct mem_cgroup *, int_active_memcg);
>  /**
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d1d037f97c5f..d27265df56b5 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4220,7 +4220,7 @@ static bool __need_reclaim(gfp_t gfp_mask)
>  		return false;
>  
>  	/* this guy won't enter reclaim */
> -	if (current->flags & PF_MEMALLOC)
> +	if (in_reclaim())
>  		return false;
>  
>  	if (gfp_mask & __GFP_NOLOCKDEP)
> @@ -4455,10 +4455,10 @@ static inline int __gfp_pfmemalloc_flags(gfp_t gfp_mask)
>  		return 0;
>  	if (gfp_mask & __GFP_MEMALLOC)
>  		return ALLOC_NO_WATERMARKS;
> -	if (in_serving_softirq() && (current->flags & PF_MEMALLOC))
> +	if (in_serving_softirq() && in_reclaim())
>  		return ALLOC_NO_WATERMARKS;
>  	if (!in_interrupt()) {
> -		if (current->flags & PF_MEMALLOC)
> +		if (in_reclaim())
>  			return ALLOC_NO_WATERMARKS;
>  		else if (oom_reserves_allowed(current))
>  			return ALLOC_OOM;
> @@ -4627,7 +4627,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  		 * because we cannot reclaim anything and only can loop waiting
>  		 * for somebody to do a work for us.
>  		 */
> -		WARN_ON_ONCE(current->flags & PF_MEMALLOC);
> +		WARN_ON_ONCE(in_reclaim());
>  	}
>  
>  restart:
> @@ -4774,7 +4774,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  		goto nopage;
>  
>  	/* Avoid recursion of direct reclaim */
> -	if (current->flags & PF_MEMALLOC)
> +	if (in_reclaim())
>  		goto nopage;
>  
>  	/* Try direct reclaim and then allocating */
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[RFC PATCH 2/2] fs: Defer evicting inodes to a workqueue

[Matthew Wilcox (Oracle)]

If we're in memory reclaim, evicting inodes is actually a bad idea.
The filesystem may need to allocate more memory to evict the inode
than it will free by evicting the inode.  It's better to defer
evicting the inode until a workqueue has time to run.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 fs/inode.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 9d882b0fc787..fe7899cdd50c 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -854,6 +854,34 @@ static void dispose_list(struct list_head *head)
 	}
 }
 
+static DEFINE_SPINLOCK(deferred_inode_lock);
+static LIST_HEAD(deferred_inode_list);
+
+static void dispose_inodes_wq(struct work_struct *work)
+{
+	LIST_HEAD(dispose);
+
+	spin_lock_irq(&deferred_inode_lock);
+	list_splice_init(&deferred_inode_list, &dispose);
+	spin_unlock_irq(&deferred_inode_lock);
+
+	dispose_list(&dispose);
+}
+
+static DECLARE_WORK(dispose_inode_work, dispose_inodes_wq);
+
+static void deferred_dispose_inodes(struct list_head *inodes)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&deferred_inode_lock, flags);
+	list_splice_tail(inodes, &deferred_inode_list);
+	spin_unlock_irqrestore(&deferred_inode_lock, flags);
+
+	printk("deferring some inodes\n");
+	schedule_work(&dispose_inode_work);
+}
+
 /**
  * evict_inodes	- evict all evictable inodes for a superblock
  * @sb:		superblock to operate on
@@ -897,13 +925,17 @@ void evict_inodes(struct super_block *sb)
 		if (need_resched()) {
 			spin_unlock(&sb->s_inode_list_lock);
 			cond_resched();
-			dispose_list(&dispose);
+			if (!in_reclaim())
+				dispose_list(&dispose);
 			goto again;
 		}
 	}
 	spin_unlock(&sb->s_inode_list_lock);
 
-	dispose_list(&dispose);
+	if (!in_reclaim())
+		dispose_list(&dispose);
+	else
+		deferred_dispose_inodes(&dispose);
 }
 EXPORT_SYMBOL_GPL(evict_inodes);
 
-- 
2.47.2

Re: [RFC PATCH 2/2] fs: Defer evicting inodes to a workqueue

[Kiryl Shutsemau]

On Wed, Sep 24, 2025 at 10:09:57AM +0100, Matthew Wilcox (Oracle) wrote:
> If we're in memory reclaim, evicting inodes is actually a bad idea.
> The filesystem may need to allocate more memory to evict the inode
> than it will free by evicting the inode.  It's better to defer
> evicting the inode until a workqueue has time to run.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/inode.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d882b0fc787..fe7899cdd50c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -854,6 +854,34 @@ static void dispose_list(struct list_head *head)
>  	}
>  }
>  
> +static DEFINE_SPINLOCK(deferred_inode_lock);
> +static LIST_HEAD(deferred_inode_list);
> +
> +static void dispose_inodes_wq(struct work_struct *work)
> +{
> +	LIST_HEAD(dispose);
> +
> +	spin_lock_irq(&deferred_inode_lock);
> +	list_splice_init(&deferred_inode_list, &dispose);
> +	spin_unlock_irq(&deferred_inode_lock);
> +
> +	dispose_list(&dispose);
> +}
> +
> +static DECLARE_WORK(dispose_inode_work, dispose_inodes_wq);
> +
> +static void deferred_dispose_inodes(struct list_head *inodes)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&deferred_inode_lock, flags);

Why _irqsave? I don't see any interactions with interrupts.

> +	list_splice_tail(inodes, &deferred_inode_list);
> +	spin_unlock_irqrestore(&deferred_inode_lock, flags);
> +
> +	printk("deferring some inodes\n");

Debug leftovers?

> +	schedule_work(&dispose_inode_work);
> +}
> +
>  /**
>   * evict_inodes	- evict all evictable inodes for a superblock
>   * @sb:		superblock to operate on
> @@ -897,13 +925,17 @@ void evict_inodes(struct super_block *sb)
>  		if (need_resched()) {
>  			spin_unlock(&sb->s_inode_list_lock);
>  			cond_resched();
> -			dispose_list(&dispose);
> +			if (!in_reclaim())
> +				dispose_list(&dispose);
>  			goto again;
>  		}
>  	}
>  	spin_unlock(&sb->s_inode_list_lock);
>  
> -	dispose_list(&dispose);
> +	if (!in_reclaim())
> +		dispose_list(&dispose);
> +	else
> +		deferred_dispose_inodes(&dispose);
>  }
>  EXPORT_SYMBOL_GPL(evict_inodes);
>  
> -- 
> 2.47.2
> 

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [RFC PATCH 2/2] fs: Defer evicting inodes to a workqueue

[Matthew Wilcox]

On Wed, Sep 24, 2025 at 10:28:09AM +0100, Kiryl Shutsemau wrote:
> > +static void deferred_dispose_inodes(struct list_head *inodes)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&deferred_inode_lock, flags);
> 
> Why _irqsave? I don't see any interactions with interrupts.

Can't we enter reclaim from paths which have interrupts disabled?
I could easily be wrong about that.

> > +	list_splice_tail(inodes, &deferred_inode_list);
> > +	spin_unlock_irqrestore(&deferred_inode_lock, flags);
> > +
> > +	printk("deferring some inodes\n");
> 
> Debug leftovers?

Oops.  Thanks.

Re: [RFC PATCH 2/2] fs: Defer evicting inodes to a workqueue

[Jan Kara]

On Wed 24-09-25 10:31:25, Matthew Wilcox wrote:
> On Wed, Sep 24, 2025 at 10:28:09AM +0100, Kiryl Shutsemau wrote:
> > > +static void deferred_dispose_inodes(struct list_head *inodes)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&deferred_inode_lock, flags);
> > 
> > Why _irqsave? I don't see any interactions with interrupts.
> 
> Can't we enter reclaim from paths which have interrupts disabled?
> I could easily be wrong about that.

You cannot really do allocation with __GFP_FS set from atomic context?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH 2/2] fs: Defer evicting inodes to a workqueue

[Matthew Wilcox]

On Wed, Sep 24, 2025 at 10:28:09AM +0100, Kiryl Shutsemau wrote:
> > +	printk("deferring some inodes\n");
> 
> Debug leftovers?

$ grep defer kern.out

I have low confidence in the correctness of this patch as I don't have a
reliable reproducer.  I haven't seen the printk trigger yet.  Maybe I
should force it to trigger sometimes.

Re: [RFC PATCH 2/2] fs: Defer evicting inodes to a workqueue

[Jan Kara]

On Wed 24-09-25 10:09:57, Matthew Wilcox (Oracle) wrote:
> If we're in memory reclaim, evicting inodes is actually a bad idea.
> The filesystem may need to allocate more memory to evict the inode
> than it will free by evicting the inode.  It's better to defer
> evicting the inode until a workqueue has time to run.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/inode.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d882b0fc787..fe7899cdd50c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -854,6 +854,34 @@ static void dispose_list(struct list_head *head)
>  	}
>  }
>  
> +static DEFINE_SPINLOCK(deferred_inode_lock);
> +static LIST_HEAD(deferred_inode_list);
> +
> +static void dispose_inodes_wq(struct work_struct *work)
> +{
> +	LIST_HEAD(dispose);
> +
> +	spin_lock_irq(&deferred_inode_lock);
> +	list_splice_init(&deferred_inode_list, &dispose);
> +	spin_unlock_irq(&deferred_inode_lock);
> +
> +	dispose_list(&dispose);
> +}
> +
> +static DECLARE_WORK(dispose_inode_work, dispose_inodes_wq);
> +
> +static void deferred_dispose_inodes(struct list_head *inodes)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&deferred_inode_lock, flags);
> +	list_splice_tail(inodes, &deferred_inode_list);
> +	spin_unlock_irqrestore(&deferred_inode_lock, flags);
> +
> +	printk("deferring some inodes\n");
> +	schedule_work(&dispose_inode_work);
> +}
> +
>  /**
>   * evict_inodes	- evict all evictable inodes for a superblock
>   * @sb:		superblock to operate on
> @@ -897,13 +925,17 @@ void evict_inodes(struct super_block *sb)

As mentioned in my reply to cover letter this is a wrong function to patch.
You'll never see this triggering, unless kswapd can trigger fs unmount :)

I guess let's keep the discussion about the approach threaded behind the
cover letter so I won't comment more here.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH 2/2] fs: Defer evicting inodes to a workqueue

[Mateusz Guzik]

On Wed, Sep 24, 2025 at 10:09:57AM +0100, Matthew Wilcox (Oracle) wrote:
> If we're in memory reclaim, evicting inodes is actually a bad idea.
> The filesystem may need to allocate more memory to evict the inode
> than it will free by evicting the inode.  It's better to defer
> evicting the inode until a workqueue has time to run.
> 

There is a bug in the patch (noted below).

As for whether the idea works on paper I have no idea.

> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/inode.c | 36 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 9d882b0fc787..fe7899cdd50c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -854,6 +854,34 @@ static void dispose_list(struct list_head *head)
>  	}
>  }
>  
> +static DEFINE_SPINLOCK(deferred_inode_lock);
> +static LIST_HEAD(deferred_inode_list);
> +
> +static void dispose_inodes_wq(struct work_struct *work)
> +{
> +	LIST_HEAD(dispose);
> +
> +	spin_lock_irq(&deferred_inode_lock);
> +	list_splice_init(&deferred_inode_list, &dispose);
> +	spin_unlock_irq(&deferred_inode_lock);
> +
> +	dispose_list(&dispose);
> +}

As a side note there is lockless machinery to do this (see
__fput_deferred), but perhaps this is just easier to look at.

> @@ -897,13 +925,17 @@ void evict_inodes(struct super_block *sb)
>  		if (need_resched()) {
>  			spin_unlock(&sb->s_inode_list_lock);
>  			cond_resched();
> -			dispose_list(&dispose);
> +			if (!in_reclaim())
> +				dispose_list(&dispose);
>  			goto again;
>  		}
>  	}
>  	spin_unlock(&sb->s_inode_list_lock);
>  
> -	dispose_list(&dispose);
> +	if (!in_reclaim())
> +		dispose_list(&dispose);
> +	else
> +		deferred_dispose_inodes(&dispose);
>  }
>  EXPORT_SYMBOL_GPL(evict_inodes);

I don't think this addresses the problems you linked. For example in the
first crash:
https://lore.kernel.org/all/CALm_T+3j+dyK02UgPiv9z0f1oj-HM63oxhsB0JF9gVAjeVfm1Q@mail.gmail.com/

evict_inodes() is only used on fs teardown AFAICS and I presume this
never executes within memory reclamation.

The trace at hand sports prune_icache_sb() -> inode_lru_isolate();
dispose_list() -> evict(), so I don't think your patch changes the
behavior in that case.

As in, I take it you wanted to patch prune_icache_sb() instead.

Even with this corrected there is a bug I don't blame you for:
weirdly enough the VFS layer expects all inodes to be sorted out in
generic_shutdown_super(). with the stock patch, assuming evict_inodes()
deferred anything and was called from something else than unmount, it
would not wait for deferred processing to finish. Same for deferred
processing in prune_icache_sb() or whatever else-non-unmount.

So for this to work you also need to patch unmount to stall waiting for
this bit to be done with all the inodes.

Re: [RFC PATCH 0/2] Defer evicting inodes to a workqueue

[Jan Kara]

On Wed 24-09-25 10:09:55, Matthew Wilcox (Oracle) wrote:
> Evicting an inode is a complex process which may require allocating
> memory, running a transaction, etc, etc.  Doing it as part of reclaim
> is a bad idea and leads to hard-to-reproduce bug reports.  This pair of
> patches defers it to a workqueue if we're in reclaim.
> 
> Bugs:
> https://lore.kernel.org/all/CALm_T+3j+dyK02UgPiv9z0f1oj-HM63oxhsB0JF9gVAjeVfm1Q@mail.gmail.com/
> https://lore.kernel.org/all/CALm_T+2cEDUJvjh6Lv+6Mg9QJxGBVAHu-CY+okQgh-emWa7-1A@mail.gmail.com/
> https://lore.kernel.org/all/20250326105914.3803197-1-matt@readmodwrite.com/
> 
> I don't know if this is a good idea, to be honest.  We're kind of lying
> to reclaim by pretending that we've freed N inodes when actually we've
> just queued them for eviction.  On the other hand, XFS has been doing
> it for years, so perhaps it's not important.

Well, I guess as soon as your page allocation forward progress depends on
inode cache eviction (or any other slab cache in fact), you are trashing
hard and you are generally in a very bad situation (TM). So I as long as
you can give dentry + inode cache a kick "hey, you're too big for the
workload" and we'll shrink it within some reasonable time, we should be
mostly OK. But also note that XFS inode reclaim has various throttling
mechanisms so that we don't queue unbounded amount of work - because some
workloads may generate insane amounts of inodes and you eventually want to
throttle inode allocation rate to the rate you can reclaim them and so far
there's no other mechanism for that than blocking reclaim.

> I think the real solution here is to convert the Linux VFS to use the
> same inode lifecycle as IRIX, but I don't fully understand the downsides
> of that approach.  One major pro of course is that XFS wouldn't have to
> work around the Linux VFS any more.
> 
> I do wonder if a better approach might be:
> 
> +++ b/fs/inode.c
> @@ -883,6 +883,10 @@ void evict_inodes(struct super_block *sb)

Why evict_inodes? I think you want prune_icache_sb() -> inode_lru_isolate()?

>                         spin_unlock(&inode->i_lock);
>                         continue;
>                 }
> +               if (in_reclaim() && (inode->i_state & I_DIRTY_ALL)) {

Also I_DIRTY_ALL is far from matching all reasons why fs may take long to
reclaim the inode. There may be block preallocation to trim or some
journalling machinery cleanup to do etc...

> +                       spin_unlock(&inode->i_lock);
> +                       continue;
> +               }
>  
>                 inode->i_state |= I_FREEING;
>                 inode_lru_list_del(inode);


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH 0/2] Defer evicting inodes to a workqueue

[Joanne Koong]

On Wed, Sep 24, 2025 at 4:35 AM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 24-09-25 10:09:55, Matthew Wilcox (Oracle) wrote:
> > +++ b/fs/inode.c
> > @@ -883,6 +883,10 @@ void evict_inodes(struct super_block *sb)
>
> Why evict_inodes? I think you want prune_icache_sb() -> inode_lru_isolate()?

I think prune_dcache_sb() can lead to inode eviction in reclaim as
well (eg prune_dcache_sb() -> shrink_dentry_list() -> shrink_kill() ->
__dentry_kill() -> dentry_unlink_inode() -> evict()), so maybe this
should also be done there too.


Thanks,
Joanne
>
> >                         spin_unlock(&inode->i_lock);
> >                         continue;
> >                 }
> > +               if (in_reclaim() && (inode->i_state & I_DIRTY_ALL)) {

Re: [RFC PATCH 0/2] Defer evicting inodes to a workqueue

[Jan Kara]

On Wed 24-09-25 10:45:35, Joanne Koong wrote:
> On Wed, Sep 24, 2025 at 4:35 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 24-09-25 10:09:55, Matthew Wilcox (Oracle) wrote:
> > > +++ b/fs/inode.c
> > > @@ -883,6 +883,10 @@ void evict_inodes(struct super_block *sb)
> >
> > Why evict_inodes? I think you want prune_icache_sb() -> inode_lru_isolate()?
> 
> I think prune_dcache_sb() can lead to inode eviction in reclaim as
> well (eg prune_dcache_sb() -> shrink_dentry_list() -> shrink_kill() ->
> __dentry_kill() -> dentry_unlink_inode() -> evict()), so maybe this
> should also be done there too.

Well, this will end up removing the inode only in some corner cases (like
inode being deleted or when I_DONTCACHE flags is set. But in the most
common case iput_final() will just insert the inode into the LRU list.
But you're right that if we want kind of guarantee that reclaim won't block
on inode eviction, then this path should be handled as well.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC PATCH 0/2] Defer evicting inodes to a workqueue

[Dave Chinner]

[sorry I'm late on this, been working on other things]

On Wed, Sep 24, 2025 at 10:09:55AM +0100, Matthew Wilcox (Oracle) wrote:
> Evicting an inode is a complex process which may require allocating
> memory, running a transaction, etc, etc.  Doing it as part of reclaim
> is a bad idea and leads to hard-to-reproduce bug reports.  This pair of
> patches defers it to a workqueue if we're in reclaim.
> 
> Bugs:
> https://lore.kernel.org/all/CALm_T+3j+dyK02UgPiv9z0f1oj-HM63oxhsB0JF9gVAjeVfm1Q@mail.gmail.com/
> https://lore.kernel.org/all/CALm_T+2cEDUJvjh6Lv+6Mg9QJxGBVAHu-CY+okQgh-emWa7-1A@mail.gmail.com/
> https://lore.kernel.org/all/20250326105914.3803197-1-matt@readmodwrite.com/

So eviction running filesystem shrinkers that then do blocking IO,
folio operations and/or memory allocation and triggering warnings?

Seems like a problem GFP_NOFS was invented to solve, yes?

> I don't know if this is a good idea, to be honest.  We're kind of lying
> to reclaim by pretending that we've freed N inodes when actually we've
> just queued them for eviction.  On the other hand, XFS has been doing
> it for years, so perhaps it's not important.

tl;dr: we aren't lying, but because most people don't understand how
shrinker-based reclaim work is accounted for it can appear that way.

Long story:

memory relcaim (vmscan.c) does not track reclaim progress via the
number of objects the shrinkers report as freed. It tracks reclaim
progress via the amount of memory freed by the shrinker task. This
is what mm_account_reclaimed_pages() does, and it is implicit in
freeing operations as it is called from the slab code when it frees
the backing memory for a slab cache.

For other shrinkers that manually free memory, they have direct
calls to this function to record memory freeing. e.g. in the XFS
buffer cache when the shrinker disposes of a buffer and calls
xfs_buf_free().

This is a completely separate mechanism to the shrinker work
equalisation mechanism that uses object counts to apply the same
pressure to all shrinkable caches in the system. The shrinkers need
to scan a specific number of objects, and return a count based on
the progress they made. This returned count is ignored by the memory
reclaim code, it is only used as measure of progress being made
in certain siutations (e.g. drop_caches) and so is meaningless in
the course of this discussion.

IOWs, all the shrinker has to do is scan that number of objects for
freeing. It does not have to free them, nor does the returned
"objects freed" imply that any memory was actually freed.

Indeed, the VFS does lazy LRU removal, so it's common for the inode
cache shrinker to remove referenced objects from the LRU and not
free them. In that case, the reclaimable object count goes down, but
the freed count does not go up. The cache got "smaller", but we
didn't free anything. This is perfectly fine.

Indeed, in the case of the superblock shrinker, the "freed" count
tracks how many objects had their VFS lifecycle terminated (e.g.
indoes that were evicted), but this does not imply that the objects
were actually freed. They are simply no longer tracked as freeable
cached objects by the VFS caches.

Why is this distinction important? Remember what I said above about
mm_account_reclaimed_pages() accounting actually freed memory? Now
consider that dentries and inodes are RCU freed.

What does that imply about the superblock shrinker actually freeing
memory?

What this means is that the superblock shrinker -never- frees inodes
or dentry objects directly. The memory being freed is -never-
accounted to the shrinker, because the shrinker task has to schedule
before an RCU grace period can expire and run the RCU callbacks to
free the objects.

IOWs, shrinkers are intended to terminate the life cycle of cached
objects, but the life cycle of the allocated object can (and does)
extend beyond the shrinker that terminates the life cycle.

In the case of XFS, the VFS inode is embedded in the XFS inode, and
the VFS lifecycle is a subset of the XFS inode lifecycle. When
the VFS inode is evicted, it's lifecycle is terminated, but it still
a valid, tracked object at the XFS level. Indeed, XFs puts clean
inodes straight back on the internal reclaim list and accounts for
it as reclaimable. So the number of objects the superblock shrinker
is tracking does not actually go down - it is now tracked via the
sb->s_op->nr_cached_objects(sb, sc) callouts in the superblock
shrinker as a reclaimable object.

If the inode needs eviction work, (i.e. inodegc) it doesn't
immediately get re-accounted as a reclaimable object. We do the
work, then once the inode is in a reclaimable state, we put it on
the reclaim queue and account for it at that point.

Put simply: XFS is not "lying" about anything to the
mm/reclaim/shrinker subystems. We are simply taking advantage of the
fact that the superblock shrinker always frees the VFS inodes
asynchronously via RCU. Hence there is always a disconnect between
between VFS object life cycle termination and the object being
freed. XFS inserts it's own "disconnection processing" into that
gap, and actually lets the superblock shrinker track those objects.

> I think the real solution here is to convert the Linux VFS to use the
> same inode lifecycle as IRIX,

This has nothing to do with Irix. I introduced background inode
freeing to XFS in 2011 to avoid the problems of having to wait for
the inode to be fully clean in evict() context. This introduced some
other issues that I solved in ~2020 with non-blocking inode reclaim
processing. And in 2021, we finally moved all the transactional
modifications out of the evict() path with the background inodegc
infrastructure I wrote.

All of this is possible because shrinkers are simply a mechanism for
terminating the life cycle of a given object and there is absolutely
no requirement for life cycle termination to free any memory.
Indeed, they may release references to other resources that pin
memory (e.g. dentries pin inodes) and so memory being freed might
actually be several steps removed...

> but I don't fully understand the downsides
> of that approach.  One major pro of course is that XFS wouldn't have to
> work around the Linux VFS any more.

Not true - this stuff is way, way more complex than you imply.

On of the key factors in Josef's active/passive inode reference
infrastructure is that it could allow us to bring the "unreferenced"
part of the XFS inode lifecycle (i.e. everything that happens after
eviction) up into the VFS via passive VFS inode object references.

And at that point, could bring VFS active reference eviction
processing up into the VFS as an async deferal mechanism. But it
would also require us to bring the XFS inode cache VFS inode
recycling code as we could now get cache hits on a passively
referenced VFS inode which then needs the active VFS inodes state
reinstantiated.

There's other interactions as we'd need to lift as well, because
experience has proven that pushing eviction off to a workqueue will
create performance regressions.  e.g. Try pushing hundreds of
thousands of inodes a second across dozens of CPUs through a single
workqueue (e.g. highly concurrent find across dozens of directories
under memory pressure).

Then we have to throttle the number of inodes you defer for
eviction. Once you have many thousands deferred, it can take a long
time for the workqueue to catch up, especially if each inode has to
free many, many extents. Unbounded queues are bad, especially when
each unit of queued work can largely be unbounded, too. (e.g.
eviction processing after an unlink of a file with tens of millions
of extents).

IOWs, there's a reason the XFS inodegc queuing is complex - it uses
bound depth per-cpu lockless lists and workqueues for the eviction
processing deferral queue. Avoiding performance impacts due to adding
two context switches to every inode eviction is not simple.

Async inode eviction also has interactions with freeze - what
happens when you have dirty inodes or inodes that require dirtying
to evict queued for eviction and then the fs is frozen? We can't
process those evictions whilst the fs is frozen - we can't modify
anything. Hence there is the possibility that every eviction worker
task can get stuck waiting for thaw, and now inode cache reclaim is
effectively stuck until a thaw occurs....

There is also user visible interactions with space usage (e.g. df vs
rm). Half of the unlink work is done on eviction (i.e. when the last
ref is dropped) and so space isn't freed until eviction processing
is complete. Hence if you run a large rm -rf, it queues all the
inode and extent freeing work to the background queues. From the
user perspective, when the rm -rf completes the directory structure
is gone, but the space hasn't been completely freed yet. It may take
minutes for the space to be freed.

So, yeah, async inode eviction is anything but simple, and from
previous attempts to lift this stuff out of XFS to the VFS, I came
to the conclusion the only way to do it sanely was to convert the
VFS inodes to use active/passive reference counting so that the VFS
could guarantee that the inode was truly unreferenced by the VFS
before cache eviction occurs....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com