Re: [uml-devel] [REGRESSION] um: ubd: block layer issue (Was: ext3filesystem corruption in user mode linux)

[Geert Uytterhoeven]

On Tue, Sep 28, 2010 at 22:12, Janjaap Bos <janjaap@xxxxxx> wrote:
>> On Tue, 2010-09-28 at 21:52 +0200, Geert Uytterhoeven wrote:
>> On Tue, Sep 28, 2010 at 19:07, Janjaap Bos <janjaap@xxxxxx> 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@xxxxxxxxxx>
> Â Â Â Â Â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@xxxxxxxxxxx>
> Â Â Â Â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@xxxxxxxxxxxxxx

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/