On 1/22/25 11:58 AM, Christoph Hellwig wrote: > 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. > Yeah that's good idea! Indeed, I think we can remove ->mq_sysfs_init_done and replace it with the QUEUE_FLAG_REGISTERED. > 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. > Agreed, makes sense to hold ->tag_list_lock so that we can contain the race between nr_hw_queue update with registering/unregistering the hctx sysfs files. I'd spin a new patch with above changes and submit. Thank you Christoph for your review and comments! --Nilay
