mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux)
@ 2010-09-28 16:16 richard -rw- weinberger
  2010-09-28 17:07 ` [uml-devel] " Janjaap Bos
  0 siblings, 1 reply; 7+ messages in thread
From: richard -rw- weinberger @ 2010-09-28 16:16 UTC (permalink / raw)
  To: tj
  Cc: LKML, user-mode-linux-devel, axboe, jdike, Geert.Uytterhoeven,
	grant.likely, adrian, mike.miller, Chris Frey

Hi Tejun!

Chris Frey ran into some problems with ext3 on UML.
See: http://marc.info/?i=20100924041410.GA18040%20()%20foursquare%20!%20net

I can reproduce this issue with any file system on Linux >= 2.6.31.
There seems to be a serious problem with the block layer on UML.
Under high load any file system gets corrupted.

The regression was most likely introduced with this three changes:
83096ebf1263b2c1ee5e653ba37d993d02e3eb7b (block: convert to pos and
nr_sectors accessors)
f81f2f7c9fee307e371f37424577d46f9eaf8692 (ubd: drop unnecessary
rq->sector manipulation)
4d6c84d91d1a539ebc47d1a36a35e9390ba11fdc (ubd: cleanup completion path)

Maybe only f81f2f7 is the bad one. They depend on each and I don't
know all the internals...

-- 
Cheers,
//richard

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux)
  2010-09-28 16:16 [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux) richard -rw- weinberger
@ 2010-09-28 17:07 ` Janjaap Bos
  2010-09-28 19:52   ` Geert Uytterhoeven
  0 siblings, 1 reply; 7+ messages in thread
From: Janjaap Bos @ 2010-09-28 17:07 UTC (permalink / raw)
  To: richard -rw- weinberger
  Cc: tj, axboe, mike.miller, user-mode-linux-devel, adrian, jdike,
	LKML, grant.likely, Geert.Uytterhoeven, Chris Frey

[-- Attachment #1: Type: text/plain, Size: 967 bytes --]

See attached patch, and earlier message posted in March 2010 on uml user
list. We are out of maintainer...

Best,

-Janjaap



On Tue, 2010-09-28 at 18:16 +0200, richard -rw- weinberger wrote:
> Hi Tejun!
> 
> Chris Frey ran into some problems with ext3 on UML.
> See: http://marc.info/?i=20100924041410.GA18040%20()%20foursquare%20!%20net
> 
> I can reproduce this issue with any file system on Linux >= 2.6.31.
> There seems to be a serious problem with the block layer on UML.
> Under high load any file system gets corrupted.
> 
> The regression was most likely introduced with this three changes:
> 83096ebf1263b2c1ee5e653ba37d993d02e3eb7b (block: convert to pos and
> nr_sectors accessors)
> f81f2f7c9fee307e371f37424577d46f9eaf8692 (ubd: drop unnecessary
> rq->sector manipulation)
> 4d6c84d91d1a539ebc47d1a36a35e9390ba11fdc (ubd: cleanup completion path)
> 
> Maybe only f81f2f7 is the bad one. They depend on each and I don't
> know all the internals...
> 


[-- Attachment #2: v2.6.33.diff --]
[-- Type: text/x-patch, Size: 4560 bytes --]

diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
index cf8a97f..a529c3f 100644
--- a/arch/um/drivers/line.c
+++ b/arch/um/drivers/line.c
@@ -305,10 +305,12 @@ int line_ioctl(struct tty_struct *tty, struct file * file,
 		for (i = 0; i < ARRAY_SIZE(tty_ioctls); i++)
 			if (cmd == tty_ioctls[i].cmd)
 				break;
+#ifdef DEBUG
 		if (i == ARRAY_SIZE(tty_ioctls)) {
 			printk(KERN_ERR "%s: %s: unknown ioctl: 0x%x\n",
 			       __func__, tty->name, cmd);
 		}
+#endif
 		ret = -ENOIOCTLCMD;
 		break;
 	}
diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 5ff5546..655ed9e 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -746,8 +746,12 @@ static int ubd_open_dev(struct ubd *ubd_dev)
 	}
 	ubd_dev->fd = fd;
 
+	/* A setting higher than 1 sector currently (v2.6.33) generates
+	data loss, both for raw and cow ubd. */
+	blk_queue_max_sectors(ubd_dev->queue, 1 * sizeof(long));
+	blk_queue_max_phys_segments(ubd_dev->queue, 1 * sizeof(long));
+
 	if(ubd_dev->cow.file != NULL){
-		blk_queue_max_sectors(ubd_dev->queue, 8 * sizeof(long));
 
 		err = -ENOMEM;
 		ubd_dev->cow.bitmap = vmalloc(ubd_dev->cow.bitmap_len);
@@ -1223,7 +1227,7 @@ static void do_ubd_request(struct request_queue *q)
 	struct io_thread_req *io_req;
 	struct request *req;
 	sector_t sector;
-	int n;
+	int n, last_sectors;
 
 	while(1){
 		struct ubd *dev = q->queuedata;
@@ -1239,9 +1243,12 @@ static void do_ubd_request(struct request_queue *q)
 
 		req = dev->request;
 		sector = blk_rq_pos(req);
+		last_sectors = 0;
 		while(dev->start_sg < dev->end_sg){
 			struct scatterlist *sg = &dev->sg[dev->start_sg];
 
+			sector += last_sectors;
+			last_sectors = 0;
 			io_req = kmalloc(sizeof(struct io_thread_req),
 					 GFP_ATOMIC);
 			if(io_req == NULL){
@@ -1253,7 +1260,7 @@ static void do_ubd_request(struct request_queue *q)
 					(unsigned long long)sector << 9,
 					sg->offset, sg->length, sg_page(sg));
 
-			sector += sg->length >> 9;
+			last_sectors = sg->length >> 9;
 			n = os_write_file(thread_fd, &io_req,
 					  sizeof(struct io_thread_req *));
 			if(n != sizeof(struct io_thread_req *)){
diff --git a/arch/um/kernel/dyn.lds.S b/arch/um/kernel/dyn.lds.S
index 7fcad58..0731311 100644
--- a/arch/um/kernel/dyn.lds.S
+++ b/arch/um/kernel/dyn.lds.S
@@ -50,8 +50,20 @@ SECTIONS
   .rela.got       : { *(.rela.got) }
   .rel.bss        : { *(.rel.bss .rel.bss.* .rel.gnu.linkonce.b.*) }
   .rela.bss       : { *(.rela.bss .rela.bss.* .rela.gnu.linkonce.b.*) }
-  .rel.plt        : { *(.rel.plt) }
-  .rela.plt       : { *(.rela.plt) }
+  .rel.plt        :
+  {
+    *(.rel.plt)
+    PROVIDE_HIDDEN (__rel_iplt_start = .);
+    *(.rel.iplt)
+    PROVIDE_HIDDEN (__rel_iplt_end = .);
+  }
+  .rela.plt       :
+  {
+    *(.rela.plt)
+    PROVIDE_HIDDEN (__rela_iplt_start = .);
+    *(.rela.iplt)
+    PROVIDE_HIDDEN (__rela_iplt_end = .);
+  }
   .init           : {
     KEEP (*(.init))
   } =0x90909090
diff --git a/arch/um/kernel/uml.lds.S b/arch/um/kernel/uml.lds.S
index e7a6cca..a658604 100644
--- a/arch/um/kernel/uml.lds.S
+++ b/arch/um/kernel/uml.lds.S
@@ -36,6 +36,21 @@ SECTIONS
     *(.gnu.linkonce.t*)
   }
 
+  .rel.plt        :
+  {
+    *(.rel.plt)
+    PROVIDE_HIDDEN (__rel_iplt_start = .);
+    *(.rel.iplt)
+    PROVIDE_HIDDEN (__rel_iplt_end = .);
+  }
+  .rela.plt       :
+  {
+    *(.rela.plt)
+    PROVIDE_HIDDEN (__rela_iplt_start = .);
+    *(.rela.iplt)
+    PROVIDE_HIDDEN (__rela_iplt_end = .);
+  }
+
   . = ALIGN(PAGE_SIZE);
   .syscall_stub : {
 	__syscall_stub_start = .;
diff --git a/arch/um/sys-x86_64/ptrace.c b/arch/um/sys-x86_64/ptrace.c
index f3458d7..1c738c7 100644
--- a/arch/um/sys-x86_64/ptrace.c
+++ b/arch/um/sys-x86_64/ptrace.c
@@ -69,7 +69,7 @@ int poke_user(struct task_struct *child, long addr, long data)
 	else if ((addr >= offsetof(struct user, u_debugreg[0])) &&
 		(addr <= offsetof(struct user, u_debugreg[7]))) {
 		addr -= offsetof(struct user, u_debugreg[0]);
-		addr = addr >> 2;
+		addr = addr >> 3; // long is 8 bytes on x86_64
 		if ((addr == 4) || (addr == 5))
 			return -EIO;
 		child->thread.arch.debugregs[addr] = data;
@@ -114,7 +114,7 @@ int peek_user(struct task_struct *child, long addr, long data)
 	else if ((addr >= offsetof(struct user, u_debugreg[0])) &&
 		(addr <= offsetof(struct user, u_debugreg[7]))) {
 		addr -= offsetof(struct user, u_debugreg[0]);
-		addr = addr >> 2;
+		addr = addr >> 3; // long is 8 bytes on x86_64
 		tmp = child->thread.arch.debugregs[addr];
 	}
 	return put_user(tmp, (unsigned long *) data);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux)
  2010-09-28 17:07 ` [uml-devel] " Janjaap Bos
@ 2010-09-28 19:52   ` Geert Uytterhoeven
  2010-09-28 20:12     ` Janjaap Bos
  0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2010-09-28 19:52 UTC (permalink / raw)
  To: Janjaap Bos
  Cc: richard -rw- weinberger, tj, axboe, mike.miller,
	user-mode-linux-devel, adrian, jdike, LKML, grant.likely,
	Geert.Uytterhoeven, Chris Frey

On Tue, Sep 28, 2010 at 19:07, Janjaap Bos <janjaap@bos.nl> wrote:
> See attached patch, and earlier message posted in March 2010 on uml user
> list. We are out of maintainer...

Thanks for the patch!

| --- a/arch/um/drivers/ubd_kern.c
| +++ b/arch/um/drivers/ubd_kern.c
| @@ -1223,7 +1227,7 @@ static void do_ubd_request(struct request_queue *q)
|       struct io_thread_req *io_req;
|       struct request *req;
|       sector_t sector;
| -     int n;
| +     int n, last_sectors;
|
|       while(1){
|               struct ubd *dev = q->queuedata;
| @@ -1239,9 +1243,12 @@ static void do_ubd_request(struct request_queue *q)
|
|               req = dev->request;
|               sector = blk_rq_pos(req);
| +             last_sectors = 0;
|               while(dev->start_sg < dev->end_sg){
|                       struct scatterlist *sg = &dev->sg[dev->start_sg];
|
| +                     sector += last_sectors;
| +                     last_sectors = 0;
|                       io_req = kmalloc(sizeof(struct io_thread_req),
|                                        GFP_ATOMIC);
|                       if(io_req == NULL){
| @@ -1253,7 +1260,7 @@ static void do_ubd_request(struct request_queue *q)
|                                       (unsigned long long)sector << 9,
|                                       sg->offset, sg->length, sg_page(sg));
|
| -                     sector += sg->length >> 9;
| +                     last_sectors = sg->length >> 9;
|                       n = os_write_file(thread_fd, &io_req,
|                                         sizeof(struct io_thread_req *));
|                       if(n != sizeof(struct io_thread_req *)){

However, I'm wondering what difference this part makes?

> On Tue, 2010-09-28 at 18:16 +0200, richard -rw- weinberger wrote:
>> Hi Tejun!
>>
>> Chris Frey ran into some problems with ext3 on UML.
>> See: http://marc.info/?i=20100924041410.GA18040%20()%20foursquare%20!%20net
>>
>> I can reproduce this issue with any file system on Linux >= 2.6.31.
>> There seems to be a serious problem with the block layer on UML.
>> Under high load any file system gets corrupted.
>>
>> The regression was most likely introduced with this three changes:
>> 83096ebf1263b2c1ee5e653ba37d993d02e3eb7b (block: convert to pos and
>> nr_sectors accessors)
>> f81f2f7c9fee307e371f37424577d46f9eaf8692 (ubd: drop unnecessary
>> rq->sector manipulation)
>> 4d6c84d91d1a539ebc47d1a36a35e9390ba11fdc (ubd: cleanup completion path)
>>
>> Maybe only f81f2f7 is the bad one. They depend on each and I don't
>> know all the internals...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux)
  2010-09-28 19:52   ` Geert Uytterhoeven
@ 2010-09-28 20:12     ` Janjaap Bos
  2010-09-28 20:37       ` Geert Uytterhoeven
  0 siblings, 1 reply; 7+ messages in thread
From: Janjaap Bos @ 2010-09-28 20:12 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: richard -rw- weinberger, tj, axboe, mike.miller,
	user-mode-linux-devel, adrian, jdike, LKML, grant.likely,
	Geert.Uytterhoeven, Chris Frey

Hi,

On Tue, 2010-09-28 at 21:52 +0200, Geert Uytterhoeven wrote:
> On Tue, Sep 28, 2010 at 19:07, Janjaap Bos <janjaap@bos.nl> wrote:
> > See attached patch, and earlier message posted in March 2010 on uml user
> > list. We are out of maintainer...
> 
> Thanks for the patch!
> 
> | --- a/arch/um/drivers/ubd_kern.c
> | +++ b/arch/um/drivers/ubd_kern.c
> | @@ -1223,7 +1227,7 @@ static void do_ubd_request(struct request_queue *q)
> |       struct io_thread_req *io_req;
> |       struct request *req;
> |       sector_t sector;
> | -     int n;
> | +     int n, last_sectors;
> |
> |       while(1){
> |               struct ubd *dev = q->queuedata;
> | @@ -1239,9 +1243,12 @@ static void do_ubd_request(struct request_queue *q)
> |
> |               req = dev->request;
> |               sector = blk_rq_pos(req);
> | +             last_sectors = 0;
> |               while(dev->start_sg < dev->end_sg){
> |                       struct scatterlist *sg = &dev->sg[dev->start_sg];
> |
> | +                     sector += last_sectors;
> | +                     last_sectors = 0;
> |                       io_req = kmalloc(sizeof(struct io_thread_req),
> |                                        GFP_ATOMIC);
> |                       if(io_req == NULL){
> | @@ -1253,7 +1260,7 @@ static void do_ubd_request(struct request_queue *q)
> |                                       (unsigned long long)sector << 9,
> |                                       sg->offset, sg->length, sg_page(sg));
> |
> | -                     sector += sg->length >> 9;
> | +                     last_sectors = sg->length >> 9;
> |                       n = os_write_file(thread_fd, &io_req,
> |                                         sizeof(struct io_thread_req *));
> |                       if(n != sizeof(struct io_thread_req *)){
> 
> However, I'm wondering what difference this part makes?
> 


It fixes ubd block handling integrity.
With large block operations errors occurred. Probably due to lost
request pointers as explained below. Need to keep a local count of
sectors and delay the update. Done by reverting commit
f81f2f7c9fee307e371f37424577d46f9eaf8692 using the present
block api. (At least which is what I intend, but perhaps quite likely I
am missing the point, also it may not be needed anymore if only single
512 byte sector blocks are used per request. Anyway... it solves the
problem for me ;-)

Regards,
-Janjaap


See:

reverted: commit f81f2f7c9fee307e371f37424577d46f9eaf8692
          Author: Tejun Heo <tj@kernel.org>
          Date:   Tue Apr 28 13:06:10 2009 +0900
          ubd: drop unnecessary rq->sector manipulation
    ubd curiously updates rq->sector while issuing the request
    in multiple pieces.  Don't do it and simply use local copy
    of sector.
        
See for original reason:
    
commit 0a6d3a2a3813e7b25267366cfbf9a4a4698dd1c2
        Author: Jeff Dike <jdike@addtoit.com>
        Date:   Sun Jul 15 23:38:47 2007 -0700
        uml: fix request->sector update
    
    It is theoretically possible for a request to finish and be freed
between writing it to the I/O thread and updating the sector count.  In
this case, the update will dereference a freed pointer.
    To avoid this, I delay the update until processing the next sg
segment, when the request pointer is known to be good.
    
    modified:   arch/um/drivers/ubd_kern.c




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux)
  2010-09-28 20:12     ` Janjaap Bos
@ 2010-09-28 20:37       ` Geert Uytterhoeven
  2010-09-28 21:19         ` richard -rw- weinberger
  0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2010-09-28 20:37 UTC (permalink / raw)
  To: Janjaap Bos
  Cc: richard -rw- weinberger, tj, axboe, mike.miller,
	user-mode-linux-devel, adrian, jdike, LKML, grant.likely,
	Geert.Uytterhoeven, Chris Frey

On Tue, Sep 28, 2010 at 22:12, Janjaap Bos <janjaap@bos.nl> wrote:
>> On Tue, 2010-09-28 at 21:52 +0200, Geert Uytterhoeven wrote:
>> On Tue, Sep 28, 2010 at 19:07, Janjaap Bos <janjaap@bos.nl> wrote:
>> > See attached patch, and earlier message posted in March 2010 on uml user
>> > list. We are out of maintainer...
>>
>> Thanks for the patch!
>>
>> | --- a/arch/um/drivers/ubd_kern.c
>> | +++ b/arch/um/drivers/ubd_kern.c
>> | @@ -1223,7 +1227,7 @@ static void do_ubd_request(struct request_queue *q)
>> |       struct io_thread_req *io_req;
>> |       struct request *req;
>> |       sector_t sector;
>> | -     int n;
>> | +     int n, last_sectors;
>> |
>> |       while(1){
>> |               struct ubd *dev = q->queuedata;
>> | @@ -1239,9 +1243,12 @@ static void do_ubd_request(struct request_queue *q)
>> |
>> |               req = dev->request;
>> |               sector = blk_rq_pos(req);
>> | +             last_sectors = 0;
>> |               while(dev->start_sg < dev->end_sg){
>> |                       struct scatterlist *sg = &dev->sg[dev->start_sg];
>> |
>> | +                     sector += last_sectors;
>> | +                     last_sectors = 0;
>> |                       io_req = kmalloc(sizeof(struct io_thread_req),
>> |                                        GFP_ATOMIC);
>> |                       if(io_req == NULL){
>> | @@ -1253,7 +1260,7 @@ static void do_ubd_request(struct request_queue *q)
>> |                                       (unsigned long long)sector << 9,
>> |                                       sg->offset, sg->length, sg_page(sg));
>> |
>> | -                     sector += sg->length >> 9;
>> | +                     last_sectors = sg->length >> 9;
>> |                       n = os_write_file(thread_fd, &io_req,
>> |                                         sizeof(struct io_thread_req *));
>> |                       if(n != sizeof(struct io_thread_req *)){
>>
>> However, I'm wondering what difference this part makes?
>>
>
>
> It fixes ubd block handling integrity.
> With large block operations errors occurred. Probably due to lost
> request pointers as explained below. Need to keep a local count of
> sectors and delay the update. Done by reverting commit
> f81f2f7c9fee307e371f37424577d46f9eaf8692 using the present
> block api. (At least which is what I intend, but perhaps quite likely I
> am missing the point, also it may not be needed anymore if only single
> 512 byte sector blocks are used per request. Anyway... it solves the
> problem for me ;-)
>
> Regards,
> -Janjaap
>
>
> See:
>
> reverted: commit f81f2f7c9fee307e371f37424577d46f9eaf8692
>          Author: Tejun Heo <tj@kernel.org>
>          Date:   Tue Apr 28 13:06:10 2009 +0900
>          ubd: drop unnecessary rq->sector manipulation
>    ubd curiously updates rq->sector while issuing the request
>    in multiple pieces.  Don't do it and simply use local copy
>    of sector.
>
> See for original reason:
>
> commit 0a6d3a2a3813e7b25267366cfbf9a4a4698dd1c2
>        Author: Jeff Dike <jdike@addtoit.com>
>        Date:   Sun Jul 15 23:38:47 2007 -0700
>        uml: fix request->sector update
>
>    It is theoretically possible for a request to finish and be freed
> between writing it to the I/O thread and updating the sector count.  In
> this case, the update will dereference a freed pointer.
>    To avoid this, I delay the update until processing the next sg
> segment, when the request pointer is known to be good.
>
>    modified:   arch/um/drivers/ubd_kern.c

This changeset from 2007 indeed moved the update of req->sector.

However, in the new code, before or after applying your patch, there's no update
of req->sector anymore. Everything is done in local variables.

Is it possible that we only need the hunk below to fix the corruption?

| diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
| index 5ff5546..655ed9e 100644
| --- a/arch/um/drivers/ubd_kern.c
| +++ b/arch/um/drivers/ubd_kern.c
| @@ -746,8 +746,12 @@ static int ubd_open_dev(struct ubd *ubd_dev)
|       }
|       ubd_dev->fd = fd;
|
| +     /* A setting higher than 1 sector currently (v2.6.33) generates
| +     data loss, both for raw and cow ubd. */
| +     blk_queue_max_sectors(ubd_dev->queue, 1 * sizeof(long));
| +     blk_queue_max_phys_segments(ubd_dev->queue, 1 * sizeof(long));
| +
|       if(ubd_dev->cow.file != NULL){
| -             blk_queue_max_sectors(ubd_dev->queue, 8 * sizeof(long));
|
|               err = -ENOMEM;
|               ubd_dev->cow.bitmap = vmalloc(ubd_dev->cow.bitmap_len);

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux)
  2010-09-28 20:37       ` Geert Uytterhoeven
@ 2010-09-28 21:19         ` richard -rw- weinberger
  2010-09-29  9:19           ` Geert Uytterhoeven
  0 siblings, 1 reply; 7+ messages in thread
From: richard -rw- weinberger @ 2010-09-28 21:19 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Janjaap Bos, tj, axboe, mike.miller, user-mode-linux-devel,
	adrian, jdike, LKML, grant.likely, Geert.Uytterhoeven,
	Chris Frey

On Tue, Sep 28, 2010 at 10:37 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> This changeset from 2007 indeed moved the update of req->sector.
>
> However, in the new code, before or after applying your patch, there's no update
> of req->sector anymore. Everything is done in local variables.
>
> Is it possible that we only need the hunk below to fix the corruption?

Yes, it seems so. So far all my test cases work fine.
I'll submit a patch in a few minutes.

> | diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> | index 5ff5546..655ed9e 100644
> | --- a/arch/um/drivers/ubd_kern.c
> | +++ b/arch/um/drivers/ubd_kern.c
> | @@ -746,8 +746,12 @@ static int ubd_open_dev(struct ubd *ubd_dev)
> |       }
> |       ubd_dev->fd = fd;
> |
> | +     /* A setting higher than 1 sector currently (v2.6.33) generates
> | +     data loss, both for raw and cow ubd. */
> | +     blk_queue_max_sectors(ubd_dev->queue, 1 * sizeof(long));
> | +     blk_queue_max_phys_segments(ubd_dev->queue, 1 * sizeof(long));
> | +
> |       if(ubd_dev->cow.file != NULL){
> | -             blk_queue_max_sectors(ubd_dev->queue, 8 * sizeof(long));
> |
> |               err = -ENOMEM;
> |               ubd_dev->cow.bitmap = vmalloc(ubd_dev->cow.bitmap_len);
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>

-- 
Cheers,
//richard

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux)
  2010-09-28 21:19         ` richard -rw- weinberger
@ 2010-09-29  9:19           ` Geert Uytterhoeven
  0 siblings, 0 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2010-09-29  9:19 UTC (permalink / raw)
  To: richard -rw- weinberger
  Cc: Janjaap Bos, tj, axboe, mike.miller, user-mode-linux-devel,
	adrian, jdike, LKML, grant.likely, Geert.Uytterhoeven,
	Chris Frey

On Tue, Sep 28, 2010 at 23:19, richard -rw- weinberger
<richard.weinberger@gmail.com> wrote:
> On Tue, Sep 28, 2010 at 10:37 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>>
>> This changeset from 2007 indeed moved the update of req->sector.
>>
>> However, in the new code, before or after applying your patch, there's no update
>> of req->sector anymore. Everything is done in local variables.
>>
>> Is it possible that we only need the hunk below to fix the corruption?
>
> Yes, it seems so. So far all my test cases work fine.
> I'll submit a patch in a few minutes.

Thanks, good to know!

>> | diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
>> | index 5ff5546..655ed9e 100644
>> | --- a/arch/um/drivers/ubd_kern.c
>> | +++ b/arch/um/drivers/ubd_kern.c
>> | @@ -746,8 +746,12 @@ static int ubd_open_dev(struct ubd *ubd_dev)
>> |       }
>> |       ubd_dev->fd = fd;
>> |
>> | +     /* A setting higher than 1 sector currently (v2.6.33) generates
>> | +     data loss, both for raw and cow ubd. */
>> | +     blk_queue_max_sectors(ubd_dev->queue, 1 * sizeof(long));
>> | +     blk_queue_max_phys_segments(ubd_dev->queue, 1 * sizeof(long));
>> | +
>> |       if(ubd_dev->cow.file != NULL){
>> | -             blk_queue_max_sectors(ubd_dev->queue, 8 * sizeof(long));
>> |
>> |               err = -ENOMEM;
>> |               ubd_dev->cow.bitmap = vmalloc(ubd_dev->cow.bitmap_len);

So we can no longer process more than one sector? Why?

Could it be 4d6c84d91d1a539ebc47d1a36a35e9390ba11fdc (ubd: cleanup
completion path),
which is one of the three commits you pointed out?

Before 4d6c84d91d1a539ebc47d1a36a35e9390ba11fdc, blk_end_request() was
only called when
rq->nr_sectors reached zero.
Now it's called for each part.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2010-09-29  9:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-28 16:16 [REGRESSION] um: ubd: block layer issue (Was: ext3 filesystem corruption in user mode linux) richard -rw- weinberger
2010-09-28 17:07 ` [uml-devel] " Janjaap Bos
2010-09-28 19:52   ` Geert Uytterhoeven
2010-09-28 20:12     ` Janjaap Bos
2010-09-28 20:37       ` Geert Uytterhoeven
2010-09-28 21:19         ` richard -rw- weinberger
2010-09-29  9:19           ` Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome