Re: [PATCH 7/7] block: don't grab q->debugfs_mutex

[Ming Lei <ming.lei@redhat.com>]

On Mon, Feb 10, 2025 at 12:31:19PM +0530, Nilay Shroff wrote:
> 
> 
> On 2/10/25 7:12 AM, Ming Lei wrote:
> > On Mon, Feb 10, 2025 at 8:52 AM Shinichiro Kawasaki
> > <shinichiro.kawasaki@wdc.com> wrote:
> >>
> >> On Feb 09, 2025 / 20:20, Ming Lei wrote:
> >>> All block internal state for dealing adding/removing debugfs entries
> >>> have been removed, and debugfs can sync everything for us in fs level,
> >>> so don't grab q->debugfs_mutex for adding/removing block internal debugfs
> >>> entries.
> >>>
> >>> Now q->debugfs_mutex is only used for blktrace, meantime move creating
> >>> queue debugfs dir code out of q->sysfs_lock. Both the two locks are
> >>> connected with queue freeze IO lock.  Then queue freeze IO lock chain
> >>> with debugfs lock is cut.
> >>>
> >>> The following lockdep report can be fixed:
> >>>
> >>> https://lore.kernel.org/linux-block/ougniadskhks7uyxguxihgeuh2pv4yaqv4q3emo4gwuolgzdt6@brotly74p6bs/
> >>>
> >>> Follows contexts which adds/removes debugfs entries:
> >>>
> >>> - update nr_hw_queues
> >>>
> >>> - add/remove disks
> >>>
> >>> - elevator switch
> >>>
> >>> - blktrace
> >>>
> >>> blktrace only adds entries under disk top directory, so we can ignore it,
> >>> because it can only work iff disk is added. Also nothing overlapped with
> >>> the other two contex, blktrace context is fine.
> >>>
> >>> Elevator switch is only allowed after disk is added, so there isn't race
> >>> with add/remove disk. blk_mq_update_nr_hw_queues() always restores to
> >>> previous elevator, so no race between these two. Elevator switch context
> >>> is fine.
> >>>
> >>> So far blk_mq_update_nr_hw_queues() doesn't hold debugfs lock for
> >>> adding/removing hctx entries, there might be race with add/remove disk,
> >>> which is just fine in reality:
> >>>
> >>> - blk_mq_update_nr_hw_queues() is usually for error recovery, and disk
> >>> won't be added/removed at the same time
> >>>
> >>> - even though there is race between the two contexts, it is just fine,
> >>> since hctx won't be freed until queue is dead
> >>>
> >>> - we never see reports in this area without holding debugfs in
> >>> blk_mq_update_nr_hw_queues()
> >>>
> >>> Reported-by: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>
> >> Ming, thank you for this quick action. I applied this series on top of
> >> v6.14-rc1 kernel and ran the block/002 test case. Unfortunately, still if fails
> >> occasionally with the lockdep "WARNING: possible circular locking dependency
> >> detected" below. Now debugfs_mutex is not reported as one of the dependent
> >> locks, then I think this fix is working as expected. Instead, eq->sysfs_lock
> >> creates similar dependency. My mere guess is that this patch avoids one
> >> dependency, but still another dependency is left.
> > 
> > Indeed, this patch cuts dependency on both q->sysfs_lock and q->debugfs_lock,
> Glad to see that with this patch we're able to cut the dependency between
> q->sysfs_lock and q->debugfs_lock.
> 
> > but elevator ->sysfs_lock isn't covered, :-(
> > 
> I believe that shall be fixed with the current effort undergoing here:
> https://lore.kernel.org/all/20250205144506.663819-1-nilay@linux.ibm.com/

I guess it isn't, your patches don't cover elevator_queue->sysfs_lock...


Thanks,
Ming