Re: [PATCH] xfs: using mutex instead of semaphore for xfs_buf_lock()

Jinliang Zheng
2025-01-15T12:05:27Z

On Wed, 15 Jan 2025 11:28:54 +1100, Dave Chinner wrote:
> On Fri, Dec 20, 2024 at 01:16:29AM +0800, Jinliang Zheng wrote:
> > xfs_buf uses a semaphore for mutual exclusion, and its count value
> > is initialized to 1, which is equivalent to a mutex.
> >
> > However, mutex->owner can provide more information when analyzing
> > vmcore, making it easier for us to identify which task currently
> > holds the lock.
>
> However, the buffer lock also protects the buffer state and contents
> whilst IO id being performed and it *is not owned by any task*.
>
> A single lock cycle for a buffer can pass through multiple tasks
> before being unlocked in a different task to that which locked it:
>
> p0 <intr> <kworker>
> xfs_buf_lock()
> ...
> <submitted for async io>
> <wait for IO completion>
> .....
> <io completion>
> queued to workqueue
> .....
> perform IO completion
> xfs_buf_unlock()
>
>
> IOWs, the buffer lock here prevents any other task from accessing
> and modifying the contents/state of the buffer until the IO in
> flight is completed. i.e. the buffer contents are guaranteed to be
> stable during write IO, and unreadable when uninitialised during
> read IO....

Yes.

>
> i.e. the locking model used by xfs_buf objects is incompatible with
> the single-owner-task critical section model implemented by
> mutexes...
>

Yes, from a model perspective.

This patch is proposed for two reasons:
1. The maximum count of the xfs_buf->b_sema is 1, which means that only one
kernel code path can hold it at the same time. From this perspective,
changing it to mutex will not have any functional impact.
2. When troubleshooting the hungtask of xfs, sometimes it is necessary to
locate who has acquired the lock. Although, as you said, xfs_buf->b_sema
will flow to other kernel code paths after being down(), it is also helpful
to know which kernel code path locked it first.

Haha, that's just my thought. If you think there is really no need to know who
called the down() on xfs_buf->b_sema, please just ignore this patch.

Thank you.
Jinliang Zheng

> -Dave.
> --
> Dave Chinner
> david@fromorbit.com