Re: [patch] new aop loop fix

Re: [patch] new aop loop fix

[Hugh Dickins]

On Wed, 13 Jun 2007, Dmitriy Monakhov wrote:

> 	loop.c code itself is not perfect. In fact before Nick's patch
> 	partial write was't possible. Assumption what write chunks are
> 	always page aligned is realy weird ( see "index++" line).
> 
> Signed-off-by: Dmitriy Monakhov <dmonakhov@openvz.org>

I'm interested, because I'm trying to chase down an -mm bug which
occasionally leaves me with 1k of zeroes where it shouldn't (in a
1k bsize ext2 looped over tmpfs).  The length of time for this to
happen varies a lot, so bisection has been misleading: maybe the
problem lies in Nick's patches, maybe it does not.

But I don't understand your fix below at all.  _If_ you need to
change the handling of index, then you need to change the handling
of offset too, don't you?

But what's wrong with how inded was handled anyway?  Yes, it might
be being incremented at the bottom of the loop when we haven't
reached the end of this page, but in that case we're not going
round the loop again anyway: len will now be 0.  So no problem.

One of us is missing something: please enlighten me - thanks.

Hugh

> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 4bab9b1..8726da5 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -215,7 +215,6 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
>  	int len, ret;
>  
>  	mutex_lock(&mapping->host->i_mutex);
> -	index = pos >> PAGE_CACHE_SHIFT;
>  	offset = pos & ((pgoff_t)PAGE_CACHE_SIZE - 1);
>  	bv_offs = bvec->bv_offset;
>  	len = bvec->bv_len;
> @@ -226,6 +225,7 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
>  		struct page *page;
>  		void *fsdata;
>  
> +		index = pos >> PAGE_CACHE_SHIFT;
>  		IV = ((sector_t)index << (PAGE_CACHE_SHIFT - 9))+(offset >> 9);
>  		size = PAGE_CACHE_SIZE - offset;
>  		if (size > len)
> @@ -255,7 +255,6 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
>  		bv_offs += copied;
>  		len -= copied;
>  		offset = 0;
> -		index++;
>  		pos += copied;
>  	}
>  	ret = 0;

[patch] new aop loop fix

[Dmitriy Monakhov]

	loop.c code itself is not perfect. In fact before Nick's patch
	partial write was't possible. Assumption what write chunks are
	always page aligned is realy weird ( see "index++" line).

Signed-off-by: Dmitriy Monakhov <dmonakhov@openvz.org>

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 4bab9b1..8726da5 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -215,7 +215,6 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
 	int len, ret;
 
 	mutex_lock(&mapping->host->i_mutex);
-	index = pos >> PAGE_CACHE_SHIFT;
 	offset = pos & ((pgoff_t)PAGE_CACHE_SIZE - 1);
 	bv_offs = bvec->bv_offset;
 	len = bvec->bv_len;
@@ -226,6 +225,7 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
 		struct page *page;
 		void *fsdata;
 
+		index = pos >> PAGE_CACHE_SHIFT;
 		IV = ((sector_t)index << (PAGE_CACHE_SHIFT - 9))+(offset >> 9);
 		size = PAGE_CACHE_SIZE - offset;
 		if (size > len)
@@ -255,7 +255,6 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
 		bv_offs += copied;
 		len -= copied;
 		offset = 0;
-		index++;
 		pos += copied;
 	}
 	ret = 0;

> 
> 

Re: [patch] new aop loop fix

[Hugh Dickins]

On Wed, 13 Jun 2007, Dmitriy Monakhov wrote:
> Btw: Nick's patches broke  LO_CRYPT_XOR mode,

It would help him if you could describe how.

> but it is ok because
> this feature was absolete and not used by anyone, am i right here?

I know nothing of this; but so long as its code remains in the driver,
I'd say we should be supporting it rather than breaking it.

Hugh

Re: [patch] new aop loop fix

[Dmitriy Monakhov]

On 14:36 Срд 13 Июн     , Hugh Dickins wrote:
> On Wed, 13 Jun 2007, Dmitriy Monakhov wrote:
> 
> > 	loop.c code itself is not perfect. In fact before Nick's patch
> > 	partial write was't possible. Assumption what write chunks are
> > 	always page aligned is realy weird ( see "index++" line).
> > 
> > Signed-off-by: Dmitriy Monakhov <dmonakhov@openvz.org>
> 
> I'm interested, because I'm trying to chase down an -mm bug which
> occasionally leaves me with 1k of zeroes where it shouldn't (in a
> 1k bsize ext2 looped over tmpfs).  The length of time for this to
> happen varies a lot, so bisection has been misleading: maybe the
> problem lies in Nick's patches, maybe it does not.
> 
> But I don't understand your fix below at all.  _If_ you need to
> change the handling of index, then you need to change the handling
> of offset too, don't you?
> 
> But what's wrong with how inded was handled anyway?  Yes, it might
> be being incremented at the bottom of the loop when we haven't
> reached the end of this page, but in that case we're not going
> round the loop again anyway: len will now be 0.  So no problem.
> 
> One of us is missing something: please enlighten me - thanks.
Yepp. You absolutely right, wrong patch was attached :(
Btw: Nick's patches broke  LO_CRYPT_XOR mode, but it is ok because
this feature was absolete and not used by anyone, am i right here?
> 
> Hugh
> 
> > 
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 4bab9b1..8726da5 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -215,7 +215,6 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
> >  	int len, ret;
> >  
> >  	mutex_lock(&mapping->host->i_mutex);
> > -	index = pos >> PAGE_CACHE_SHIFT;
> >  	offset = pos & ((pgoff_t)PAGE_CACHE_SIZE - 1);
> >  	bv_offs = bvec->bv_offset;
> >  	len = bvec->bv_len;
> > @@ -226,6 +225,7 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
> >  		struct page *page;
> >  		void *fsdata;
> >  
> > +		index = pos >> PAGE_CACHE_SHIFT;
> >  		IV = ((sector_t)index << (PAGE_CACHE_SHIFT - 9))+(offset >> 9);
> >  		size = PAGE_CACHE_SIZE - offset;
> >  		if (size > len)
> > @@ -255,7 +255,6 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
> >  		bv_offs += copied;
> >  		len -= copied;
> >  		offset = 0;
> > -		index++;
> >  		pos += copied;
> >  	}
> >  	ret = 0;
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] deny partial write for loop dev fd

[Dmitriy Monakhov]

Partial write can be easily supported by LO_CRYPT_NONE mode, but
it is not easy in LO_CRYPT_CRYPTOAPI case, because of its block nature.
I don't know who still used cryptoapi, but theoretically it is possible.
So let's leave things as they are. Loop device doesn't support partial
write before Nick's "write_begin/write_end" patch set, and let's it 
behave the same way after.

Signed-off-by: Dmitriy Monakhov <dmonakhov@openvz.org>
---
 drivers/block/loop.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 4bab9b1..de122f3 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -244,10 +244,8 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
 
 		ret = pagecache_write_end(file, mapping, pos, size, copied,
 							page, fsdata);
-		if (ret < 0)
+		if (ret < 0 || ret != copied)
 			goto fail;
-		if (ret < copied)
-			copied = ret;
 
 		if (unlikely(transfer_result))
 			goto fail;
-- 
1.5.2

Re: [PATCH] deny partial write for loop dev fd

[Nick Piggin]

On Sat, Jun 16, 2007 at 07:39:17PM +0400, Dmitriy Monakhov wrote:
> Partial write can be easily supported by LO_CRYPT_NONE mode, but
> it is not easy in LO_CRYPT_CRYPTOAPI case, because of its block nature.
> I don't know who still used cryptoapi, but theoretically it is possible.
> So let's leave things as they are. Loop device doesn't support partial
> write before Nick's "write_begin/write_end" patch set, and let's it 
> behave the same way after.

OK... but just bailing out here doesn't exactly solve the problem,
does it? Some data is already written at this point, so failing
just means that the underlying file just gets corrupted instead.

OTOH, my attempt to support partial writes isn't right anyway,
because it doesn't get propogated back to the caller correctly
anyway.

So we'll go with your patch, thanks.


> 
> Signed-off-by: Dmitriy Monakhov <dmonakhov@openvz.org>
> ---
>  drivers/block/loop.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 4bab9b1..de122f3 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -244,10 +244,8 @@ static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec,
>  
>  		ret = pagecache_write_end(file, mapping, pos, size, copied,
>  							page, fsdata);
> -		if (ret < 0)
> +		if (ret < 0 || ret != copied)
>  			goto fail;
> -		if (ret < copied)
> -			copied = ret;
>  
>  		if (unlikely(transfer_result))
>  			goto fail;
> -- 
> 1.5.2
>