On Thu, Feb 06, 2025 at 06:52:36PM +0530, Nilay Shroff wrote:
>
>
> On 2/5/25 9:29 PM, Christoph Hellwig wrote:
> > On Wed, Feb 05, 2025 at 08:14:47PM +0530, Nilay Shroff wrote:
> >>
> >> static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
> >> @@ -5006,8 +5008,10 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
> >> return;
> >>
> >> memflags = memalloc_noio_save();
> >> - list_for_each_entry(q, &set->tag_list, tag_set_list)
> >> + list_for_each_entry(q, &set->tag_list, tag_set_list) {
> >> + mutex_lock(&q->sysfs_lock);
> >
> > This now means we hold up to number of request queues sysfs_lock
> > at the same time. I doubt lockdep will be happy about this.
> > Did you test this patch with a multi-namespace nvme device or
> > a multi-LU per host SCSI setup?
> >
> Yeah I tested with a multi namespace NVMe disk and lockdep didn't
> complain. Agreed we need to hold up q->sysfs_lock for multiple
> request queues at the same time and that may not be elegant, but
> looking at the mess in __blk_mq_update_nr_hw_queues we may not
> have other choice which could help correct the lock order.
All q->sysfs_lock instance actually shares same lock class, so this way
should have triggered double lock warning, please see mutex_init().
The ->sysfs_lock involved in this patch looks only for sync elevator
switch with reallocating hctxs, so I am wondering why not add new
dedicated lock for this purpose only?
Then we needn't to worry about its dependency with q->q_usage_counter(io)?
Thanks,
Ming
