Re: [PATCH 2/3] block: introduce alloc_sched_data and free_sched_data elevator methods

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

On Mon, Oct 27, 2025 at 11:08:13PM +0530, Nilay Shroff wrote:
> Hi Ming,
> 
> On 10/22/25 10:09 AM, Ming Lei wrote:
> > On Thu, Oct 16, 2025 at 11:00:48AM +0530, Nilay Shroff wrote:
> >> The recent lockdep splat [1] highlights a potential deadlock risk
> >> involving ->elevator_lock and ->freeze_lock dependencies on -pcpu_alloc_
> >> mutex. The trace shows that the issue occurs when the Kyber scheduler
> >> allocates dynamic memory for its elevator data during initialization.
> >>
> >> To address this, introduce two new elevator operation callbacks:
> >> ->alloc_sched_data and ->free_sched_data.
> > 
> > This way looks good.
> > 
> >>
> >> When an elevator implements these methods, they are invoked during
> >> scheduler switch before acquiring ->freeze_lock and ->elevator_lock.
> >> This allows safe allocation and deallocation of per-elevator data
> > 
> > This per-elevator data should be very similar with `struct elevator_tags`
> > from block layer viewpoint: both have same lifetime, and follow same
> > allocation constraint(per-cpu lock).
> > 
> > Can we abstract elevator data structure to cover both? Then I guess the
> > code should be more readable & maintainable, what do you think of this way?
> > 
> > One easiest way could be to add 'void *data' into `struct elevator_tags`,
> > just the naming of `elevator_tags` is not generic enough, but might not
> > a big deal.
> > 
> I realized that struct elevator_tags is already a member of struct elevator_queue,
> and we also have a separate void *elevator_data member within the same structure.
> 
> So, adding void *data directly into struct elevator_tags may not be ideal, as it
> would mix two logically distinct resources under a misleading name. Instead, we
> can abstract both — void *elevator_data and struct elevator_tags — into a new
> structure named struct elevator_resources. For instance:
> 
> struct elevator_resources {
>     void *data;
>     struct elevator_tags *et;
> };
> 
> struct elv_change_ctx {
> 	const char *name;
> 	bool no_uevent;
> 	struct elevator_queue *old;
> 	struct elevator_queue *new;
> 	struct elevator_type *type;
> 	struct elevator_resources res;
> };
> 
> I've just sent out PATCHv3 with the above change. Please review and let me know
> if this approach looks good to you.

It is fine to add `struct elevator_resources` for further abstraction, but
you need to abstract related methods too, otherwise the patch 3 still becomes
hard to follow: the existing blk_mq_free_sched_tags can be renamed to
blk_mq_free_sched_resource first, then you can call blk_mq_free_sched_data()
from blk_mq_free_sched_resource() inside only, instead of calling it
following every blk_mq_free_sched_tags().

Same with blk_mq_alloc_sched_tags_batch()/blk_mq_free_sched_tags_batch(),
you can make universal blk_mq_alloc_sched_res_batch/blk_mq_free_sched_res_batch()
to cover both tags & schedule data, and it is easier to extend in future too.




thanks
Ming