fix type of "offset" in ext4_io_end

PATCH Superseded Eric Sandeen 3 participants 9 messages Jan 29, 2010 – Feb 5, 2010 ext4 file system

lore

Share & tools

lore

Eric Sandeen

author

2010-01-29 18:24 UTC #0 lore raw

PATCH 1/1 fix type of "offset" in ext4_io_end

The "offset" member in ext4_io_end holds bytes, not blocks, so ext4_lblk_t is wrong - and too small (u32)

This caused the testcase "Possible ext4 data corruption with large files and async I/O" sent by Giel to fail when it wrapped around to 0.

Also fix up the type of arguments to ext4_convert_unwritten_extents(), it gets ssize_t from ext4_end_aio_dio_nolock() and ext4_ext_direct_IO()

Reported-by Giel de Nijs <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fs/ext4/ext4.h +3 -3

index d0a2afb..4a825c1 100644
@@ -139,8 +139,8 @@ typedef struct ext4_io_end {
 	struct inode		*inode;		/* file being written to */
 	unsigned int		flag;		/* unwritten or not */
 	int			error;		/* I/O error code */
-	ext4_lblk_t		offset;		/* offset in the file */
-	size_t			size;		/* size of the extent */
+	loff_t			offset;		/* offset in the file */
+	ssize_t			size;		/* size of the extent */
 	struct work_struct	work;		/* data work queue */
 } ext4_io_end_t;

@@ -1740,7 +1740,7 @@ extern void ext4_ext_release(struct super_block *);
 extern long ext4_fallocate(struct inode *inode, int mode, loff_t offset,
 			  loff_t len);
 extern int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset,
-			  loff_t len);
+			  ssize_t len);
 extern int ext4_get_blocks(handle_t *handle, struct inode *inode,
 			   sector_t block, unsigned int max_blocks,
 			   struct buffer_head *bh, int flags);

fs/ext4/extents.c +1 -1

index 8b8bae4..9333dc9 100644
@@ -3547,7 +3547,7 @@ retry:
  * Returns 0 on success.
  */
 int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset,
-				    loff_t len)
+				    ssize_t len)
 {
 	handle_t *handle;
 	ext4_lblk_t block;

Josef Bacik

Reviewed-by on v1 1/1

2010-01-29 20:21 UTC #1 lore raw

Quoted 7 lines · Eric Sandeen

The "offset" member in ext4_io_end holds bytes, not blocks, so ext4_lblk_t is wrong - and too small (u32)
This caused the testcase "Possible ext4 data corruption with large files and async I/O" sent by Giel to fail when it wrapped around to 0.

Quoted 8 lines

Also fix up the type of arguments to ext4_convert_unwritten_extents(), it gets ssize_t from ext4_end_aio_dio_nolock() and ext4_ext_direct_IO()
Reported-by Giel de Nijs <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---

Reviewed-by Josef Bacik <[email protected]>

Eric Sandeen

author

2010-01-29 20:28 UTC #2 lore raw

PATCH v2 1/1 fix type of "offset" in ext4_io_end

In reply to Eric Sandeen

The "offset" member in ext4_io_end holds bytes, not blocks, so ext4_lblk_t is wrong - and too small (u32)

This caused the testcase "Possible ext4 data corruption with large files and async I/O" sent by Giel to fail when it wrapped around to 0.

Also fix up the type of arguments to ext4_convert_unwritten_extents(), it gets ssize_t from ext4_end_aio_dio_nolock() and ext4_ext_direct_IO()

Reported-by Giel de Nijs <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

V2: Post-review Josef spotted another size_t in ext4_end_aio_dio_nolock()

---diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index d0a2afb..4a825c1 100644

fs/ext4/ext4.h +3 -3

@@ -139,8 +139,8 @@ typedef struct ext4_io_end {
 	struct inode		*inode;		/* file being written to */
 	unsigned int		flag;		/* unwritten or not */
 	int			error;		/* I/O error code */
-	ext4_lblk_t		offset;		/* offset in the file */
-	size_t			size;		/* size of the extent */
+	loff_t			offset;		/* offset in the file */
+	ssize_t			size;		/* size of the extent */
 	struct work_struct	work;		/* data work queue */
 } ext4_io_end_t;

@@ -1740,7 +1740,7 @@ extern void ext4_ext_release(struct super_block *);
 extern long ext4_fallocate(struct inode *inode, int mode, loff_t offset,
 			  loff_t len);
 extern int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset,
-			  loff_t len);
+			  ssize_t len);
 extern int ext4_get_blocks(handle_t *handle, struct inode *inode,
 			   sector_t block, unsigned int max_blocks,
 			   struct buffer_head *bh, int flags);

fs/ext4/extents.c +1 -1

index 8b8bae4..9333dc9 100644
@@ -3547,7 +3547,7 @@ retry:
  * Returns 0 on success.
  */
 int ext4_convert_unwritten_extents(struct inode *inode, loff_t offset,
-				    loff_t len)
+				    ssize_t len)
 {
 	handle_t *handle;
 	ext4_lblk_t block;

fs/ext4/inode.c +1 -1

index e233879..4f36f7d 100644
@@ -3540,7 +3540,7 @@ static int ext4_end_aio_dio_nolock(ext4_io_end_t *io)
 {
 	struct inode *inode = io->inode;
 	loff_t offset = io->offset;
-	size_t size = io->size;
+	ssize_t size = io->size;
 	int ret = 0;
 
 	ext4_debug("end_aio_dio_onlock: io 0x%p from inode %lu,list->next 0x%p,"

Josef Bacik

Reviewed-by on v2 1/1

2010-01-29 20:32 UTC #3 lore raw

Quoted 11 lines · Eric Sandeen

The "offset" member in ext4_io_end holds bytes, not blocks, so ext4_lblk_t is wrong - and too small (u32)
This caused the testcase "Possible ext4 data corruption with large files and async I/O" sent by Giel to fail when it wrapped around to 0.
Also fix up the type of arguments to ext4_convert_unwritten_extents(), it gets ssize_t from ext4_end_aio_dio_nolock() and ext4_ext_direct_IO()

Quoted 6 lines

Reported-by Giel de Nijs <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---
V2: Post-review Josef spotted another size_t in ext4_end_aio_dio_nolock()

Reviewed-by Josef Back <[email protected]>

probably should have said something before replying to the last email :). Thanks,

Josef

[email protected]

maintainer

2010-01-30 17:35 UTC #4 lore raw

In reply to Eric Sandeen

Quoted 7 lines · Eric Sandeen

The "offset" member in ext4_io_end holds bytes, not blocks, so ext4_lblk_t is wrong - and too small (u32)
This caused the testcase "Possible ext4 data corruption with large files and async I/O" sent by Giel to fail when it wrapped around to 0.

Quoted 7 lines

Also fix up the type of arguments to ext4_convert_unwritten_extents(), it gets ssize_t from ext4_end_aio_dio_nolock() and ext4_ext_direct_IO()
Reported-by Giel de Nijs <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---

This looks important enough (and low risk enough) to push to Linus
before the next merge window. Comments?

- Ted

Eric Sandeen

author

2010-01-30 18:34 UTC #5 lore raw

Quoted 12 lines · [email protected] wrote:

Quoted 11 lines · Eric Sandeen
The "offset" member in ext4_io_end holds bytes, not blocks, so ext4_lblk_t is wrong - and too small (u32)
This caused the testcase "Possible ext4 data corruption with large files and async I/O" sent by Giel to fail when it wrapped around to 0.
Also fix up the type of arguments to ext4_convert_unwritten_extents(), it gets ssize_t from ext4_end_aio_dio_nolock() and ext4_ext_direct_IO()

Quoted 8 lines

Quoted 3 lines
Reported-by Giel de Nijs <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
---
This looks important enough (and low risk enough) to push to Linus
before the next merge window. Comments?
- Ted

yeah, I think so.

if there's any concern over the size_t type changes I could split it up.

-Eric

[email protected]

maintainer

2010-02-05 13:37 UTC #6 lore raw

In reply to Eric Sandeen

Quoted 7 lines · Eric Sandeen

The "offset" member in ext4_io_end holds bytes, not blocks, so ext4_lblk_t is wrong - and too small (u32)
This caused the testcase "Possible ext4 data corruption with large files and async I/O" sent by Giel to fail when it wrapped around to 0.

Quoted 6 lines

Also fix up the type of arguments to ext4_convert_unwritten_extents(), it gets ssize_t from ext4_end_aio_dio_nolock() and ext4_ext_direct_IO()
Reported-by Giel de Nijs <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>

So I was going to submit this patch to Linus, but the last two times I've run xfsqa ("xfsqa -g quick" and "xfsqa -g auto"), test #126 has failed. If I run the test stand-alone, it passes. It's a bit of a head-scratcher. I'm currently backing out the patch and trying to do an xfsqa -g auto run to make sure this was something that had crept in before applying this patch, so this may very well not be this patch --- I certainly can't see anything wrong with it. But I thought I would give a heads up....

- Ted

Eric Sandeen

author

2010-02-05 15:51 UTC #7 lore raw

Quoted 24 lines · [email protected] wrote:

Quoted 7 lines · Eric Sandeen
The "offset" member in ext4_io_end holds bytes, not blocks, so ext4_lblk_t is wrong - and too small (u32)
This caused the testcase "Possible ext4 data corruption with large files and async I/O" sent by Giel to fail when it wrapped around to 0.
Quoted 6 lines
Also fix up the type of arguments to ext4_convert_unwritten_extents(), it gets ssize_t from ext4_end_aio_dio_nolock() and ext4_ext_direct_IO()
Reported-by Giel de Nijs <[email protected]>
Signed-off-by Eric Sandeen <[email protected]>
So I was going to submit this patch to Linus, but the last two times I've run xfsqa ("xfsqa -g quick" and "xfsqa -g auto"), test #126 has failed. If I run the test stand-alone, it passes. It's a bit of a head-scratcher. I'm currently backing out the patch and trying to do an xfsqa -g auto run to make sure this was something that had crept in before applying this patch, so this may very well not be this patch --- I certainly can't see anything wrong with it. But I thought I would give a heads up....

Quoted 1 line

- Ted

hrm 126 is a permissions test, I can't imagine it'd be related...

I see it fail on a sorta-stock .32 kernel like this:

126.out.bad renamed from 126.out +1 -1

@@ -5,7 +5,7 @@
 w a 002 file owned by (99/99) as user/group(12/100)  PASS
 w a 020 file owned by (99/99) as user/group(200/99)  PASS
 w a 200 file owned by (99/99) as user/group(99/500)  PASS
-r a 004 file owned by (99/99) as user/group(12/100)  PASS
+r a 004 file owned by (99/99) as user/group(12/100)  FAIL
 r a 040 file owned by (99/99) as user/group(200/99)  PASS
 r a 400 file owned by (99/99) as user/group(99/500)  PASS
 r a 000 file owned by (99/99) as user/group(99/99)  FAIL

What's your failure?

-Eric

[email protected]

maintainer

2010-02-05 16:34 UTC #8 lore raw

In reply to [email protected]

Quoted 8 lines · On Fri, Feb 05, 2010 at 08:37:51AM -0500, [email protected] wrote:

So I was going to submit this patch to Linus, but the last two times I've run xfsqa ("xfsqa -g quick" and "xfsqa -g auto"), test #126 has failed. If I run the test stand-alone, it passes. It's a bit of a head-scratcher. I'm currently backing out the patch and trying to do an xfsqa -g auto run to make sure this was something that had crept in before applying this patch, so this may very well not be this patch --- I certainly can't see anything wrong with it. But I thought I would give a heads up....

Eric and I did more looking at this, and looks like it's a busted test. So we're considering this a false alarm for now.

(Safety Pup says: "Null terminate your arguments to fopen!")

- Ted