Re: [PATCH v2] fs/resctrl: Fix deadlock for errors during mount

[Luck, Tony]

On Thu, May 07, 2026 at 11:30:53AM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 5/7/26 10:47 AM, Luck, Tony wrote:
> > Reinette,
> > 
> > This looks promising.
> > 
> > I'll split out the missing call to mon_put_kn_priv(); into its own
> > patch before the deadlock fix.
> > 
> > Going to re-run the tests I did before forcing kernfs_get_tree()
> > to fail early without setting new_sb_created, and also late.
> 
> Thank you very much.

Tests pass.

> > 
> >> +/*
> >> + * Temporary forward declaration for testing only. Move functions instead.
> >> + */
> >> +static void resctrl_unmount(void);
> >> +static void mon_put_kn_priv(void);
> > 
> > Question: How much are forward declarations hated? And how to handle
> > this?
> 
> I am not aware of forward declarations being hated and I am not aware of
> documented tip rules about this. Even so, I do find it cleaner if they
> can be avoided. To handle this a prep patch that just moves the code
> without any functional change should work?
> 
> > Moving the functions around in the same patch really obscures the
> > actual change. Is it OK to have a patch to make the functional
> > change including the forward declarations. Then a separate commit
> > that does the re-order (where it is obvious that functions are
> > being picked up and moved without any code changes)?
> 
> My preference would be a prep patch that does the move with new
> capability built on top. It seems unnecessary to me to add a forward
> declaration in one patch just to remove it in a following patch.
> I agree that moving code as part of functional change should be avoided.

OK. I have a three patch series:

1) Pure code move, no changes [while I picked up a block of functions
and moved them earlier, "git show" presents the move in terms of a
different set of functions moving]

2) Add missing mon_put_kn_priv()

3) Fix the mount error deadlock

Internal AI scans have only two complaints. Both appear spurious;

First it says that:

	if (!ctx->kfc.new_sb_created)
		resctrl_unmount();
should be:

	if (ret && !ctx->kfc.new_sb_created)
		resctrl_unmount();

It admits that current code can't return ret == 0 (success) while
failing to create a super block. But argues in some twisted future
that might be possible.

Second:

Code used to call rdt_last_cmd_clear() unconditionally on success or
failure of the mount. Now the call for the end-of-function error path
is gone.

This is true, but it doesn't matter. If the filesystem did not mount
then there is no /sys/fs/resctrl/info/last_cmd_status file to leak an
old error string.

> 
> Reinette

I just need to write up the cover letter.

-Tony