[uml-devel] [patch 1/3] uml-ubd-no-empty-queue

[uml-devel] [patch 1/3] uml-ubd-no-empty-queue

[blaisorblade_spam]

Avoid using, in the UBD driver, the elv_queue_empty function. It's for
the block layer only; in fact, the Anticipatory Scheduler can return NULL
with elv_next_request() even if the queue is not empty, because it waits
for the process to send another request before seeking on the disk.

In fact, if (with uml-ubd-any-elevator) we let UBD use any scheduler,
elevator=as will make the UBD driver Oops, if we don't have this patch.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade_spam@yahoo.it>
---

 uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff -puN arch/um/drivers/ubd_kern.c~uml-ubd-no-empty-queue arch/um/drivers/ubd_kern.c
--- uml-linux-2.6.8.1/arch/um/drivers/ubd_kern.c~uml-ubd-no-empty-queue	2004-08-29 14:40:51.313410952 +0200
+++ uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c	2004-08-29 14:40:51.315410648 +0200
@@ -1038,8 +1038,7 @@ static void do_ubd_request(request_queue
 	int err, n;
 
 	if(thread_fd == -1){
-		while(!elv_queue_empty(q)){
-			req = elv_next_request(q);
+		while((req = elv_next_request(q)) != NULL){
 			err = prepare_request(req, &io_req);
 			if(!err){
 				do_io(&io_req);
@@ -1048,9 +1047,8 @@ static void do_ubd_request(request_queue
 		}
 	}
 	else {
-		if(do_ubd || elv_queue_empty(q))
+		if(do_ubd || (req = elv_next_request(q)) == NULL)
 			return;
-		req = elv_next_request(q);
 		err = prepare_request(req, &io_req);
 		if(!err){
 			do_ubd = ubd_handler;
_


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

[patch 1/3] uml-ubd-no-empty-queue

[blaisorblade_spam]

Avoid using, in the UBD driver, the elv_queue_empty function. It's for
the block layer only; in fact, the Anticipatory Scheduler can return NULL
with elv_next_request() even if the queue is not empty, because it waits
for the process to send another request before seeking on the disk.

In fact, if (with uml-ubd-any-elevator) we let UBD use any scheduler,
elevator=as will make the UBD driver Oops, if we don't have this patch.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade_spam@yahoo.it>
---

 uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff -puN arch/um/drivers/ubd_kern.c~uml-ubd-no-empty-queue arch/um/drivers/ubd_kern.c
--- uml-linux-2.6.8.1/arch/um/drivers/ubd_kern.c~uml-ubd-no-empty-queue	2004-08-29 14:40:51.313410952 +0200
+++ uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c	2004-08-29 14:40:51.315410648 +0200
@@ -1038,8 +1038,7 @@ static void do_ubd_request(request_queue
 	int err, n;
 
 	if(thread_fd == -1){
-		while(!elv_queue_empty(q)){
-			req = elv_next_request(q);
+		while((req = elv_next_request(q)) != NULL){
 			err = prepare_request(req, &io_req);
 			if(!err){
 				do_io(&io_req);
@@ -1048,9 +1047,8 @@ static void do_ubd_request(request_queue
 		}
 	}
 	else {
-		if(do_ubd || elv_queue_empty(q))
+		if(do_ubd || (req = elv_next_request(q)) == NULL)
 			return;
-		req = elv_next_request(q);
 		err = prepare_request(req, &io_req);
 		if(!err){
 			do_ubd = ubd_handler;
_

[uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[Andrew Morton]

Please don't use a filename like uml-ubd-no-empty-queue as the Subject:
of your patches.  Please prepare an English-language summary.  See
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

I applied three of these - two got rejects against Linus's current
tree.

Do you have to do this

 -menu "SCSI support"
 +if BROKEN
 +	menu "SCSI support"
  
 -config SCSI

I think you'll find that

	menu "SCSI support"
	depends on BROKEN

works OK.


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [patch 1/3] uml-ubd-no-empty-queue

[Andrew Morton]

Please don't use a filename like uml-ubd-no-empty-queue as the Subject:
of your patches.  Please prepare an English-language summary.  See
http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

I applied three of these - two got rejects against Linus's current
tree.

Do you have to do this

 -menu "SCSI support"
 +if BROKEN
 +	menu "SCSI support"
  
 -config SCSI

I think you'll find that

	menu "SCSI support"
	depends on BROKEN

works OK.

[uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[Jeff Garzik]

Andrew Morton wrote:
> Please don't use a filename like uml-ubd-no-empty-queue as the Subject:
> of your patches.  Please prepare an English-language summary.  See
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt


also

http://linux.yyz.us/patch-format.html



-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [patch 1/3] uml-ubd-no-empty-queue

[Jeff Garzik]

Andrew Morton wrote:
> Please don't use a filename like uml-ubd-no-empty-queue as the Subject:
> of your patches.  Please prepare an English-language summary.  See
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt


also

http://linux.yyz.us/patch-format.html

Re: [uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[BlaisorBlade]

On Monday 06 September 2004 23:26, Andrew Morton wrote:
> Please don't use a filename like uml-ubd-no-empty-queue as the Subject:
> of your patches.  Please prepare an English-language summary.  See
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
Ok, but how can I specify the filename you'll give to the patch?

It would make management between my tree and yours easier for me, if possible.

> I applied three of these - two got rejects against Linus's current
> tree.
>
> Do you have to do this
>
>  -menu "SCSI support"
>  +if BROKEN
>  +	menu "SCSI support"
>
>  -config SCSI
>
> I think you'll find that
>
> 	menu "SCSI support"
> 	depends on BROKEN
>
> works OK.
If you want to add the dependency to SCSI, GENERIC_ISA_DMA and every item in 
arch/um/Kconfig_scsi by hand, you're welcome, but I guess not :-).

Otherwise, please note I bracketed everything with if BROKEN....endif. If you 
don't like the indent, that's not a problem.
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[BlaisorBlade]

On Monday 06 September 2004 23:26, Andrew Morton wrote:
> Please don't use a filename like uml-ubd-no-empty-queue as the Subject:
> of your patches.  Please prepare an English-language summary.  See
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
Ok, but how can I specify the filename you'll give to the patch?

It would make management between my tree and yours easier for me, if possible.

> I applied three of these - two got rejects against Linus's current
> tree.
>
> Do you have to do this
>
>  -menu "SCSI support"
>  +if BROKEN
>  +	menu "SCSI support"
>
>  -config SCSI
>
> I think you'll find that
>
> 	menu "SCSI support"
> 	depends on BROKEN
>
> works OK.
If you want to add the dependency to SCSI, GENERIC_ISA_DMA and every item in 
arch/um/Kconfig_scsi by hand, you're welcome, but I guess not :-).

Otherwise, please note I bracketed everything with if BROKEN....endif. If you 
don't like the indent, that's not a problem.
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729

Re: [uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[Andrew Morton]

BlaisorBlade <blaisorblade_spam@yahoo.it> wrote:
>
> On Monday 06 September 2004 23:26, Andrew Morton wrote:
>  > Please don't use a filename like uml-ubd-no-empty-queue as the Subject:
>  > of your patches.  Please prepare an English-language summary.  See
>  > http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
>  Ok, but how can I specify the filename you'll give to the patch?
> 
>  It would make management between my tree and yours easier for me, if possible.

hm.  By choosing a suitable Subject:, I guess.

Start the Subject with "uml:" and then avoid getting fancy in the rest of
the subject and things should work out OK.  Spaces and other funny
characters are replaced with "-" and underscores are retained.

I use the below piece of ad-hoc revoltium to canonicalise Subject:s into
filenames:

line=$(echo "$line" | tr 'A-Z' 'a-z')
line=$(echo "$line" | sed -e 's/^subject:[ 	]*//')
line=$(echo "$line" | sed -e 's/^fw:[ 	]*//')
line=$(echo "$line" | sed -e 's/^fwd:[ 	]*//')
line=$(echo "$line" | sed -e 's/^aw:[ 	]*//')
line=$(echo "$line" | sed -e 's/^re:[ 	]*//')

line=$(echo "$line" | sed -e 's/^patch//')
line=$(echo "$line" | sed -e "s/['\(\)\<\>\{\}\,\.\\]//g")
line=$(echo "$line" | sed -e "s/[\#\*\&\+\^\!\~\`\:\?\;]//g")
line=$(echo "$line" | sed -e "s/[\$]//g")
line=$(echo "$line" | sed -e 's/"//g')
line=$(echo "$line" | sed -e 's/^[-]*//g')
line=$(echo "$line" | sed -e 's/\[[^]]*\]//g')
line=$(echo "$line" | sed -e 's/[ 	]*\[patch\][	]*//')
line=$(echo "$line" | sed -e 's/\[//g')
line=$(echo "$line" | sed -e 's/\]//g')
line=$(echo "$line" | sed -e 's/^[ 	]*//')
line=$(echo "$line" | sed -e 's/ -/-/g')
line=$(echo "$line" | sed -e 's/- /-/g')
line=$(echo "$line" | sed -e 's/[ 	][ 	]*/-/g')
line=$(echo "$line" | sed -e 's,/,-,g')
line=$(echo "$line" | sed -e 's/--/-/g')
line=$(echo "$line" | sed -e 's/-$//g')
line=$(echo "$line" | sed -e 's/^-//g')



-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[Andrew Morton]

BlaisorBlade <blaisorblade_spam@yahoo.it> wrote:
>
> On Monday 06 September 2004 23:26, Andrew Morton wrote:
>  > Please don't use a filename like uml-ubd-no-empty-queue as the Subject:
>  > of your patches.  Please prepare an English-language summary.  See
>  > http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
>  Ok, but how can I specify the filename you'll give to the patch?
> 
>  It would make management between my tree and yours easier for me, if possible.

hm.  By choosing a suitable Subject:, I guess.

Start the Subject with "uml:" and then avoid getting fancy in the rest of
the subject and things should work out OK.  Spaces and other funny
characters are replaced with "-" and underscores are retained.

I use the below piece of ad-hoc revoltium to canonicalise Subject:s into
filenames:

line=$(echo "$line" | tr 'A-Z' 'a-z')
line=$(echo "$line" | sed -e 's/^subject:[ 	]*//')
line=$(echo "$line" | sed -e 's/^fw:[ 	]*//')
line=$(echo "$line" | sed -e 's/^fwd:[ 	]*//')
line=$(echo "$line" | sed -e 's/^aw:[ 	]*//')
line=$(echo "$line" | sed -e 's/^re:[ 	]*//')

line=$(echo "$line" | sed -e 's/^patch//')
line=$(echo "$line" | sed -e "s/['\(\)\<\>\{\}\,\.\\]//g")
line=$(echo "$line" | sed -e "s/[\#\*\&\+\^\!\~\`\:\?\;]//g")
line=$(echo "$line" | sed -e "s/[\$]//g")
line=$(echo "$line" | sed -e 's/"//g')
line=$(echo "$line" | sed -e 's/^[-]*//g')
line=$(echo "$line" | sed -e 's/\[[^]]*\]//g')
line=$(echo "$line" | sed -e 's/[ 	]*\[patch\][	]*//')
line=$(echo "$line" | sed -e 's/\[//g')
line=$(echo "$line" | sed -e 's/\]//g')
line=$(echo "$line" | sed -e 's/^[ 	]*//')
line=$(echo "$line" | sed -e 's/ -/-/g')
line=$(echo "$line" | sed -e 's/- /-/g')
line=$(echo "$line" | sed -e 's/[ 	][ 	]*/-/g')
line=$(echo "$line" | sed -e 's,/,-,g')
line=$(echo "$line" | sed -e 's/--/-/g')
line=$(echo "$line" | sed -e 's/-$//g')
line=$(echo "$line" | sed -e 's/^-//g')

[uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[Jens Axboe]

On Mon, Sep 06 2004, blaisorblade_spam@yahoo.it wrote:
> 
> Avoid using, in the UBD driver, the elv_queue_empty function. It's for
> the block layer only; in fact, the Anticipatory Scheduler can return NULL
> with elv_next_request() even if the queue is not empty, because it waits
> for the process to send another request before seeking on the disk.
> 
> In fact, if (with uml-ubd-any-elevator) we let UBD use any scheduler,
> elevator=as will make the UBD driver Oops, if we don't have this patch.
> 
> Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade_spam@yahoo.it>
> ---
> 
>  uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff -puN arch/um/drivers/ubd_kern.c~uml-ubd-no-empty-queue arch/um/drivers/ubd_kern.c
> --- uml-linux-2.6.8.1/arch/um/drivers/ubd_kern.c~uml-ubd-no-empty-queue	2004-08-29 14:40:51.313410952 +0200
> +++ uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c	2004-08-29 14:40:51.315410648 +0200
> @@ -1038,8 +1038,7 @@ static void do_ubd_request(request_queue
>  	int err, n;
>  
>  	if(thread_fd == -1){
> -		while(!elv_queue_empty(q)){
> -			req = elv_next_request(q);
> +		while((req = elv_next_request(q)) != NULL){
>  			err = prepare_request(req, &io_req);
>  			if(!err){
>  				do_io(&io_req);
> @@ -1048,9 +1047,8 @@ static void do_ubd_request(request_queue
>  		}
>  	}
>  	else {
> -		if(do_ubd || elv_queue_empty(q))
> +		if(do_ubd || (req = elv_next_request(q)) == NULL)
>  			return;
> -		req = elv_next_request(q);
>  		err = prepare_request(req, &io_req);
>  		if(!err){
>  			do_ubd = ubd_handler;

Patch is correct.

-- 
Jens Axboe



-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [patch 1/3] uml-ubd-no-empty-queue

[Jens Axboe]

On Mon, Sep 06 2004, blaisorblade_spam@yahoo.it wrote:
> 
> Avoid using, in the UBD driver, the elv_queue_empty function. It's for
> the block layer only; in fact, the Anticipatory Scheduler can return NULL
> with elv_next_request() even if the queue is not empty, because it waits
> for the process to send another request before seeking on the disk.
> 
> In fact, if (with uml-ubd-any-elevator) we let UBD use any scheduler,
> elevator=as will make the UBD driver Oops, if we don't have this patch.
> 
> Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade_spam@yahoo.it>
> ---
> 
>  uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff -puN arch/um/drivers/ubd_kern.c~uml-ubd-no-empty-queue arch/um/drivers/ubd_kern.c
> --- uml-linux-2.6.8.1/arch/um/drivers/ubd_kern.c~uml-ubd-no-empty-queue	2004-08-29 14:40:51.313410952 +0200
> +++ uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c	2004-08-29 14:40:51.315410648 +0200
> @@ -1038,8 +1038,7 @@ static void do_ubd_request(request_queue
>  	int err, n;
>  
>  	if(thread_fd == -1){
> -		while(!elv_queue_empty(q)){
> -			req = elv_next_request(q);
> +		while((req = elv_next_request(q)) != NULL){
>  			err = prepare_request(req, &io_req);
>  			if(!err){
>  				do_io(&io_req);
> @@ -1048,9 +1047,8 @@ static void do_ubd_request(request_queue
>  		}
>  	}
>  	else {
> -		if(do_ubd || elv_queue_empty(q))
> +		if(do_ubd || (req = elv_next_request(q)) == NULL)
>  			return;
> -		req = elv_next_request(q);
>  		err = prepare_request(req, &io_req);
>  		if(!err){
>  			do_ubd = ubd_handler;

Patch is correct.

-- 
Jens Axboe

Re: [uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[BlaisorBlade]

On Tuesday 07 September 2004 11:35, Jens Axboe wrote:
> On Mon, Sep 06 2004, blaisorblade_spam@yahoo.it wrote:

> Patch is correct.
Ok, thanks. Do you see anything else that needs fixing? The code the patch 
below removes has hidden this bug, so there could be other serious bugs (and 
by serious I mean Oopses or data loss).

Known issues:
- need to port to BIOs (a bit hard because I need first to understand the 
request mangling - see cowify_req. The code idea is to redirect each sector 
independently either to the UBD backing file or to the COW file. COW stand 
for Copy On Write: it allows to have the UBD read only and a COW file 
containing just the changes).

- Uml SMP support does not compile from sometimes so spinlocking is broken in 
ubd_finish (only in some cases - when called by do_ubd_request and thread_fd 
== -1). To see this, add ubd=sync on command line and turn spinlock debugging 
on.

--- uml-linux-2.6.8.1/arch/um/drivers/ubd_kern.c~uml-ubd-any-elevator   
2004-08-29 14:40:53.731043416 +0200
+++ uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c  2004-08-29 
14:40:53.733043112 +0200
@@ -749,8 +749,6 @@ int ubd_init(void)
                return -1;
        }
                
-       elevator_init(ubd_queue, &elevator_noop);
-
        if (fake_major != MAJOR_NR) {
                char name[sizeof("ubd_nnn\0")];
 
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729


-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Re: [patch 1/3] uml-ubd-no-empty-queue

[BlaisorBlade]

On Tuesday 07 September 2004 11:35, Jens Axboe wrote:
> On Mon, Sep 06 2004, blaisorblade_spam@yahoo.it wrote:

> Patch is correct.
Ok, thanks. Do you see anything else that needs fixing? The code the patch 
below removes has hidden this bug, so there could be other serious bugs (and 
by serious I mean Oopses or data loss).

Known issues:
- need to port to BIOs (a bit hard because I need first to understand the 
request mangling - see cowify_req. The code idea is to redirect each sector 
independently either to the UBD backing file or to the COW file. COW stand 
for Copy On Write: it allows to have the UBD read only and a COW file 
containing just the changes).

- Uml SMP support does not compile from sometimes so spinlocking is broken in 
ubd_finish (only in some cases - when called by do_ubd_request and thread_fd 
== -1). To see this, add ubd=sync on command line and turn spinlock debugging 
on.

--- uml-linux-2.6.8.1/arch/um/drivers/ubd_kern.c~uml-ubd-any-elevator   
2004-08-29 14:40:53.731043416 +0200
+++ uml-linux-2.6.8.1-paolo/arch/um/drivers/ubd_kern.c  2004-08-29 
14:40:53.733043112 +0200
@@ -749,8 +749,6 @@ int ubd_init(void)
                return -1;
        }
                
-       elevator_init(ubd_queue, &elevator_noop);
-
        if (fake_major != MAJOR_NR) {
                char name[sizeof("ubd_nnn\0")];
 
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729