new aop loop fix

PATCH In review Dmitriy Monakhov 3 participants 6 messages Jun 13, 2007 – Jun 18, 2007 Block layer

Share & tools

Hugh Dickins

2007-06-13 13:36 UTC #1 lore raw

Quoted 5 lines · 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 <[email protected]>

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

Quoted 29 lines

drivers/block/loop.c +1 -2

index 4bab9b1..8726da5 100644
@@ -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;

Dmitriy Monakhov

author

2007-06-13 14:29 UTC #2 lore raw

Quoted 18 lines · Hugh Dickins

Quoted 5 lines · 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 <[email protected]>
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?

Quoted 6 lines

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?

Quoted 37 lines

Hugh

Quoted 22 lines

drivers/block/loop.c +1 -1

index 4bab9b1..8726da5 100644
@@ -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)

drivers/block/loop.c static int do_lo_send_aops(struct loop_device *lo, struct bio_vec *bvec, · line 260

Quoted 7 lines

drivers/block/loop.c +0 -1

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hugh Dickins

2007-06-13 14:48 UTC #3 lore raw

Quoted 1 line · Dmitriy Monakhov

Btw: Nick's patches broke LO_CRYPT_XOR mode,

It would help him if you could describe how.

Quoted 2 lines

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

Dmitriy Monakhov

author

2007-06-16 15:39 UTC #4 lore raw

PATCH 1/1 deny partial write for loop dev fd

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 <[email protected]>

---
 drivers/block/loop.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

drivers/block/loop.c +1 -3

index 4bab9b1..de122f3 100644
@@ -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;

Signature · 1 line

1.5.2

Nick Piggin

2007-06-18 02:39 UTC #5 lore raw

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

Quoted 6 lines · 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.

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.

Quoted 25 lines

Signed-off-by Dmitriy Monakhov <[email protected]>

---
 drivers/block/loop.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

drivers/block/loop.c +1 -3

index 4bab9b1..de122f3 100644
@@ -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;

Signature · 1 line

1.5.2