On 4/14/25 6:54 AM, Ming Lei wrote:
> On Sat, Apr 12, 2025 at 12:50:10AM +0530, Nilay Shroff wrote:
>>
>>
>> On 4/10/25 7:00 PM, Ming Lei wrote:
>>> +int elevator_change_done(struct request_queue *q, struct elev_change_ctx *ctx)
>>> +{
>>> + int ret = 0;
>>> +
>>> + if (ctx->old) {
>>> + elv_unregister_queue(q, ctx->old);
>>> + kobject_put(&ctx->old->kobj);
>>> + }
>>> + if (ctx->new) {
>>> + ret = elv_register_queue(q, ctx->new, ctx->uevent);
>>> + if (ret) {
>>> + unsigned memflags = blk_mq_freeze_queue(q);
>>> +
>>> + mutex_lock(&q->elevator_lock);
>>> + elevator_exit(q);
>>> + mutex_unlock(&q->elevator_lock);
>>> + blk_mq_unfreeze_queue(q, memflags);
>>> + }
>>> + }
>>> + return 0;
>>> +}
>>> +
>> We could have sysf elevator attributes simultaneously accessed while this function
>> adds/removes sysfs elevator attributes without any protection. In fact, the show/store
>> methods of elevator attributes runs with e->sysfs_lock held. So it seems moving
>> the above function out of lock protection might cause crash or other side effects?
>
> sysfs/kobject provides such protection, and kobject_del() will drain any
> in-flight attribute access.
>
Okay, so in that case do we now really need e->sysfs_lock protection while accessing
elevator attributes?
Thanks,
--Nilay
