| Message ID | 4B6344E7.9050503@redhat.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
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 <giel@vectorwise.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > V2: Post-review Josef spotted another size_t in ext4_end_aio_dio_nolock() > Reviewed-by: Josef Back <josef@redhat.com> probably should have said something before replying to the last email :). Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <giel@vectorwise.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- This looks important enough (and low risk enough) to push to Linus before the next merge window. Comments? - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
tytso@mit.edu 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 <giel@vectorwise.com> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <giel@vectorwise.com> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 05, 2010 at 08:37:51AM -0500, tytso@MIT.EDU 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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,"
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 <giel@vectorwise.com> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html