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 <[email protected]>
---
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;
_
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.
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
On Mon, Sep 06 2004, [email protected] 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 <[email protected]>
> ---
>
> 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
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
On Tuesday 07 September 2004 11:35, Jens Axboe wrote:
> On Mon, Sep 06 2004, [email protected] 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
BlaisorBlade <[email protected]> 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')