| Message ID | 20170511164809.29739-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 11, 2017 at 06:48:09PM +0200, Jan Kara wrote: > Add tests for bugs found in ext4 & xfs SEEK_HOLE implementations > fixed by following patches: > > xfs: Fix missed holes in SEEK_HOLE implementation > ext4: Fix SEEK_HOLE > > Signed-off-by: Jan Kara <jack@suse.cz> This will cause ext4 and xfs start to fail with current linus tree and appear as a new regression. So we usually don't add new tests to existing cases. But seek_sanity_test.c deals with different SEEK_DATA/HOLE implentations nicely, which would be a bit tricky to do in a new test by shell, and it has all the infrastructures for new tests like this. So I think I'd prefer merging this patch as is, and document the false regression alert in release announce email. I'm including this patch in release testing now, please shout if ext4 and/or xfs developers have different thoughts, there'll be around two days before new release :) Thanks, Eryu > --- > src/seek_sanity_test.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 96 insertions(+) > > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c > index a6dd48cc257b..86a9397fa7e9 100644 > --- a/src/seek_sanity_test.c > +++ b/src/seek_sanity_test.c > @@ -246,6 +246,100 @@ out: > } > > /* > + * test file with unwritten extents, only have pagevec worth of dirty pages > + * in page cache, a hole and then another page. > + */ > +static int test14(int fd, int testnum) > +{ > + int ret = 0; > + char *buf = NULL; > + int bufsz = sysconf(_SC_PAGE_SIZE) * 14; > + int filsz = 4 << 20; > + > + /* HOLE - unwritten DATA in dirty page */ > + /* Each unit is bufsz */ > + buf = do_malloc(bufsz); > + if (!buf) > + goto out; > + memset(buf, 'a', bufsz); > + > + /* preallocate 4M space to file */ > + ret = do_fallocate(fd, 0, filsz, 0); > + if (ret < 0) { > + /* Report success if fs doesn't support fallocate */ > + if (errno == EOPNOTSUPP) { > + fprintf(stdout, "Test skipped as fs doesn't support fallocate.\n"); > + ret = 0; > + } > + goto out; > + } > + > + ret = do_pwrite(fd, buf, bufsz, 0); > + if (ret) > + goto out; > + > + ret = do_pwrite(fd, buf, bufsz, 3 * bufsz); > + if (ret) > + goto out; > + > + /* offset at the beginning */ > + ret += do_lseek(testnum, 1, fd, filsz, SEEK_HOLE, 0, bufsz); > + ret += do_lseek(testnum, 2, fd, filsz, SEEK_HOLE, 1, bufsz); > + ret += do_lseek(testnum, 2, fd, filsz, SEEK_HOLE, 3 * bufsz, 4 * bufsz); > + ret += do_lseek(testnum, 3, fd, filsz, SEEK_DATA, 0, 0); > + ret += do_lseek(testnum, 4, fd, filsz, SEEK_DATA, 1, 1); > + ret += do_lseek(testnum, 3, fd, filsz, SEEK_DATA, bufsz, 3 * bufsz); > + > +out: > + do_free(buf); > + return ret; > +} > + > +/* > + * test file with unwritten extents, only have pagevec worth of dirty pages > + * in page cache. > + */ > +static int test13(int fd, int testnum) > +{ > + int ret = 0; > + char *buf = NULL; > + int bufsz = sysconf(_SC_PAGE_SIZE) * 14; > + int filsz = 4 << 20; > + > + /* HOLE - unwritten DATA in dirty page */ > + /* Each unit is bufsz */ > + buf = do_malloc(bufsz); > + if (!buf) > + goto out; > + memset(buf, 'a', bufsz); > + > + /* preallocate 4M space to file */ > + ret = do_fallocate(fd, 0, filsz, 0); > + if (ret < 0) { > + /* Report success if fs doesn't support fallocate */ > + if (errno == EOPNOTSUPP) { > + fprintf(stdout, "Test skipped as fs doesn't support fallocate.\n"); > + ret = 0; > + } > + goto out; > + } > + > + ret = do_pwrite(fd, buf, bufsz, 0); > + if (ret) > + goto out; > + > + /* offset at the beginning */ > + ret += do_lseek(testnum, 1, fd, filsz, SEEK_HOLE, 0, bufsz); > + ret += do_lseek(testnum, 2, fd, filsz, SEEK_HOLE, 1, bufsz); > + ret += do_lseek(testnum, 3, fd, filsz, SEEK_DATA, 0, 0); > + ret += do_lseek(testnum, 4, fd, filsz, SEEK_DATA, 1, 1); > + > +out: > + do_free(buf); > + return ret; > +} > + > +/* > * Test huge file to check for overflows of block counts due to usage of > * 32-bit types. > */ > @@ -678,6 +772,8 @@ struct testrec seek_tests[] = { > { 10, test10, "Test a huge file for offset overflow" }, > { 11, test11, "Test a huge file for block number signed" }, > { 12, test12, "Test a huge file for block number overflow" }, > + { 13, test13, "Test file with unwritten extents, only have pagevec dirty pages" }, > + { 14, test14, "Test file with unwritten extents, small hole after pagevec dirty pages" }, > }; > > static int run_test(struct testrec *tr) > -- > 2.12.0 > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 12-05-17 16:04:43, Eryu Guan wrote: > On Thu, May 11, 2017 at 06:48:09PM +0200, Jan Kara wrote: > > Add tests for bugs found in ext4 & xfs SEEK_HOLE implementations > > fixed by following patches: > > > > xfs: Fix missed holes in SEEK_HOLE implementation > > ext4: Fix SEEK_HOLE > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > This will cause ext4 and xfs start to fail with current linus tree and > appear as a new regression. So we usually don't add new tests to > existing cases. > > But seek_sanity_test.c deals with different SEEK_DATA/HOLE implentations > nicely, which would be a bit tricky to do in a new test by shell, and it > has all the infrastructures for new tests like this. So I think I'd > prefer merging this patch as is, and document the false regression alert > in release announce email. Yeah, I agree regressing a test is not ideal but at least from the full output it would be visible that the new testcases are those that failed and at that point it is same as if a new test was failing. > I'm including this patch in release testing now, please shout if ext4 > and/or xfs developers have different thoughts, there'll be around two > days before new release :) Thanks! Honza
On Fri, May 12, 2017 at 04:04:43PM +0800, Eryu Guan wrote: > On Thu, May 11, 2017 at 06:48:09PM +0200, Jan Kara wrote: > > Add tests for bugs found in ext4 & xfs SEEK_HOLE implementations > > fixed by following patches: > > > > xfs: Fix missed holes in SEEK_HOLE implementation > > ext4: Fix SEEK_HOLE > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > This will cause ext4 and xfs start to fail with current linus tree and > appear as a new regression. So we usually don't add new tests to > existing cases. > > But seek_sanity_test.c deals with different SEEK_DATA/HOLE implentations > nicely, which would be a bit tricky to do in a new test by shell, and it > has all the infrastructures for new tests like this. So I think I'd > prefer merging this patch as is, and document the false regression alert > in release announce email. Make the new tests optional (i.e. on a cli switch) and add a new xfstest that runs them? Old test remains unchanged, doesn't fail, new test covers the new tests, will fail on old kernels (which is ok for new tests). Cheers, Dave.
On Sat, May 13, 2017 at 09:06:57AM +1000, Dave Chinner wrote: > On Fri, May 12, 2017 at 04:04:43PM +0800, Eryu Guan wrote: > > On Thu, May 11, 2017 at 06:48:09PM +0200, Jan Kara wrote: > > > Add tests for bugs found in ext4 & xfs SEEK_HOLE implementations > > > fixed by following patches: > > > > > > xfs: Fix missed holes in SEEK_HOLE implementation > > > ext4: Fix SEEK_HOLE > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > This will cause ext4 and xfs start to fail with current linus tree and > > appear as a new regression. So we usually don't add new tests to > > existing cases. > > > > But seek_sanity_test.c deals with different SEEK_DATA/HOLE implentations > > nicely, which would be a bit tricky to do in a new test by shell, and it > > has all the infrastructures for new tests like this. So I think I'd > > prefer merging this patch as is, and document the false regression alert > > in release announce email. > > Make the new tests optional (i.e. on a cli switch) and add a new > xfstest that runs them? Old test remains unchanged, doesn't fail, > new test covers the new tests, will fail on old kernels (which is ok > for new tests). Yeah, this should work and looks a better solution to me. This avoids regressing generic/285 again when adding another new test in future, future tests could just follow this path too. Thanks for the suggestion! Jan, could you please update the patch and, as suggested by Dave, make it a new test? I can do it too if you like. Thanks, Eryu -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun 14-05-17 01:26:21, Eryu Guan wrote: > On Sat, May 13, 2017 at 09:06:57AM +1000, Dave Chinner wrote: > > On Fri, May 12, 2017 at 04:04:43PM +0800, Eryu Guan wrote: > > > On Thu, May 11, 2017 at 06:48:09PM +0200, Jan Kara wrote: > > > > Add tests for bugs found in ext4 & xfs SEEK_HOLE implementations > > > > fixed by following patches: > > > > > > > > xfs: Fix missed holes in SEEK_HOLE implementation > > > > ext4: Fix SEEK_HOLE > > > > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > > > This will cause ext4 and xfs start to fail with current linus tree and > > > appear as a new regression. So we usually don't add new tests to > > > existing cases. > > > > > > But seek_sanity_test.c deals with different SEEK_DATA/HOLE implentations > > > nicely, which would be a bit tricky to do in a new test by shell, and it > > > has all the infrastructures for new tests like this. So I think I'd > > > prefer merging this patch as is, and document the false regression alert > > > in release announce email. > > > > Make the new tests optional (i.e. on a cli switch) and add a new > > xfstest that runs them? Old test remains unchanged, doesn't fail, > > new test covers the new tests, will fail on old kernels (which is ok > > for new tests). > > Yeah, this should work and looks a better solution to me. This avoids > regressing generic/285 again when adding another new test in future, > future tests could just follow this path too. Thanks for the suggestion! > > Jan, could you please update the patch and, as suggested by Dave, make > it a new test? I can do it too if you like. Yeah, Dave's idea looks good. I'll work on it and send an updated version. Honza
diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c index a6dd48cc257b..86a9397fa7e9 100644 --- a/src/seek_sanity_test.c +++ b/src/seek_sanity_test.c @@ -246,6 +246,100 @@ out: } /* + * test file with unwritten extents, only have pagevec worth of dirty pages + * in page cache, a hole and then another page. + */ +static int test14(int fd, int testnum) +{ + int ret = 0; + char *buf = NULL; + int bufsz = sysconf(_SC_PAGE_SIZE) * 14; + int filsz = 4 << 20; + + /* HOLE - unwritten DATA in dirty page */ + /* Each unit is bufsz */ + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + /* preallocate 4M space to file */ + ret = do_fallocate(fd, 0, filsz, 0); + if (ret < 0) { + /* Report success if fs doesn't support fallocate */ + if (errno == EOPNOTSUPP) { + fprintf(stdout, "Test skipped as fs doesn't support fallocate.\n"); + ret = 0; + } + goto out; + } + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + ret = do_pwrite(fd, buf, bufsz, 3 * bufsz); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, filsz, SEEK_HOLE, 0, bufsz); + ret += do_lseek(testnum, 2, fd, filsz, SEEK_HOLE, 1, bufsz); + ret += do_lseek(testnum, 2, fd, filsz, SEEK_HOLE, 3 * bufsz, 4 * bufsz); + ret += do_lseek(testnum, 3, fd, filsz, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 4, fd, filsz, SEEK_DATA, 1, 1); + ret += do_lseek(testnum, 3, fd, filsz, SEEK_DATA, bufsz, 3 * bufsz); + +out: + do_free(buf); + return ret; +} + +/* + * test file with unwritten extents, only have pagevec worth of dirty pages + * in page cache. + */ +static int test13(int fd, int testnum) +{ + int ret = 0; + char *buf = NULL; + int bufsz = sysconf(_SC_PAGE_SIZE) * 14; + int filsz = 4 << 20; + + /* HOLE - unwritten DATA in dirty page */ + /* Each unit is bufsz */ + buf = do_malloc(bufsz); + if (!buf) + goto out; + memset(buf, 'a', bufsz); + + /* preallocate 4M space to file */ + ret = do_fallocate(fd, 0, filsz, 0); + if (ret < 0) { + /* Report success if fs doesn't support fallocate */ + if (errno == EOPNOTSUPP) { + fprintf(stdout, "Test skipped as fs doesn't support fallocate.\n"); + ret = 0; + } + goto out; + } + + ret = do_pwrite(fd, buf, bufsz, 0); + if (ret) + goto out; + + /* offset at the beginning */ + ret += do_lseek(testnum, 1, fd, filsz, SEEK_HOLE, 0, bufsz); + ret += do_lseek(testnum, 2, fd, filsz, SEEK_HOLE, 1, bufsz); + ret += do_lseek(testnum, 3, fd, filsz, SEEK_DATA, 0, 0); + ret += do_lseek(testnum, 4, fd, filsz, SEEK_DATA, 1, 1); + +out: + do_free(buf); + return ret; +} + +/* * Test huge file to check for overflows of block counts due to usage of * 32-bit types. */ @@ -678,6 +772,8 @@ struct testrec seek_tests[] = { { 10, test10, "Test a huge file for offset overflow" }, { 11, test11, "Test a huge file for block number signed" }, { 12, test12, "Test a huge file for block number overflow" }, + { 13, test13, "Test file with unwritten extents, only have pagevec dirty pages" }, + { 14, test14, "Test file with unwritten extents, small hole after pagevec dirty pages" }, }; static int run_test(struct testrec *tr)
Add tests for bugs found in ext4 & xfs SEEK_HOLE implementations fixed by following patches: xfs: Fix missed holes in SEEK_HOLE implementation ext4: Fix SEEK_HOLE Signed-off-by: Jan Kara <jack@suse.cz> --- src/seek_sanity_test.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)