On 2/18/25 2:35 PM, Christoph Hellwig wrote:
> 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.
Yes I will update comment, I don't know how I missed updating it :(
>
>>
>> /* 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.
yes I will add proper comment.
>
>> 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?
Yes I would do add proper commit log as suggested
> 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.
You are absolutely correct and that was my original plan to do away with
->store_nolock and ->show_nolock methods in the end however problem arised
due to "read_ahead_kb" attribute. As you know, "read_ahead_kb" requires
update outside of atomic limit APIs (for the reason mentioned in last
patch comment as well as in the cover letter). So we can't move
"read_ahead_kb" into ->store_limit. For updating "read_ahead_kb",
we follow the below sequence:
lock ->limits_lock
lock ->freeze_lock
update bdi->ra_pages
unlock ->freeze_lock
unlock ->limits_lock
As you could see above, we have to take ->limits_lock before ->freeze_lock
while updating ra_pages (doing the opposite would surely trigger lockdep
splat). However for attributes which are grouped under ->store_nolock the
update sequence is:
lock ->freeze_lock
invoke attribute specific store method
unlock ->freeze_lock.
So now if you suggest keeping only ->store and do away with ->store_nolock
method then we have to move feeze-lock inside each attributes' corresponding
store method as we can't keep freeze-lock under ->store in queue_attr_store().
> There's also no more need for ->lock_module when the elevator store
> method is called unfrozen and without a lock.
>
yes agreed, I would remove ->load_module and we can now load module
from elevator ->store method directly before we freeze the queue.
> ->show and ->show_nolock also are identical now and can be done
> away with.
>
Yes this is possible and I just kept it for pairing show_nolock with
store_nolock. But I would remove it.
> 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?
>
Yes this can be done. I will add it when I spin next patchset.
Thanks,
--Nilay
