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

[Chen, Yu C]

Hi Reinette,
On 5/12/2026 10:34 PM, Reinette Chatre wrote:
> Hi Chenyu,
>
> On 5/12/26 12:28 AM, Chen, Yu C wrote:
> > On 5/12/2026 6:53 AM, Reinette Chatre wrote:
> > > Hi Tony,
> > >
> > > On 5/8/26 11:21 AM, Tony Luck wrote:
> >
> > > +     * Obtain reference with locks held to protect against interference
> > > +     * from resctrl_exit().
> > > +    */
> > > +    kernfs_get(rdt_root_kn);
> >
> > [ ... ]
> >
> > > @@ -3130,6 +3144,7 @@ static int rdt_get_tree(struct fs_context *fc)
> > >         */
> > >        if (!ctx->kfc.new_sb_created)
> > >            resctrl_unmount();
> > > +    kernfs_put(rdt_root_kn);
> >
> > I wonder if above should be protected against
> >      cpus_read_lock();
> >      mutex_lock(&rdtgroup_mutex);
> > like kernfs_get()?
>
> It is not obvious to me what this protection would be needed for.
> Do you have a troublesome scenario in mind?
>
> rdt_root_kn is a local copy of rdtgroup_default.kn. The latter is indeed
> protected by the mutex. The reason why the kernfs_get() is protected
> by the mutex is to ensure what rdt_root_kn points to, rdtgroup_default.kn, remains
> accessible after the mutex is dropped. Nothing else modifies rdt_root_kn. I
> understand the appeal of symmetry but it is not clear to me what the extra
> locking is needed for here?

Thanks for the detailed explanation. I now agree there is no need to
protect kernfs_put() with a lock here only for symmetry reason. I
previously thought racing conditions would occur if two code paths
concurrently enter kernfs_put() and target the same data area.
However, since kernfs_put() contains an atomic compare, only one
code path can proceed, making the operation safe.

> Could it perhaps make this flow easier to understand if the kernfs_get() is
> of the mutex protected rdtgroup_default.kn while the kernfs_put() is
> of the local backup copy? For example:
>
> 	/* Ensure root kn remains accessible after mutex is unlocked */
> 	kernfs_get(rdtgroup_default.kn);
> 	/*
> 	 * Make backup of rdtgroup_default.kn just in case one of the
> 	 * following flows (that sets rdtgroup_default.kn to NULL) run after
> 	 * the mutex is unlocked:
> 	 * resctrl_exit()->resctrl_fs_teardown()->rdtgroup_destroy_root()
> 	 * kernfs_get_tree()->deactivate_locked_super()->rdt_kill_sb()->resctrl_unmount()->resctrl_fs_teardown()->rdtgroup_destroy_root()
> 	 * These flows would not actually result in rdtgroup_default.kn
> 	 * being removed thanks to the additional reference.
> 	 /

Yes, this comment is very clear and helpful.

thanks,
Chenyu