On Tue, Apr 15, 2025 at 03:07:18PM +0530, Nilay Shroff wrote:
>
>
> On 4/14/25 7:12 AM, Ming Lei wrote:
> > On Fri, Apr 11, 2025 at 12:27:17AM +0530, Nilay Shroff wrote:
> >>
> >>
> >> On 4/10/25 7:00 PM, Ming Lei wrote:
> >>> Move debugfs/sysfs register out of freezing queue in
> >>> __blk_mq_update_nr_hw_queues(), so that the following lockdep dependency
> >>> can be killed:
> >>>
> >>> #2 (&q->q_usage_counter(io)#16){++++}-{0:0}:
> >>> #1 (fs_reclaim){+.+.}-{0:0}:
> >>> #0 (&sb->s_type->i_mutex_key#3){+.+.}-{4:4}: //debugfs
> >>>
> >>> And registering/un-registering debugfs/sysfs does not require queue to be
> >>> frozen.
> >>>
> >>> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> >>> ---
> >>> block/blk-mq.c | 20 ++++++++++----------
> >>> 1 file changed, 10 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/block/blk-mq.c b/block/blk-mq.c
> >>> index 7219b01764da..0fb72a698d77 100644
> >>> --- a/block/blk-mq.c
> >>> +++ b/block/blk-mq.c
> >>> @@ -4947,15 +4947,15 @@ static void __blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set,
> >>> if (set->nr_maps == 1 && nr_hw_queues == set->nr_hw_queues)
> >>> return;
> >>>
> >>> - memflags = memalloc_noio_save();
> >>> - list_for_each_entry(q, &set->tag_list, tag_set_list)
> >>> - blk_mq_freeze_queue_nomemsave(q);
> >>> -
> >>> list_for_each_entry(q, &set->tag_list, tag_set_list) {
> >>> blk_mq_debugfs_unregister_hctxs(q);
> >>> blk_mq_sysfs_unregister_hctxs(q);
> >>> }
> >> As we removed hctx sysfs protection while un-registering it, this might
> >> cause crash or other side-effect if simultaneously these sysfs attributes
> >> are accessed. The read access of these attributes are still protected
> >> using ->elevator_lock.
> >
> > The ->elevator_lock in ->show() is useless except for reading the elevator
> > internal data(sched tags, requests, ...), even for reading elevator data,
> > it should have been relying on elevator reference, instead of lock, but
> > that is another topic & improvement in future.
> >
> > Also this patch does _not_ change ->elevator_lock for above debugfs/sysfs
> > unregistering, does it? It is always done without holding ->elevator_lock.
> > Also ->show() does not require ->q_usage_counter too.
> >
> > As I mentioned, kobject/sysfs provides protection between ->show()/->store()
> > and kobject_del(), isn't it the reason why you want to remove ->sys_lock?
> >
> > https://lore.kernel.org/linux-block/20250226124006.1593985-1-nilay@xxxxxxxxxxxxx/
> >
> Yes you were correct, that was the reason we wanted to remove ->sysfs_lock.
> However for these particular hctx sysfs attributes (nr_tags and nr_reserved_tags)
> could be updated simultaneously from another blk-mq sysfs attribute named nr_requests.
> Hence IMO, the default protection provided by sysfs/kernfs may not be sufficient and
> so we need to protect those attributes using ->elevator_lock.
Yes, what is why this patchset doesn't kill more ->elevator_lock uses, such
as, the uses in blk-mq-debugs, update_nr_requests, but many of them can be
replaced with grabbing elevator reference.
But with/without this patch, the touched register/unregisger code does not
require ->elevator_lock:
blk_mq_debugfs_unregister_hctxs(q);
blk_mq_sysfs_unregister_hctxs(q);
so I don't understand why you argue here about ->elevator_lock use?
>
> Consider this case: While blk_mq_update_nr_hw_queues removes hctx attributes,
> and simultaneously if nr_requests is also updating num of tags, would that not
> cause any side effect?
Why is updating nr_requests related with removing hctx attributes?
Can you explain the side effect in details?
> Maybe we also want to protect blk_mq_update_nr_requests
> with srcu read lock (set->update_nr_hwq_srcu) so that it couldn't run while
> blk_mq_update_nr_hw_queues is in progress?
Yeah, agree, and it can be one new patch for covering race between
blk_mq_update_nr_requests and blk_mq_update_nr_hw_queues, the point is just
that nr_hw_queues is being changed, and not related with removing hctx
attributes, IMO.
Thanks,
Ming
