Re: [PATCH 1/1] um: ubd: Fix data corruption

[Tejun Heo]

Hello, sorry about chiming in later.  I was off last week.

On 09/29/2010 08:34 AM, Chris Frey wrote:
> On Wed, Sep 29, 2010 at 02:21:07PM +0900, Jens Axboe wrote:
>> This seems to imply that the original commit pin pointed is not
>> the only issue we have in that code atm.
>>
>> I think we need to find the real fix here, just disabling merging
>> is not a fix (it's just a nasty work-around for the real bug).
> 
> You're probably right, and I'm quite willing to help test further patches
> to help get to the bottom of this.

I think we're on the right track.  The problem with Jens' patch was
that it didn't consider the fact that blk_end_request() now internally
updates the current position of the request, so if restart happens
after some of part of the request is complete it will end up adding
the offsets multiple times.  I think slightly modifying it to track
the current position instead of offset should do it.  Chris, can you
please try the following patch and see whether the problem goes away?

Thanks.

diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
index 1bcd208..d8a5d8c 100644
--- a/arch/um/drivers/ubd_kern.c
+++ b/arch/um/drivers/ubd_kern.c
@@ -163,6 +163,7 @@ struct ubd {
 	struct scatterlist sg[MAX_SG];
 	struct request *request;
 	int start_sg, end_sg;
+	sector_t rq_pos;
 };

 #define DEFAULT_COW { \
@@ -187,6 +188,7 @@ struct ubd {
 	.request =		NULL, \
 	.start_sg =		0, \
 	.end_sg =		0, \
+	.rq_pos =		0, \
 }

 /* Protected by ubd_lock */
@@ -1228,7 +1230,6 @@ static void do_ubd_request(struct request_queue *q)
 {
 	struct io_thread_req *io_req;
 	struct request *req;
-	sector_t sector;
 	int n;

 	while(1){
@@ -1244,7 +1245,7 @@ static void do_ubd_request(struct request_queue *q)
 		}

 		req = dev->request;
-		sector = blk_rq_pos(req);
+		dev->rq_pos = blk_rq_pos(req);
 		while(dev->start_sg < dev->end_sg){
 			struct scatterlist *sg = &dev->sg[dev->start_sg];

@@ -1256,10 +1257,10 @@ static void do_ubd_request(struct request_queue *q)
 				return;
 			}
 			prepare_request(req, io_req,
-					(unsigned long long)sector << 9,
+					(unsigned long long)dev->rq_pos << 9,
 					sg->offset, sg->length, sg_page(sg));

-			sector += sg->length >> 9;
+			dev->rq_pos += sg->length >> 9;
 			n = os_write_file(thread_fd, &io_req,
 					  sizeof(struct io_thread_req *));
 			if(n != sizeof(struct io_thread_req *)){
--
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/