[PATCH 0/3 v2] xfs: Fix SEEK_HOLE implementation

[PATCH 0/3 v2] xfs: Fix SEEK_HOLE implementation

[Jan Kara]

Hello,

this is the second revision of the patches to fix bugs in XFS's SEEK_HOLE
implementation and cleanup the code a bit.

Changes since v1:
* Fixed some more buggy cases
* Simplified code a bit as suggested by Darrick
* Fixed range check as spotted by Brian

								Honza

[PATCH 1/3] xfs: Fix missed holes in SEEK_HOLE implementation

[Jan Kara]

XFS SEEK_HOLE implementation could miss a hole in an unwritten extent as
can be seen by the following command:

xfs_io -c "falloc 0 256k" -c "pwrite 0 56k" -c "pwrite 128k 8k"
       -c "seek -h 0" file
wrote 57344/57344 bytes at offset 0
56 KiB, 14 ops; 0.0000 sec (49.312 MiB/sec and 12623.9856 ops/sec)
wrote 8192/8192 bytes at offset 131072
8 KiB, 2 ops; 0.0000 sec (70.383 MiB/sec and 18018.0180 ops/sec)
Whence	Result
HOLE	139264

Where we can see that hole at offset 56k was just ignored by SEEK_HOLE
implementation. The bug is in xfs_find_get_desired_pgoff() which does
not properly detect the case when pages are not contiguous.

Fix the problem by properly detecting when found page has larger offset
than expected.

CC: stable@vger.kernel.org
Fixes: d126d43f631f996daeee5006714fed914be32368
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_file.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 35703a801372..f371812e20c6 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1076,17 +1076,6 @@ xfs_find_get_desired_pgoff(
 			break;
 		}
 
-		/*
-		 * At lease we found one page.  If this is the first time we
-		 * step into the loop, and if the first page index offset is
-		 * greater than the given search offset, a hole was found.
-		 */
-		if (type == HOLE_OFF && lastoff == startoff &&
-		    lastoff < page_offset(pvec.pages[0])) {
-			found = true;
-			break;
-		}
-
 		for (i = 0; i < nr_pages; i++) {
 			struct page	*page = pvec.pages[i];
 			loff_t		b_offset;
@@ -1098,18 +1087,18 @@ xfs_find_get_desired_pgoff(
 			 * file mapping. However, page->index will not change
 			 * because we have a reference on the page.
 			 *
-			 * Searching done if the page index is out of range.
-			 * If the current offset is not reaches the end of
-			 * the specified search range, there should be a hole
-			 * between them.
+			 * If current page offset is beyond where we've ended,
+			 * we've found a hole.
 			 */
-			if (page->index > end) {
-				if (type == HOLE_OFF && lastoff < endoff) {
-					*offset = lastoff;
-					found = true;
-				}
+			if (type == HOLE_OFF && lastoff < endoff &&
+			    lastoff < page_offset(pvec.pages[i])) {
+				found = true;
+				*offset = lastoff;
 				goto out;
 			}
+			/* Searching done if the page index is out of range. */
+			if (page->index > end)
+				goto out;
 
 			lock_page(page);
 			/*
-- 
2.12.0

[PATCH 2/3] xfs: Fix off-by-in in loop termination in xfs_find_get_desired_pgoff()

[Jan Kara]

There is an off-by-one error in loop termination conditions in
xfs_find_get_desired_pgoff(). It doesn't have any visible effects but
still it is good to fix it.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index f371812e20c6..8acb8ab79267 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1097,7 +1097,7 @@ xfs_find_get_desired_pgoff(
 				goto out;
 			}
 			/* Searching done if the page index is out of range. */
-			if (page->index > end)
+			if (page->index >= end)
 				goto out;
 
 			lock_page(page);
@@ -1153,7 +1153,7 @@ xfs_find_get_desired_pgoff(
 
 		index = pvec.pages[i - 1]->index + 1;
 		pagevec_release(&pvec);
-	} while (index <= end);
+	} while (index < end);
 
 out:
 	pagevec_release(&pvec);
-- 
2.12.0

[PATCH 3/3] xfs: Move handling of missing page into one place in xfs_find_get_desired_pgoff()

[Jan Kara]

Currently several places in xfs_find_get_desired_pgoff() handle the case
of a missing page. Make them all handled in one place after the loop has
terminated.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/xfs/xfs_file.c | 38 ++++++++------------------------------
 1 file changed, 8 insertions(+), 30 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 8acb8ab79267..ff599ab45e2e 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1052,29 +1052,8 @@ xfs_find_get_desired_pgoff(
 		want = min_t(pgoff_t, end - index, PAGEVEC_SIZE);
 		nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
 					  want);
-		/*
-		 * No page mapped into given range.  If we are searching holes
-		 * and if this is the first time we got into the loop, it means
-		 * that the given offset is landed in a hole, return it.
-		 *
-		 * If we have already stepped through some block buffers to find
-		 * holes but they all contains data.  In this case, the last
-		 * offset is already updated and pointed to the end of the last
-		 * mapped page, if it does not reach the endpoint to search,
-		 * that means there should be a hole between them.
-		 */
-		if (nr_pages == 0) {
-			/* Data search found nothing */
-			if (type == DATA_OFF)
-				break;
-
-			ASSERT(type == HOLE_OFF);
-			if (lastoff == startoff || lastoff < endoff) {
-				found = true;
-				*offset = lastoff;
-			}
+		if (nr_pages == 0)
 			break;
-		}
 
 		for (i = 0; i < nr_pages; i++) {
 			struct page	*page = pvec.pages[i];
@@ -1140,21 +1119,20 @@ xfs_find_get_desired_pgoff(
 
 		/*
 		 * The number of returned pages less than our desired, search
-		 * done.  In this case, nothing was found for searching data,
-		 * but we found a hole behind the last offset.
+		 * done.
 		 */
-		if (nr_pages < want) {
-			if (type == HOLE_OFF) {
-				*offset = lastoff;
-				found = true;
-			}
+		if (nr_pages < want)
 			break;
-		}
 
 		index = pvec.pages[i - 1]->index + 1;
 		pagevec_release(&pvec);
 	} while (index < end);
 
+	/* No page at lastoff and we are not done - we found a hole. */
+	if (type == HOLE_OFF && lastoff < endoff) {
+		*offset = lastoff;
+		found = true;
+	}
 out:
 	pagevec_release(&pvec);
 	return found;
-- 
2.12.0

Re: [PATCH 0/3 v2] xfs: Fix SEEK_HOLE implementation

[Eryu Guan]

Hi Jan,

On Wed, May 17, 2017 at 02:10:43PM +0200, Jan Kara wrote:
> Hello,
> 
> this is the second revision of the patches to fix bugs in XFS's SEEK_HOLE
> implementation and cleanup the code a bit.
> 
> Changes since v1:
> * Fixed some more buggy cases
> * Simplified code a bit as suggested by Darrick
> * Fixed range check as spotted by Brian

I applied this patchset on top of 4.12-rc1 kernel to test your v4 test
case, your new test passed all my tests, but I found generic/285
regressed with sub-page block size XFS, 285.full showed that failure was
from subtest 7

07. Test file with unwritten extents, only have dirty pages
07.01 SEEK_HOLE expected 0 or 11264, got 0.                       succ
07.02 SEEK_HOLE expected 1 or 11264, got 1.                       succ
07.03 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
07.04 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL

And manual test showed subtest 8 failed too

# ./src/seek_sanity_test -s 8 -e 8 /mnt/xfs/testfile
File system magic#: 0x58465342
Allocation size: 4096

08. Test file with unwritten extents, only have unwritten pages
08.01 SEEK_HOLE expected 0 or 5632, got 0.                        succ
08.02 SEEK_HOLE expected 1 or 5632, got 1.                        succ
08.03 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
08.04 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL

Other subtests all passed with sub-page block size XFS.

Thanks,
Eryu

Re: [PATCH 0/3 v2] xfs: Fix SEEK_HOLE implementation

[Jan Kara]

On Wed 17-05-17 20:31:15, Eryu Guan wrote:
> Hi Jan,
> 
> On Wed, May 17, 2017 at 02:10:43PM +0200, Jan Kara wrote:
> > Hello,
> > 
> > this is the second revision of the patches to fix bugs in XFS's SEEK_HOLE
> > implementation and cleanup the code a bit.
> > 
> > Changes since v1:
> > * Fixed some more buggy cases
> > * Simplified code a bit as suggested by Darrick
> > * Fixed range check as spotted by Brian
> 
> I applied this patchset on top of 4.12-rc1 kernel to test your v4 test
> case, your new test passed all my tests, but I found generic/285
> regressed with sub-page block size XFS, 285.full showed that failure was
> from subtest 7
> 
> 07. Test file with unwritten extents, only have dirty pages
> 07.01 SEEK_HOLE expected 0 or 11264, got 0.                       succ
> 07.02 SEEK_HOLE expected 1 or 11264, got 1.                       succ
> 07.03 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
> 07.04 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
> 
> And manual test showed subtest 8 failed too
> 
> # ./src/seek_sanity_test -s 8 -e 8 /mnt/xfs/testfile
> File system magic#: 0x58465342
> Allocation size: 4096
> 
> 08. Test file with unwritten extents, only have unwritten pages
> 08.01 SEEK_HOLE expected 0 or 5632, got 0.                        succ
> 08.02 SEEK_HOLE expected 1 or 5632, got 1.                        succ
> 08.03 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
> 08.04 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
> 
> Other subtests all passed with sub-page block size XFS.

Strange. It doesn't fail for me this way even with 1k blocksize. I'll
investigate more tomorrow.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/3 v2] xfs: Fix SEEK_HOLE implementation

[Jan Kara]

On Wed 17-05-17 16:57:46, Jan Kara wrote:
> On Wed 17-05-17 20:31:15, Eryu Guan wrote:
> > Hi Jan,
> > 
> > On Wed, May 17, 2017 at 02:10:43PM +0200, Jan Kara wrote:
> > > Hello,
> > > 
> > > this is the second revision of the patches to fix bugs in XFS's SEEK_HOLE
> > > implementation and cleanup the code a bit.
> > > 
> > > Changes since v1:
> > > * Fixed some more buggy cases
> > > * Simplified code a bit as suggested by Darrick
> > > * Fixed range check as spotted by Brian
> > 
> > I applied this patchset on top of 4.12-rc1 kernel to test your v4 test
> > case, your new test passed all my tests, but I found generic/285
> > regressed with sub-page block size XFS, 285.full showed that failure was
> > from subtest 7
> > 
> > 07. Test file with unwritten extents, only have dirty pages
> > 07.01 SEEK_HOLE expected 0 or 11264, got 0.                       succ
> > 07.02 SEEK_HOLE expected 1 or 11264, got 1.                       succ
> > 07.03 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
> > 07.04 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
> > 
> > And manual test showed subtest 8 failed too
> > 
> > # ./src/seek_sanity_test -s 8 -e 8 /mnt/xfs/testfile
> > File system magic#: 0x58465342
> > Allocation size: 4096
> > 
> > 08. Test file with unwritten extents, only have unwritten pages
> > 08.01 SEEK_HOLE expected 0 or 5632, got 0.                        succ
> > 08.02 SEEK_HOLE expected 1 or 5632, got 1.                        succ
> > 08.03 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
> > 08.04 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
> > 
> > Other subtests all passed with sub-page block size XFS.
> 
> Strange. It doesn't fail for me this way even with 1k blocksize. I'll
> investigate more tomorrow.

So I've been trying quite hard to reproduce the failure but I failed. Since
you are apparently getting some error out of lseek can you find out which
error it is (likely ENXIO but I'd like to confirm) and where it gets
generated? I don't see how it could possibly happen that SEEK_DATA would
miss that single page generated by this test and how any of my patches
would influence this particular situation. Thanks!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/3 v2] xfs: Fix SEEK_HOLE implementation

[Eryu Guan]

On Thu, May 18, 2017 at 11:03:46AM +0200, Jan Kara wrote:
> On Wed 17-05-17 16:57:46, Jan Kara wrote:
> > On Wed 17-05-17 20:31:15, Eryu Guan wrote:
> > > Hi Jan,
> > > 
> > > On Wed, May 17, 2017 at 02:10:43PM +0200, Jan Kara wrote:
> > > > Hello,
> > > > 
> > > > this is the second revision of the patches to fix bugs in XFS's SEEK_HOLE
> > > > implementation and cleanup the code a bit.
> > > > 
> > > > Changes since v1:
> > > > * Fixed some more buggy cases
> > > > * Simplified code a bit as suggested by Darrick
> > > > * Fixed range check as spotted by Brian
> > > 
> > > I applied this patchset on top of 4.12-rc1 kernel to test your v4 test
> > > case, your new test passed all my tests, but I found generic/285
> > > regressed with sub-page block size XFS, 285.full showed that failure was
> > > from subtest 7
> > > 
> > > 07. Test file with unwritten extents, only have dirty pages
> > > 07.01 SEEK_HOLE expected 0 or 11264, got 0.                       succ
> > > 07.02 SEEK_HOLE expected 1 or 11264, got 1.                       succ
> > > 07.03 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
> > > 07.04 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
> > > 
> > > And manual test showed subtest 8 failed too
> > > 
> > > # ./src/seek_sanity_test -s 8 -e 8 /mnt/xfs/testfile
> > > File system magic#: 0x58465342
> > > Allocation size: 4096
> > > 
> > > 08. Test file with unwritten extents, only have unwritten pages
> > > 08.01 SEEK_HOLE expected 0 or 5632, got 0.                        succ
> > > 08.02 SEEK_HOLE expected 1 or 5632, got 1.                        succ
> > > 08.03 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
> > > 08.04 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
> > > 
> > > Other subtests all passed with sub-page block size XFS.
> > 
> > Strange. It doesn't fail for me this way even with 1k blocksize. I'll
> > investigate more tomorrow.
> 
> So I've been trying quite hard to reproduce the failure but I failed. Since
> you are apparently getting some error out of lseek can you find out which
> error it is (likely ENXIO but I'd like to confirm) and where it gets
> generated? I don't see how it could possibly happen that SEEK_DATA would
> miss that single page generated by this test and how any of my patches
> would influence this particular situation. Thanks!

Yes, it's ENXIO, strace log:

...
open("/mnt/xfs/testfile07", O_RDWR|O_CREAT|O_TRUNC, 0644) = 3
write(1, "07. Test file with unwritten ext"..., 6007. Test file with unwritten extents, only have dirty pages
) = 60
fallocate(3, 0, 0, 11264)               = 0
pwrite(3, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 1024, 10240) = 1024
lseek(3, 0, SEEK_HOLE)                  = 0
write(1, "07.01 SEEK_HOLE expected 0 or 11"..., 7107.01 SEEK_HOLE expected 0 or 11264, got 0.                       succ
) = 71
lseek(3, 1, SEEK_HOLE)                  = 1
write(1, "07.02 SEEK_HOLE expected 1 or 11"..., 7107.02 SEEK_HOLE expected 1 or 11264, got 1.                       succ
) = 71
lseek(3, 0, SEEK_DATA)                  = -1 ENXIO (No such device or address)
write(1, "07.03 SEEK_DATA expected 10240 o"..., 7107.03 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
) = 71
lseek(3, 1, SEEK_DATA)                  = -1 ENXIO (No such device or address)
write(1, "07.04 SEEK_DATA expected 10240 o"..., 7107.04 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
...

It's the second patch "xfs: Fix off-by-in in loop termination in
xfs_find_get_desired_pgoff()" introduced the issue for me. ENXIO was
returned from the if (nmap == 1) block in __xfs_seek_hole_data()

                /*
                 * We only received one extent out of the two requested. This
                 * means we've hit EOF and didn't find what we are looking for.
                 */
                if (nmap == 1) {
		....
                        /*
                         * If we were looking for data, it's nowhere to be found
                         */
                        ASSERT(whence == SEEK_DATA);
                        error = -ENXIO;
                        goto out_error;
                }

Seems that's because the do {} while() loop in xfs_find_get_desired_pgoff() was
broken out earlier due to patch 2.

                        /* Searching done if the page index is out of range. */
                        if (page->index >= end) {
                                goto out;
                        }

In my case, it returned earlier because page->index == end == 2.

I was testing with 1k block size xfs. xfs_info output:
meta-data=/dev/sdc2              isize=512    agcount=4, agsize=5242880 blks
         =                       sectsz=512   attr=2, projid32bit=1
         =                       crc=1        finobt=1 spinodes=0 rmapbt=0
         =                       reflink=0
data     =                       bsize=1024   blocks=20971520, imaxpct=25
         =                       sunit=0      swidth=0 blks
naming   =version 2              bsize=4096   ascii-ci=0 ftype=1
log      =internal               bsize=1024   blocks=10240, version=2
         =                       sectsz=512   sunit=0 blks, lazy-count=1
realtime =none                   extsz=4096   blocks=0, rtextents=0

Hope this helps, if you need more info please let me know.

Thanks,
Eryu

Re: [PATCH 0/3 v2] xfs: Fix SEEK_HOLE implementation

[Jan Kara]

On Thu 18-05-17 17:47:53, Eryu Guan wrote:
> On Thu, May 18, 2017 at 11:03:46AM +0200, Jan Kara wrote:
> > On Wed 17-05-17 16:57:46, Jan Kara wrote:
> > > On Wed 17-05-17 20:31:15, Eryu Guan wrote:
> > > > Hi Jan,
> > > > 
> > > > On Wed, May 17, 2017 at 02:10:43PM +0200, Jan Kara wrote:
> > > > > Hello,
> > > > > 
> > > > > this is the second revision of the patches to fix bugs in XFS's SEEK_HOLE
> > > > > implementation and cleanup the code a bit.
> > > > > 
> > > > > Changes since v1:
> > > > > * Fixed some more buggy cases
> > > > > * Simplified code a bit as suggested by Darrick
> > > > > * Fixed range check as spotted by Brian
> > > > 
> > > > I applied this patchset on top of 4.12-rc1 kernel to test your v4 test
> > > > case, your new test passed all my tests, but I found generic/285
> > > > regressed with sub-page block size XFS, 285.full showed that failure was
> > > > from subtest 7
> > > > 
> > > > 07. Test file with unwritten extents, only have dirty pages
> > > > 07.01 SEEK_HOLE expected 0 or 11264, got 0.                       succ
> > > > 07.02 SEEK_HOLE expected 1 or 11264, got 1.                       succ
> > > > 07.03 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
> > > > 07.04 SEEK_DATA expected 10240 or 10240, got -1.                  FAIL
> > > > 
> > > > And manual test showed subtest 8 failed too
> > > > 
> > > > # ./src/seek_sanity_test -s 8 -e 8 /mnt/xfs/testfile
> > > > File system magic#: 0x58465342
> > > > Allocation size: 4096
> > > > 
> > > > 08. Test file with unwritten extents, only have unwritten pages
> > > > 08.01 SEEK_HOLE expected 0 or 5632, got 0.                        succ
> > > > 08.02 SEEK_HOLE expected 1 or 5632, got 1.                        succ
> > > > 08.03 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
> > > > 08.04 SEEK_DATA expected 5120 or 5120, got -1.                    FAIL
> > > > 
> > > > Other subtests all passed with sub-page block size XFS.
> > > 
> > > Strange. It doesn't fail for me this way even with 1k blocksize. I'll
> > > investigate more tomorrow.
> > 
> > So I've been trying quite hard to reproduce the failure but I failed. Since
> > you are apparently getting some error out of lseek can you find out which
> > error it is (likely ENXIO but I'd like to confirm) and where it gets
> > generated? I don't see how it could possibly happen that SEEK_DATA would
> > miss that single page generated by this test and how any of my patches
> > would influence this particular situation. Thanks!

<snip>

> Seems that's because the do {} while() loop in xfs_find_get_desired_pgoff() was
> broken out earlier due to patch 2.
> 
>                         /* Searching done if the page index is out of range. */
>                         if (page->index >= end) {
>                                 goto out;
>                         }
> 
> In my case, it returned earlier because page->index == end == 2.

Ah! That's it. I'm not sure why you get so short unwritten extent but it's
certainly possible. I can now reproduce the issue with:

xfs_io -f -c "falloc 0 10k" -c "pwrite 9k 512" -c "seek -d 0" /mnt/file
wrote 512/512 bytes at offset 9216
512.000000 bytes, 1 ops; 0.0000 sec (6.975 MiB/sec and 14285.7143 ops/sec)
Whence	Result
DATA	EOF

on 1k blocksize filesystem. And the problem is indeed that in this case I
have screwed up the condition due to rounding. I'll fix the second patch in
both series for ext4 & xfs. Thanks for debugging this!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR