[v1] elevator: do not request_module if elevator exists

[PATCH] elevator: do not request_module if elevator exists

Posted by Breno Leitao 1 month, 2 weeks ago

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")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 block/elevator.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/elevator.c b/block/elevator.c
index 4122026b11f1..1904e217505a 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -709,13 +709,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] elevator: do not request_module if elevator exists

Posted by Christoph Hellwig 1 month, 2 weeks ago

On Thu, Oct 10, 2024 at 07:15:08AM -0700, 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")
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  block/elevator.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/elevator.c b/block/elevator.c
> index 4122026b11f1..1904e217505a 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -709,13 +709,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))

__elevator_find needs to be called with elv_list_lock.

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

Posted by Jens Axboe 1 month, 2 weeks ago

On 10/11/24 2:24 AM, Christoph Hellwig wrote:
>> diff --git a/block/elevator.c b/block/elevator.c
>> index 4122026b11f1..1904e217505a 100644
>> --- a/block/elevator.c
>> +++ b/block/elevator.c
>> @@ -709,13 +709,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))
> 
> __elevator_find needs to be called with elv_list_lock.

Doh yes. Breno, I just dropped it for now, just send a v2.

-- 
Jens Axboe

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

Posted by Breno Leitao 1 month, 2 weeks ago

Hello Jens,

On Fri, Oct 11, 2024 at 07:12:43AM -0600, Jens Axboe wrote:
> On 10/11/24 2:24 AM, Christoph Hellwig wrote:
> >> diff --git a/block/elevator.c b/block/elevator.c
> >> index 4122026b11f1..1904e217505a 100644
> >> --- a/block/elevator.c
> >> +++ b/block/elevator.c
> >> @@ -709,13 +709,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))
> > 
> > __elevator_find needs to be called with elv_list_lock.
> 
> Doh yes. Breno, I just dropped it for now, just send a v2.

Sure, I will be sending soon. Sorry for not finding it earlier.

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

Posted by Breno Leitao 1 month, 2 weeks ago

Hello Christoph,

On Fri, Oct 11, 2024 at 01:24:15AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 10, 2024 at 07:15:08AM -0700, 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")
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  block/elevator.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/elevator.c b/block/elevator.c
> > index 4122026b11f1..1904e217505a 100644
> > --- a/block/elevator.c
> > +++ b/block/elevator.c
> > @@ -709,13 +709,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))
> 
> __elevator_find needs to be called with elv_list_lock.

That is right. Thanks for the heads-up.

I will send a fix soon.

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

Posted by Jens Axboe 1 month, 2 weeks ago

On Thu, 10 Oct 2024 07:15:08 -0700, 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.
> 
> [...]

Applied, thanks!

[1/1] elevator: do not request_module if elevator exists
      commit: 822138bfd69ba93e240dc3663ad719cd8c25d1fa

Best regards,
-- 
Jens Axboe