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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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






[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux