Re: [PATCH 11/15] block: move elv_register[unregister]_queue out of elevator_lock

[Ming Lei <ming.lei@redhat.com>]

On Tue, Apr 15, 2025 at 03:09:12PM +0530, Nilay Shroff wrote:
> 
> 
> 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? 

Yeah, I think so, elevator_exit() is always called after elevator kobject
is deleted.

However, this patchset moves elv_unregister_queue() after blk_mq_exit_sched(),
we may need this lock for failing elevator's attribute ->show() & ->store()
by adding one '->exiting' flag to 'struct elevator_queue'.

Thanks,
Ming