Re: [PATCHv2 3/6] block: Introduce a dedicated lock for protecting queue elevator updates

[Christoph Hellwig <hch@lst.de>]

On Tue, Feb 18, 2025 at 01:58:56PM +0530, Nilay Shroff wrote:
> As the scope of q->sysfs_lock is not well-defined, its misuse has
> resulted in numerous lockdep warnings. To resolve this, replace q->
> sysfs_lock with a new dedicated q->elevator_lock, which will be
> exclusively used to protect elevator switching and updates.

Well, it's not replacing q->sysfs_lock as that is still around.
It changes some data to now be protected by elevator_lock instead,
and you should spell out which, preferably in a comment next to the
elevator_lock definition (I should have done the same for limits_lock
despite the trivial applicability to the limits field).

>  	/* q->elevator needs protection from ->sysfs_lock */
> -	mutex_lock(&q->sysfs_lock);
> +	mutex_lock(&q->elevator_lock);

Well, the above comment is no trivially wrong.

>  
>  	/* the check has to be done with holding sysfs_lock */

Same for this one.

They could probably go away with the proper comments near elevator_lock
itself.

>  static ssize_t queue_requests_show(struct gendisk *disk, char *page)
>  {
> -	return queue_var_show(disk->queue->nr_requests, page);
> +	int ret;
> +
> +	mutex_lock(&disk->queue->sysfs_lock);
> +	ret = queue_var_show(disk->queue->nr_requests, page);
> +	mutex_unlock(&disk->queue->sysfs_lock);

This also shifts taking sysfs_lock into the the ->show and ->store
methods.  Which feels like a good thing, but isn't mentioned in the
commit log or directly releate to elevator_lock.  Can you please move
taking the locking into the methods into a preparation patch with a
proper commit log?  Also it's pretty strange the ->store_nolock is
now called with the queue frozen but ->store is not and both are
called without any locks.  So I think part of that prep patch should
also be moving the queue freezing into ->store and do away with
the separate ->store_nolock, and just keep the special ->store_limit.
There's also no more need for ->lock_module when the elevator store
method is called unfrozen and without a lock.

->show and ->show_nolock also are identical now and can be done
away with.

So maybe shifting the freezing and locking into the methods should go
very first, before dropping the lock for the attributes that don't it?