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]>
---
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index d0a2afb..4a825c1 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -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);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 8b8bae4..9333dc9 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -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;
On Fri, Jan 29, 2010 at 12:24:04PM -0600, Eric Sandeen wrote:
> 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]>
> ---
>
Reviewed-by: Josef Bacik <[email protected]>
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
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -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);
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 8b8bae4..9333dc9 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -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;
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index e233879..4f36f7d 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -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,"
On Fri, Jan 29, 2010 at 02:28:23PM -0600, Eric Sandeen wrote:
> 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()
>
Reviewed-by: Josef Back <[email protected]>
probably should have said something before replying to the last email :).
Thanks,
Josef
On Fri, Jan 29, 2010 at 02:28:23PM -0600, Eric Sandeen wrote:
> 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]>
> ---
This looks important enough (and low risk enough) to push to Linus
before the next merge window. Comments?
- Ted
[email protected] wrote:
> On Fri, Jan 29, 2010 at 02:28:23PM -0600, Eric Sandeen wrote:
>> 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]>
>> ---
>
> 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
On Fri, Jan 29, 2010 at 02:28:23PM -0600, Eric Sandeen wrote:
> 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]>
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
[email protected] wrote:
> On Fri, Jan 29, 2010 at 02:28:23PM -0600, Eric Sandeen wrote:
>> 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]>
>
> 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
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 2009-08-05 20:19:40.380978986 -0500
+++ 126.out.bad 2010-02-05 09:01:13.088372667 -0600
@@ -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
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