Re: [PATCH] fs/namespace: Boost the mount_lock.lock owner instead of spinning on PREEMPT_RT.

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2021-10-26 23:06:21 [+0200], Christian Brauner wrote:
> I'm not an expert in real-time locking but this solution is way less
> intrusive and easier to explain and understand than the first version.
> Based on how I understand priority inheritance this solution seems good.
> 
> The scenario that we seem to mostly worry is:
> Let's assume a low-priority task A is holding lock_mount_hash() and
> writes MNT_WRITE_HOLD to mnt->mnt_flags. Another high-priority task B is
> spinning waiting for MNT_WRITE_HOLD to be cleared.
> On rt kernels task A could be scheduled away causing the high-priority
> task B to spin for a long time. However, if we force the high-priority

s/for a long time/indefinitely

> task B to grab lock_mount_hash() we thereby priority boost low-priorty
> task A causing it to relinquish the lock quickly preventing task B from
> spinning for a long time.

Yes. Task B goes idle, Task A continues with B's priority until 
unlock_mount_hash(). After unlock A gets its old priority back and B
continues.

Side note: Because task A is holding a spinlock_t (lock_mount_hash()) it
can't be moved to another CPU (other reasons). Therefore if task B is on
the same CPU as A with higher priority then the scheduler can't move A
away and won't move B either because it is running. So the system locks
up.

> Under the assumption I got this right:
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Thanks.

> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -343,8 +343,24 @@ int __mnt_want_write(struct vfsmount *m)
> >  	 * incremented count after it has set MNT_WRITE_HOLD.
> >  	 */
> >  	smp_mb();
> > -	while (READ_ONCE(mnt->mnt.mnt_flags) & MNT_WRITE_HOLD)
> > -		cpu_relax();
> > +	might_lock(&mount_lock.lock);
> > +	while (READ_ONCE(mnt->mnt.mnt_flags) & MNT_WRITE_HOLD) {
> > +		if (!IS_ENABLED(CONFIG_PREEMPT_RT)) {
> > +			cpu_relax();
> 
> IS_ENABLED will have the same effect as using ifdef, i.e. compiling the
> irrelevant branch out, I hope.

Yes. It turns into if (0) or if (1) leading to an elimination of one of
the branches.

Sebastian