On 2/10/25 1:55 PM, Shinichiro Kawasaki wrote: > On Feb 10, 2025 / 12:31, 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@xxxxxxx> 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@xxxxxxx> >>>>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx> >>>> >>>> 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@xxxxxxxxxxxxx/ > > Thanks Nilay for the information. I applied your patch series on top of > v6.14-rc1 and Ming's. Then I ran block/002. Alas, still the test case fails with > the "WARNING: possible circular locking dependency detected". The lockdep splat > contents has changed as below: > Thanks Shinichiro for trying out the patch! However this patchset is not complete yet. I have to work on few suggestions from Christoph and Ming and then I will re-post the patchset. We're planning to replace q->sysfs_lock with a new dedicated lock for elevator switch case and that should help cut the dependency between q->q_usage_counter(io) and q->sysfs_lock. So I think you need to wait for sometime before the patchest is ready. Thanks, --Nilay
