[PATCH] fix small highmem bio bounce bvec handling glitch

[PATCH] fix small highmem bio bounce bvec handling glitch

[Christophe Saout]

Hi,

[Oops, forgot to Cc LKML]

while going through the bio highmem handling code I noticed a small
glitch. __end_that_request_first might modify the bv_offset and bv_len
if the segment was partially completed. The bio-read-bounce-back code
should use the unmodified bv_offset when copying the segment data:

--- linux.orig/mm/highmem.c	2004-01-21 19:08:45.000000000 +0100
+++ linux/mm/highmem.c	2004-02-26 15:47:14.574722576 +0100
@@ -294,7 +294,12 @@
 		if (tovec->bv_page == fromvec->bv_page)
 			continue;
 
-		vfrom = page_address(fromvec->bv_page) + fromvec->bv_offset;
+		/*
+		 * fromvec->bv_offset and fromvec->bv_len might have been
+		 * modified by the block layer, so use the original copy,
+		 * bounce_copy_vec already uses tovec->bv_len
+		 */
+		vfrom = page_address(fromvec->bv_page) + tovec->bv_offset;
 
 		bounce_copy_vec(tovec, vfrom);
 	}

Re: [PATCH] fix small highmem bio bounce bvec handling glitch

[Jens Axboe]

On Thu, Feb 26 2004, Christophe Saout wrote:
> Hi,
> 
> [Oops, forgot to Cc LKML]
> 
> while going through the bio highmem handling code I noticed a small
> glitch. __end_that_request_first might modify the bv_offset and bv_len
> if the segment was partially completed. The bio-read-bounce-back code
> should use the unmodified bv_offset when copying the segment data:
> 
> --- linux.orig/mm/highmem.c	2004-01-21 19:08:45.000000000 +0100
> +++ linux/mm/highmem.c	2004-02-26 15:47:14.574722576 +0100
> @@ -294,7 +294,12 @@
>  		if (tovec->bv_page == fromvec->bv_page)
>  			continue;
>  
> -		vfrom = page_address(fromvec->bv_page) + fromvec->bv_offset;
> +		/*
> +		 * fromvec->bv_offset and fromvec->bv_len might have been
> +		 * modified by the block layer, so use the original copy,
> +		 * bounce_copy_vec already uses tovec->bv_len
> +		 */
> +		vfrom = page_address(fromvec->bv_page) + tovec->bv_offset;
>  
>  		bounce_copy_vec(tovec, vfrom);

Irk yes, that's is pretty nasty, I really wish we could avoid screwing
with vec entries (it's pretty obscure for bio clones, too)... Fix is
correct, and good idea to add the comment!

-- 
Jens Axboe

Re: [PATCH] fix small highmem bio bounce bvec handling glitch

[Christophe Saout]

Am Do, den 26.02.2004 schrieb Jens Axboe um 16:10:

> > --- linux.orig/mm/highmem.c	2004-01-21 19:08:45.000000000 +0100
> > +++ linux/mm/highmem.c	2004-02-26 15:47:14.574722576 +0100
> > @@ -294,7 +294,12 @@
> >  		if (tovec->bv_page == fromvec->bv_page)
> >  			continue;
> >  
> > -		vfrom = page_address(fromvec->bv_page) + fromvec->bv_offset;
> > +		/*
> > +		 * fromvec->bv_offset and fromvec->bv_len might have been
> > +		 * modified by the block layer, so use the original copy,
> > +		 * bounce_copy_vec already uses tovec->bv_len
> > +		 */
> > +		vfrom = page_address(fromvec->bv_page) + tovec->bv_offset;
> >  
> >  		bounce_copy_vec(tovec, vfrom);
> 
> Irk yes, that's is pretty nasty, I really wish we could avoid screwing
> with vec entries

What about a bio->bi_bvec_done field?

> (it's pretty obscure for bio clones, too)...

Yes, I noticed that dm-crypt also does the same mistake for reads. I'm
going to change it too (easily accomplished).

Re: [PATCH] fix small highmem bio bounce bvec handling glitch

[Jens Axboe]

On Thu, Feb 26 2004, Christophe Saout wrote:
> Am Do, den 26.02.2004 schrieb Jens Axboe um 16:10:
> 
> > > --- linux.orig/mm/highmem.c	2004-01-21 19:08:45.000000000 +0100
> > > +++ linux/mm/highmem.c	2004-02-26 15:47:14.574722576 +0100
> > > @@ -294,7 +294,12 @@
> > >  		if (tovec->bv_page == fromvec->bv_page)
> > >  			continue;
> > >  
> > > -		vfrom = page_address(fromvec->bv_page) + fromvec->bv_offset;
> > > +		/*
> > > +		 * fromvec->bv_offset and fromvec->bv_len might have been
> > > +		 * modified by the block layer, so use the original copy,
> > > +		 * bounce_copy_vec already uses tovec->bv_len
> > > +		 */
> > > +		vfrom = page_address(fromvec->bv_page) + tovec->bv_offset;
> > >  
> > >  		bounce_copy_vec(tovec, vfrom);
> > 
> > Irk yes, that's is pretty nasty, I really wish we could avoid screwing
> > with vec entries
> 
> What about a bio->bi_bvec_done field?

That'd work. Suparna originally suggested bio->bi_voffset (iirc) as a
current offset. It just doesn't feel completely right...

> > (it's pretty obscure for bio clones, too)...
> 
> Yes, I noticed that dm-crypt also does the same mistake for reads. I'm
> going to change it too (easily accomplished).

Great.

-- 
Jens Axboe