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

[PATCH 0/3 v3] 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 v2:
* Fixed rounding error in patch 2 spotted by Eryu

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

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

[Brian Foster]

On Thu, May 18, 2017 at 12:48:48PM +0200, Jan Kara wrote:
> 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>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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() since 'end' may index a page beyond end of
desired range if 'endoff' is page aligned. 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index f371812e20c6..3714b5736fd3 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
 
 	index = startoff >> PAGE_SHIFT;
 	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
-	end = endoff >> PAGE_SHIFT;
+	end = (endoff - 1) >> PAGE_SHIFT;
 	do {
 		int		want;
 		unsigned	nr_pages;
-- 
2.12.0

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

[Brian Foster]

On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> There is an off-by-one error in loop termination conditions in
> xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> desired range if 'endoff' is page aligned. 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index f371812e20c6..3714b5736fd3 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
>  
>  	index = startoff >> PAGE_SHIFT;
>  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> -	end = endoff >> PAGE_SHIFT;
> +	end = (endoff - 1) >> PAGE_SHIFT;

Hmm.. I think this messes with the want count for the pagevec_lookup().
E.g.:

# xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
wrote 16384/16384 bytes at offset 0
16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
Whence  Result
HOLE    12288

Brian

>  	do {
>  		int		want;
>  		unsigned	nr_pages;
> -- 
> 2.12.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eryu Guan]

On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote:
> On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> > There is an off-by-one error in loop termination conditions in
> > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> > desired range if 'endoff' is page aligned. 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 | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index f371812e20c6..3714b5736fd3 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
> >  
> >  	index = startoff >> PAGE_SHIFT;
> >  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> > -	end = endoff >> PAGE_SHIFT;
> > +	end = (endoff - 1) >> PAGE_SHIFT;
> 
> Hmm.. I think this messes with the want count for the pagevec_lookup().
> E.g.:
> 
> # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
> wrote 16384/16384 bytes at offset 0
> 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
> Whence  Result
> HOLE    12288

I think the root cause is that the calculation for 'want' is wrong, it
has an off-by-one bug too. I sent a patch[1] to fix it, with my patch
applied on top of Jan's patchset, your test case passed (report HOLE at
16k). Can you please take a look if it's a correct fix? Thanks!

Eryu

[1] [PATCH] xfs: fix off-by-one on max nr_pages in xfs_find_get_desired_pgoff()
    https://www.spinics.net/lists/linux-xfs/msg06980.html
> 
> Brian
> 
> >  	do {
> >  		int		want;
> >  		unsigned	nr_pages;
> > -- 
> > 2.12.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jan Kara]

On Tue 23-05-17 11:21:23, Eryu Guan wrote:
> On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote:
> > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> > > There is an off-by-one error in loop termination conditions in
> > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> > > desired range if 'endoff' is page aligned. 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 | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > index f371812e20c6..3714b5736fd3 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
> > >  
> > >  	index = startoff >> PAGE_SHIFT;
> > >  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> > > -	end = endoff >> PAGE_SHIFT;
> > > +	end = (endoff - 1) >> PAGE_SHIFT;
> > 
> > Hmm.. I think this messes with the want count for the pagevec_lookup().
> > E.g.:
> > 
> > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
> > wrote 16384/16384 bytes at offset 0
> > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
> > Whence  Result
> > HOLE    12288
> 
> I think the root cause is that the calculation for 'want' is wrong, it
> has an off-by-one bug too. I sent a patch[1] to fix it, with my patch
> applied on top of Jan's patchset, your test case passed (report HOLE at
> 16k). Can you please take a look if it's a correct fix? Thanks!

Yes, I've messed that up. It is a bug introduced by my series as Brian
properly noticed. Thanks guys for noticing and fixing it! Darrick, should I
fold in Eryu's fix and send v4 of the series or will you just pick up
Eryu's fix?

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

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

[Eryu Guan]

On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote:
> On Tue 23-05-17 11:21:23, Eryu Guan wrote:
> > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote:
> > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> > > > There is an off-by-one error in loop termination conditions in
> > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> > > > desired range if 'endoff' is page aligned. 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 | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > index f371812e20c6..3714b5736fd3 100644
> > > > --- a/fs/xfs/xfs_file.c
> > > > +++ b/fs/xfs/xfs_file.c
> > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
> > > >  
> > > >  	index = startoff >> PAGE_SHIFT;
> > > >  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> > > > -	end = endoff >> PAGE_SHIFT;
> > > > +	end = (endoff - 1) >> PAGE_SHIFT;
> > > 
> > > Hmm.. I think this messes with the want count for the pagevec_lookup().
> > > E.g.:
> > > 
> > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
> > > wrote 16384/16384 bytes at offset 0
> > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
> > > Whence  Result
> > > HOLE    12288
> > 
> > I think the root cause is that the calculation for 'want' is wrong, it
> > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch
> > applied on top of Jan's patchset, your test case passed (report HOLE at
> > 16k). Can you please take a look if it's a correct fix? Thanks!
> 
> Yes, I've messed that up. It is a bug introduced by my series as Brian
> properly noticed. Thanks guys for noticing and fixing it! Darrick, should I
> fold in Eryu's fix and send v4 of the series or will you just pick up
> Eryu's fix?

I think it's a separate bug, the issue described in my patch can be
reproduced on stock 4.12-rc1 kernel, without your patchset. The
situation for ext4 is similar to XFS, it seems not a bug introduced by
your patches.

Thanks for the review!

Eryu

> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Brian Foster]

On Tue, May 23, 2017 at 07:08:32PM +0800, Eryu Guan wrote:
> On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote:
> > On Tue 23-05-17 11:21:23, Eryu Guan wrote:
> > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote:
> > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> > > > > There is an off-by-one error in loop termination conditions in
> > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> > > > > desired range if 'endoff' is page aligned. 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 | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > > index f371812e20c6..3714b5736fd3 100644
> > > > > --- a/fs/xfs/xfs_file.c
> > > > > +++ b/fs/xfs/xfs_file.c
> > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
> > > > >  
> > > > >  	index = startoff >> PAGE_SHIFT;
> > > > >  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> > > > > -	end = endoff >> PAGE_SHIFT;
> > > > > +	end = (endoff - 1) >> PAGE_SHIFT;
> > > > 
> > > > Hmm.. I think this messes with the want count for the pagevec_lookup().
> > > > E.g.:
> > > > 
> > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
> > > > wrote 16384/16384 bytes at offset 0
> > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
> > > > Whence  Result
> > > > HOLE    12288
> > > 
> > > I think the root cause is that the calculation for 'want' is wrong, it
> > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch
> > > applied on top of Jan's patchset, your test case passed (report HOLE at
> > > 16k). Can you please take a look if it's a correct fix? Thanks!
> > 
> > Yes, I've messed that up. It is a bug introduced by my series as Brian
> > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I
> > fold in Eryu's fix and send v4 of the series or will you just pick up
> > Eryu's fix?
> 
> I think it's a separate bug, the issue described in my patch can be
> reproduced on stock 4.12-rc1 kernel, without your patchset. The
> situation for ext4 is similar to XFS, it seems not a bug introduced by
> your patches.
> 
> Thanks for the review!
> 

I think there's the possiblity of multiple things going on here. The
problem noted above didn't occur without this series applied. Of course,
that doesn't mean that there isn't some other problem with the current
code that Eryu has reproduced and fixed.

In any event, I don't think we should knowingly apply patches with
regressions, even if they are fixed up in the same patch series. Could
we get this fixed up/combined somehow or another so we at least do not
introduce a transient regression (it doesn't matter to me if the
independent problem is addressed in a separate patch or at the same
time)?

Also, Eryu, could you Tested-by this series before it goes in? It seems
you have some tests that stress it quite thoroughly. :)

Brian

> Eryu
> 
> > 
> > 								Honza
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" 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 linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eryu Guan]

On Tue, May 23, 2017 at 08:17:20AM -0400, Brian Foster wrote:
> On Tue, May 23, 2017 at 07:08:32PM +0800, Eryu Guan wrote:
> > On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote:
> > > On Tue 23-05-17 11:21:23, Eryu Guan wrote:
> > > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote:
> > > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> > > > > > There is an off-by-one error in loop termination conditions in
> > > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> > > > > > desired range if 'endoff' is page aligned. 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 | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > > > index f371812e20c6..3714b5736fd3 100644
> > > > > > --- a/fs/xfs/xfs_file.c
> > > > > > +++ b/fs/xfs/xfs_file.c
> > > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
> > > > > >  
> > > > > >  	index = startoff >> PAGE_SHIFT;
> > > > > >  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> > > > > > -	end = endoff >> PAGE_SHIFT;
> > > > > > +	end = (endoff - 1) >> PAGE_SHIFT;
> > > > > 
> > > > > Hmm.. I think this messes with the want count for the pagevec_lookup().
> > > > > E.g.:
> > > > > 
> > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
> > > > > wrote 16384/16384 bytes at offset 0
> > > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
> > > > > Whence  Result
> > > > > HOLE    12288
> > > > 
> > > > I think the root cause is that the calculation for 'want' is wrong, it
> > > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch
> > > > applied on top of Jan's patchset, your test case passed (report HOLE at
> > > > 16k). Can you please take a look if it's a correct fix? Thanks!
> > > 
> > > Yes, I've messed that up. It is a bug introduced by my series as Brian
> > > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I
> > > fold in Eryu's fix and send v4 of the series or will you just pick up
> > > Eryu's fix?
> > 
> > I think it's a separate bug, the issue described in my patch can be
> > reproduced on stock 4.12-rc1 kernel, without your patchset. The
> > situation for ext4 is similar to XFS, it seems not a bug introduced by
> > your patches.
> > 
> > Thanks for the review!
> > 
> 
> I think there's the possiblity of multiple things going on here. The
> problem noted above didn't occur without this series applied. Of course,
> that doesn't mean that there isn't some other problem with the current
> code that Eryu has reproduced and fixed.
> 
> In any event, I don't think we should knowingly apply patches with
> regressions, even if they are fixed up in the same patch series. Could
> we get this fixed up/combined somehow or another so we at least do not
> introduce a transient regression (it doesn't matter to me if the
> independent problem is addressed in a separate patch or at the same
> time)?
> 
> Also, Eryu, could you Tested-by this series before it goes in? It seems
> you have some tests that stress it quite thoroughly. :)

I have no tests more than generic/285 and generic/436, I just run them
with different block size xfs/ext4 and on different architectures
(x86_64 and ppc64) :) But sure, I'll give a Tested-by tag once we work
out which patches should go in.

Thanks,
Eryu

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

[Darrick J. Wong]

On Tue, May 23, 2017 at 09:05:34PM +0800, Eryu Guan wrote:
> On Tue, May 23, 2017 at 08:17:20AM -0400, Brian Foster wrote:
> > On Tue, May 23, 2017 at 07:08:32PM +0800, Eryu Guan wrote:
> > > On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote:
> > > > On Tue 23-05-17 11:21:23, Eryu Guan wrote:
> > > > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote:
> > > > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> > > > > > > There is an off-by-one error in loop termination conditions in
> > > > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> > > > > > > desired range if 'endoff' is page aligned. 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 | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > > > > index f371812e20c6..3714b5736fd3 100644
> > > > > > > --- a/fs/xfs/xfs_file.c
> > > > > > > +++ b/fs/xfs/xfs_file.c
> > > > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
> > > > > > >  
> > > > > > >  	index = startoff >> PAGE_SHIFT;
> > > > > > >  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> > > > > > > -	end = endoff >> PAGE_SHIFT;
> > > > > > > +	end = (endoff - 1) >> PAGE_SHIFT;
> > > > > > 
> > > > > > Hmm.. I think this messes with the want count for the pagevec_lookup().
> > > > > > E.g.:
> > > > > > 
> > > > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
> > > > > > wrote 16384/16384 bytes at offset 0
> > > > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
> > > > > > Whence  Result
> > > > > > HOLE    12288
> > > > > 
> > > > > I think the root cause is that the calculation for 'want' is wrong, it
> > > > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch
> > > > > applied on top of Jan's patchset, your test case passed (report HOLE at
> > > > > 16k). Can you please take a look if it's a correct fix? Thanks!
> > > > 
> > > > Yes, I've messed that up. It is a bug introduced by my series as Brian
> > > > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I
> > > > fold in Eryu's fix and send v4 of the series or will you just pick up
> > > > Eryu's fix?
> > > 
> > > I think it's a separate bug, the issue described in my patch can be
> > > reproduced on stock 4.12-rc1 kernel, without your patchset. The
> > > situation for ext4 is similar to XFS, it seems not a bug introduced by
> > > your patches.
> > > 
> > > Thanks for the review!
> > > 
> > 
> > I think there's the possiblity of multiple things going on here. The
> > problem noted above didn't occur without this series applied. Of course,
> > that doesn't mean that there isn't some other problem with the current
> > code that Eryu has reproduced and fixed.
> > 
> > In any event, I don't think we should knowingly apply patches with
> > regressions, even if they are fixed up in the same patch series. Could
> > we get this fixed up/combined somehow or another so we at least do not
> > introduce a transient regression (it doesn't matter to me if the
> > independent problem is addressed in a separate patch or at the same
> > time)?
> > 
> > Also, Eryu, could you Tested-by this series before it goes in? It seems
> > you have some tests that stress it quite thoroughly. :)
> 
> I have no tests more than generic/285 and generic/436, I just run them
> with different block size xfs/ext4 and on different architectures
> (x86_64 and ppc64) :) But sure, I'll give a Tested-by tag once we work
> out which patches should go in.

FWIW I added Eryu's patch and then all of Jan's to my 4.12-fixes branch:
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-4.12-fixes

(Have a look to make sure I didn't miss anything, but be warned that I
have no reservations against reworking a personal repo branch arbitrarily).

Then I ran all the tests I saw mentioned (g/285, g/436, 4k and 1k block
sizes) and didn't see any problems, so I'm now running all the other
tests.  If nothing blows up then that'll end up as the next push to
Linus around the end of the week.

--D

> 
> Thanks,
> Eryu
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darrick J. Wong]

On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote:
> On Tue 23-05-17 11:21:23, Eryu Guan wrote:
> > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote:
> > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> > > > There is an off-by-one error in loop termination conditions in
> > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> > > > desired range if 'endoff' is page aligned. 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 | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > index f371812e20c6..3714b5736fd3 100644
> > > > --- a/fs/xfs/xfs_file.c
> > > > +++ b/fs/xfs/xfs_file.c
> > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
> > > >  
> > > >  	index = startoff >> PAGE_SHIFT;
> > > >  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> > > > -	end = endoff >> PAGE_SHIFT;
> > > > +	end = (endoff - 1) >> PAGE_SHIFT;
> > > 
> > > Hmm.. I think this messes with the want count for the pagevec_lookup().
> > > E.g.:
> > > 
> > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
> > > wrote 16384/16384 bytes at offset 0
> > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
> > > Whence  Result
> > > HOLE    12288
> > 
> > I think the root cause is that the calculation for 'want' is wrong, it
> > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch
> > applied on top of Jan's patchset, your test case passed (report HOLE at
> > 16k). Can you please take a look if it's a correct fix? Thanks!
> 
> Yes, I've messed that up. It is a bug introduced by my series as Brian
> properly noticed. Thanks guys for noticing and fixing it! Darrick, should I
> fold in Eryu's fix and send v4 of the series or will you just pick up
> Eryu's fix?

FWIW it looked like a separate problem to me as well.  It appears to me
that I could merge Eryu's patch immediately prior to Jan's series; is
that ok with everyone?

--D

> 
> 								Honza
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Brian Foster]

On Tue, May 23, 2017 at 08:30:18AM -0700, Darrick J. Wong wrote:
> On Tue, May 23, 2017 at 10:50:44AM +0200, Jan Kara wrote:
> > On Tue 23-05-17 11:21:23, Eryu Guan wrote:
> > > On Mon, May 22, 2017 at 01:50:47PM -0400, Brian Foster wrote:
> > > > On Thu, May 18, 2017 at 12:48:49PM +0200, Jan Kara wrote:
> > > > > There is an off-by-one error in loop termination conditions in
> > > > > xfs_find_get_desired_pgoff() since 'end' may index a page beyond end of
> > > > > desired range if 'endoff' is page aligned. 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 | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > > index f371812e20c6..3714b5736fd3 100644
> > > > > --- a/fs/xfs/xfs_file.c
> > > > > +++ b/fs/xfs/xfs_file.c
> > > > > @@ -1043,7 +1043,7 @@ xfs_find_get_desired_pgoff(
> > > > >  
> > > > >  	index = startoff >> PAGE_SHIFT;
> > > > >  	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
> > > > > -	end = endoff >> PAGE_SHIFT;
> > > > > +	end = (endoff - 1) >> PAGE_SHIFT;
> > > > 
> > > > Hmm.. I think this messes with the want count for the pagevec_lookup().
> > > > E.g.:
> > > > 
> > > > # xfs_io -fc "truncate 0" -c "falloc 0 16k" -c "pwrite 0 16k" -c "seek -h 0" /mnt/file 
> > > > wrote 16384/16384 bytes at offset 0
> > > > 16 KiB, 4 ops; 0.0000 sec (200.321 MiB/sec and 51282.0513 ops/sec)
> > > > Whence  Result
> > > > HOLE    12288
> > > 
> > > I think the root cause is that the calculation for 'want' is wrong, it
> > > has an off-by-one bug too. I sent a patch[1] to fix it, with my patch
> > > applied on top of Jan's patchset, your test case passed (report HOLE at
> > > 16k). Can you please take a look if it's a correct fix? Thanks!
> > 
> > Yes, I've messed that up. It is a bug introduced by my series as Brian
> > properly noticed. Thanks guys for noticing and fixing it! Darrick, should I
> > fold in Eryu's fix and send v4 of the series or will you just pick up
> > Eryu's fix?
> 
> FWIW it looked like a separate problem to me as well.  It appears to me
> that I could merge Eryu's patch immediately prior to Jan's series; is
> that ok with everyone?
> 

Fine by me if that effectively hides the regression..

Brian

> --D
> 
> > 
> > 								Honza
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 3714b5736fd3..204e81abce05 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 3/3] xfs: Move handling of missing page into one place in xfs_find_get_desired_pgoff()

[Brian Foster]

On Thu, May 18, 2017 at 12:48:50PM +0200, Jan Kara wrote:
> 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>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  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 3714b5736fd3..204e81abce05 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Darrick J. Wong]

On Thu, May 18, 2017 at 12:48:47PM +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.

Looks ok, will give it a test; for now,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
> Changes since v2:
> * Fixed rounding error in patch 2 spotted by Eryu
> 
> Changes since v1:
> * Fixed some more buggy cases
> * Simplified code a bit as suggested by Darrick
> * Fixed range check as spotted by Brian
> 
> 								Honza
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 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