[PATCH v2] elevator: do not request_module if elevator exists

[PATCH v2] elevator: do not request_module if elevator exists

[Breno Leitao]

Whenever an I/O elevator is changed, the system attempts to load a
module for the new elevator. This occurs regardless of whether the
elevator is already loaded or built directly into the kernel. This
behavior introduces unnecessary overhead and potential issues.

This makes the operation slower, and more error-prone. For instance,
making the problem fixed by [1] visible for users that doesn't even rely
on modules being available through modules.

Do not try to load the ioscheduler if it is already visible.

This change brings two main benefits: it improves the performance of
elevator changes, and it reduces the likelihood of errors occurring
during this process.

[1] Commit e3accac1a976 ("block: Fix elv_iosched_local_module handling of "none" scheduler")
Fixes: 734e1a860312 ("block: Prevent deadlocks when switching elevators")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
Changelog:
 v2:
   * Protecet __elevator_find() by elv_list_lock (Christoph Hellwig)
 v1:
   * https://lore.kernel.org/all/20241010141509.4028059-1-leitao@debian.org/

 block/elevator.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block/elevator.c b/block/elevator.c
index 565807f0b1c7..1ac9be3e47d1 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -106,6 +106,17 @@ static struct elevator_type *__elevator_find(const char *name)
 	return NULL;
 }
 
+static struct elevator_type *elevator_find(const char *name)
+{
+	struct elevator_type *e;
+
+	spin_lock(&elv_list_lock);
+	e = __elevator_find(name);
+	spin_unlock(&elv_list_lock);
+
+	return e;
+}
+
 static struct elevator_type *elevator_find_get(const char *name)
 {
 	struct elevator_type *e;
@@ -708,13 +719,16 @@ int elv_iosched_load_module(struct gendisk *disk, const char *buf,
 			    size_t count)
 {
 	char elevator_name[ELV_NAME_MAX];
+	const char *name;
 
 	if (!elv_support_iosched(disk->queue))
 		return -EOPNOTSUPP;
 
 	strscpy(elevator_name, buf, sizeof(elevator_name));
+	name = strstrip(elevator_name);
 
-	request_module("%s-iosched", strstrip(elevator_name));
+	if (!elevator_find(name))
+		request_module("%s-iosched", name);
 
 	return 0;
 }
-- 
2.43.5

Re: [PATCH v2] elevator: do not request_module if elevator exists

[Jens Axboe]

On 10/11/24 9:49 AM, Breno Leitao wrote:
> Whenever an I/O elevator is changed, the system attempts to load a
> module for the new elevator. This occurs regardless of whether the
> elevator is already loaded or built directly into the kernel. This
> behavior introduces unnecessary overhead and potential issues.
> 
> This makes the operation slower, and more error-prone. For instance,
> making the problem fixed by [1] visible for users that doesn't even rely
> on modules being available through modules.
> 
> Do not try to load the ioscheduler if it is already visible.
> 
> This change brings two main benefits: it improves the performance of
> elevator changes, and it reduces the likelihood of errors occurring
> during this process.
> 
> [1] Commit e3accac1a976 ("block: Fix elv_iosched_local_module handling of "none" scheduler")
> Fixes: 734e1a860312 ("block: Prevent deadlocks when switching elevators")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> Changelog:
>  v2:
>    * Protecet __elevator_find() by elv_list_lock (Christoph Hellwig)
>  v1:
>    * https://lore.kernel.org/all/20241010141509.4028059-1-leitao@debian.org/
> 
>  block/elevator.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/block/elevator.c b/block/elevator.c
> index 565807f0b1c7..1ac9be3e47d1 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -106,6 +106,17 @@ static struct elevator_type *__elevator_find(const char *name)
>  	return NULL;
>  }
>  
> +static struct elevator_type *elevator_find(const char *name)
> +{
> +	struct elevator_type *e;
> +
> +	spin_lock(&elv_list_lock);
> +	e = __elevator_find(name);
> +	spin_unlock(&elv_list_lock);
> +
> +	return e;
> +}

Probably just drop this helper. If it's only used in one spot, then we
don't need to add a helper for it.

-- 
Jens Axboe

Re: [PATCH v2] elevator: do not request_module if elevator exists

[Breno Leitao]

hello Jens,

On Fri, Oct 11, 2024 at 10:02:13AM -0600, Jens Axboe wrote:
> On 10/11/24 9:49 AM, Breno Leitao wrote:
> > Whenever an I/O elevator is changed, the system attempts to load a
> > module for the new elevator. This occurs regardless of whether the
> > elevator is already loaded or built directly into the kernel. This
> > behavior introduces unnecessary overhead and potential issues.
> > 
> > This makes the operation slower, and more error-prone. For instance,
> > making the problem fixed by [1] visible for users that doesn't even rely
> > on modules being available through modules.
> > 
> > Do not try to load the ioscheduler if it is already visible.
> > 
> > This change brings two main benefits: it improves the performance of
> > elevator changes, and it reduces the likelihood of errors occurring
> > during this process.
> > 
> > [1] Commit e3accac1a976 ("block: Fix elv_iosched_local_module handling of "none" scheduler")
> > Fixes: 734e1a860312 ("block: Prevent deadlocks when switching elevators")
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> > Changelog:
> >  v2:
> >    * Protecet __elevator_find() by elv_list_lock (Christoph Hellwig)
> >  v1:
> >    * https://lore.kernel.org/all/20241010141509.4028059-1-leitao@debian.org/
> > 
> >  block/elevator.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/elevator.c b/block/elevator.c
> > index 565807f0b1c7..1ac9be3e47d1 100644
> > --- a/block/elevator.c
> > +++ b/block/elevator.c
> > @@ -106,6 +106,17 @@ static struct elevator_type *__elevator_find(const char *name)
> >  	return NULL;
> >  }
> >  
> > +static struct elevator_type *elevator_find(const char *name)
> > +{
> > +	struct elevator_type *e;
> > +
> > +	spin_lock(&elv_list_lock);
> > +	e = __elevator_find(name);
> > +	spin_unlock(&elv_list_lock);
> > +
> > +	return e;
> > +}
> 
> Probably just drop this helper. If it's only used in one spot, then we
> don't need to add a helper for it.

Sure, let me send a v3 without the helper.

Thanks for the review
--breno