On Mon, Jan 20, 2025 at 06:34:11PM +0530, Nilay Shroff wrote:

The request queue uses ->sysfs_dir_lock for protecting the addition/
deletion of kobject entries under sysfs while we register/unregister
blk-mq. However kobject addition/deletion is already protected with
kernfs/sysfs internal synchronization primitives. So use of q->sysfs_
dir_lock seems redundant.

From the pure sysfs perspective, yes.  The weird thing with block
layer sysfs is that unregistration/registration can happen at weird
times, though.

Moreover, q->sysfs_dir_lock is also used at few other callsites along
with q->sysfs_lock for protecting the addition/deletion of kojects.
One such example is when we register with sysfs a set of independent
access ranges for a disk. Here as well we could get rid off q->sysfs_
dir_lock and only use q->sysfs_lock.

The only variable which q->sysfs_dir_lock appears to protect is q->
mq_sysfs_init_done which is set/unset while registering/unregistering
blk-mq with sysfs. But this variable could be easily converted to an
atomic type and used safely without using q->sysfs_dir_lock.

So sysfs_dir_lock absolutely should go.  OTOH relying more on sysfs_lock
is a bad idea, as that is also serialied with the attributes, which
again on a pure sysfs basis isn't needed.  Given that you don't add
new critical sections for it this should be fine, though.

So with this patch we remove q->sysfs_dir_lock from each callsite
and also make q->mq_sysfs_init_done an atomic variable.

Using an atomic_t for a single variable is usually not a good idea.
Let's take a step back and look at what mq_sysfs_init_done is trying
to do.

It is set by blk_mq_sysfs_register, which is called by
blk_register_queue. before marking the queue registered and setting the
disk live.  It is cleared in blk_mq_sysfs_unregister called from
blk_unregister_queue just after clearing the queue registered bit.

So maybe we could do something with the queue registered bit, although
eventually I'd like to kill that as well, but either way we need
to explain how the flag prevents the nr_hw_queues update racing with
disk addition/removal.  AFAICS we're not completely getting this
right even right now.  We'd probably need to hold tag_list_lock
over registering and unregistering the hctx sysfs files.