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

[PATCH 1/2] 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 a first page in the pagevec has larger
index than expected (and even if the condition was right, we would fail
to update the returned offset).

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 | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 35703a801372..df51c025adfe 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1077,13 +1077,12 @@ xfs_find_get_desired_pgoff(
 		}
 
 		/*
-		 * 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.
+		 * At least we found one page. If the current offset is smaller
+		 * than the first page offset, a hole was found.
 		 */
-		if (type == HOLE_OFF && lastoff == startoff &&
-		    lastoff < page_offset(pvec.pages[0])) {
+		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
 			found = true;
+			*offset = lastoff;
 			break;
 		}
 
-- 
2.12.0

[PATCH 1/2] 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 a first page in the pagevec has larger
index than expected (and even if the condition was right, we would fail
to update the returned offset).

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 | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 35703a801372..df51c025adfe 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1077,13 +1077,12 @@ xfs_find_get_desired_pgoff(
 		}
 
 		/*
-		 * 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.
+		 * At least we found one page. If the current offset is smaller
+		 * than the first page offset, a hole was found.
 		 */
-		if (type == HOLE_OFF && lastoff == startoff &&
-		    lastoff < page_offset(pvec.pages[0])) {
+		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
 			found = true;
+			*offset = lastoff;
 			break;
 		}
 
-- 
2.12.0

[PATCH 2/2] 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 | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index df51c025adfe..719923b99ba1 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
 				break;
 
 			ASSERT(type == HOLE_OFF);
-			if (lastoff == startoff || lastoff < endoff) {
-				found = true;
-				*offset = lastoff;
-			}
 			break;
 		}
 
@@ -1080,11 +1076,8 @@ xfs_find_get_desired_pgoff(
 		 * At least we found one page. If the current offset is smaller
 		 * than the first page offset, a hole was found.
 		 */
-		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
-			found = true;
-			*offset = lastoff;
+		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0]))
 			break;
-		}
 
 		for (i = 0; i < nr_pages; i++) {
 			struct page	*page = pvec.pages[i];
@@ -1150,21 +1143,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 2/2] xfs: Move handling of missing page into one place in xfs_find_get_desired_pgoff()

[Darrick J. Wong]

On Thu, May 11, 2017 at 06:50:23PM +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>
> ---
>  fs/xfs/xfs_file.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index df51c025adfe..719923b99ba1 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
>  				break;
>  
>  			ASSERT(type == HOLE_OFF);
> -			if (lastoff == startoff || lastoff < endoff) {
> -				found = true;
> -				*offset = lastoff;
> -			}
>  			break;
>  		}

Hm.  This leaves the following weird looking hunk:

if (nr_pages == 0) {
	/* Data search found nothing */
	if (type == DATA_OFF)
		break;

	ASSERT(type == HOLE_OFF);
	break;
}

Which could be simplified to:

if (nr_pages == 0) {
	ASSERT(type == HOLE_OFF || type == DATA_OFF);
	break;
}

Right?  Maybe a better cleanup would be to name the enum defining
{HOLE,DATA}_OFF and change the xfs_find_get_desired_pgoff prototype to
use that enum, and then we also get compiler type checking.

The rest of this looks ok I think.

--D

> @@ -1080,11 +1076,8 @@ xfs_find_get_desired_pgoff(
>  		 * At least we found one page. If the current offset is smaller
>  		 * than the first page offset, a hole was found.
>  		 */
> -		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
> -			found = true;
> -			*offset = lastoff;
> +		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0]))
>  			break;
> -		}
>  
>  		for (i = 0; i < nr_pages; i++) {
>  			struct page	*page = pvec.pages[i];
> @@ -1150,21 +1143,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 1/2] xfs: Fix missed holes in SEEK_HOLE implementation

[Darrick J. Wong]

On Thu, May 11, 2017 at 06:50:22PM +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 a first page in the pagevec has larger
> index than expected (and even if the condition was right, we would fail
> to update the returned offset).
> 
> 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>

Looks ok, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_file.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 35703a801372..df51c025adfe 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1077,13 +1077,12 @@ xfs_find_get_desired_pgoff(
>  		}
>  
>  		/*
> -		 * 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.
> +		 * At least we found one page. If the current offset is smaller
> +		 * than the first page offset, a hole was found.
>  		 */
> -		if (type == HOLE_OFF && lastoff == startoff &&
> -		    lastoff < page_offset(pvec.pages[0])) {
> +		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
>  			found = true;
> +			*offset = lastoff;
>  			break;
>  		}
>  
> -- 
> 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/2] xfs: Move handling of missing page into one place in xfs_find_get_desired_pgoff()

[Brian Foster]

On Thu, May 11, 2017 at 06:50:23PM +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>
> ---
>  fs/xfs/xfs_file.c | 24 ++++++++----------------
>  1 file changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index df51c025adfe..719923b99ba1 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
>  				break;
>  
>  			ASSERT(type == HOLE_OFF);
> -			if (lastoff == startoff || lastoff < endoff) {
> -				found = true;
> -				*offset = lastoff;
> -			}
>  			break;
>  		}
>  
> @@ -1080,11 +1076,8 @@ xfs_find_get_desired_pgoff(
>  		 * At least we found one page. If the current offset is smaller
>  		 * than the first page offset, a hole was found.
>  		 */
> -		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
> -			found = true;
> -			*offset = lastoff;
> +		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0]))
>  			break;
> -		}
>  
>  		for (i = 0; i < nr_pages; i++) {
>  			struct page	*page = pvec.pages[i];

FWIW, it looks like there's still a bug here that the first patch
doesn't address:

# xfs_io -fc "truncate 0" -c "falloc 0 32k" -c "pwrite 0 4k" -c "pwrite 32k 4k" -c "seek -h 0" /mnt/file
wrote 4096/4096 bytes at offset 0
4 KiB, 1 ops; 0.0000 sec (126.008 MiB/sec and 32258.0645 ops/sec)
wrote 4096/4096 bytes at offset 32768
4 KiB, 1 ops; 0.0000 sec (102.796 MiB/sec and 26315.7895 ops/sec)
Whence  Result
HOLE    36864
# xfs_io -c fsync -c "seek -h 0" /mnt/file 
Whence  Result
HOLE    4096

I initially thought this patch might fix it, but on further digging it
doesn't appear to. What looks like is going on here is that the
'page->index > end' check doesn't actually catch this case of the page
being just beyond the current unwritten extent (perhaps the check should
be 'page->index >= end'), so lastoff is set beyond end and we skip the
hole.

Brian

> @@ -1150,21 +1143,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 2/2] xfs: Move handling of missing page into one place in xfs_find_get_desired_pgoff()

[Jan Kara]

On Mon 15-05-17 09:28:37, Brian Foster wrote:
> On Thu, May 11, 2017 at 06:50:23PM +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>
> > ---
> >  fs/xfs/xfs_file.c | 24 ++++++++----------------
> >  1 file changed, 8 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index df51c025adfe..719923b99ba1 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
> >  				break;
> >  
> >  			ASSERT(type == HOLE_OFF);
> > -			if (lastoff == startoff || lastoff < endoff) {
> > -				found = true;
> > -				*offset = lastoff;
> > -			}
> >  			break;
> >  		}
> >  
> > @@ -1080,11 +1076,8 @@ xfs_find_get_desired_pgoff(
> >  		 * At least we found one page. If the current offset is smaller
> >  		 * than the first page offset, a hole was found.
> >  		 */
> > -		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
> > -			found = true;
> > -			*offset = lastoff;
> > +		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0]))
> >  			break;
> > -		}
> >  
> >  		for (i = 0; i < nr_pages; i++) {
> >  			struct page	*page = pvec.pages[i];
> 
> FWIW, it looks like there's still a bug here that the first patch
> doesn't address:
> 
> # xfs_io -fc "truncate 0" -c "falloc 0 32k" -c "pwrite 0 4k" -c "pwrite 32k 4k" -c "seek -h 0" /mnt/file
> wrote 4096/4096 bytes at offset 0
> 4 KiB, 1 ops; 0.0000 sec (126.008 MiB/sec and 32258.0645 ops/sec)
> wrote 4096/4096 bytes at offset 32768
> 4 KiB, 1 ops; 0.0000 sec (102.796 MiB/sec and 26315.7895 ops/sec)
> Whence  Result
> HOLE    36864
> # xfs_io -c fsync -c "seek -h 0" /mnt/file 
> Whence  Result
> HOLE    4096
> 
> I initially thought this patch might fix it, but on further digging it
> doesn't appear to. What looks like is going on here is that the
> 'page->index > end' check doesn't actually catch this case of the page
> being just beyond the current unwritten extent (perhaps the check should
> be 'page->index >= end'), so lastoff is set beyond end and we skip the
> hole.

Right, that looks like another bug in the implementation. Will you send a
fix or should I?

								Honza


> 
> Brian
> 
> > @@ -1150,21 +1143,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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Brian Foster]

On Mon, May 15, 2017 at 04:44:21PM +0200, Jan Kara wrote:
> On Mon 15-05-17 09:28:37, Brian Foster wrote:
> > On Thu, May 11, 2017 at 06:50:23PM +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>
> > > ---
> > >  fs/xfs/xfs_file.c | 24 ++++++++----------------
> > >  1 file changed, 8 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > index df51c025adfe..719923b99ba1 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
> > >  				break;
> > >  
> > >  			ASSERT(type == HOLE_OFF);
> > > -			if (lastoff == startoff || lastoff < endoff) {
> > > -				found = true;
> > > -				*offset = lastoff;
> > > -			}
> > >  			break;
> > >  		}
> > >  
> > > @@ -1080,11 +1076,8 @@ xfs_find_get_desired_pgoff(
> > >  		 * At least we found one page. If the current offset is smaller
> > >  		 * than the first page offset, a hole was found.
> > >  		 */
> > > -		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
> > > -			found = true;
> > > -			*offset = lastoff;
> > > +		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0]))
> > >  			break;
> > > -		}
> > >  
> > >  		for (i = 0; i < nr_pages; i++) {
> > >  			struct page	*page = pvec.pages[i];
> > 
> > FWIW, it looks like there's still a bug here that the first patch
> > doesn't address:
> > 
> > # xfs_io -fc "truncate 0" -c "falloc 0 32k" -c "pwrite 0 4k" -c "pwrite 32k 4k" -c "seek -h 0" /mnt/file
> > wrote 4096/4096 bytes at offset 0
> > 4 KiB, 1 ops; 0.0000 sec (126.008 MiB/sec and 32258.0645 ops/sec)
> > wrote 4096/4096 bytes at offset 32768
> > 4 KiB, 1 ops; 0.0000 sec (102.796 MiB/sec and 26315.7895 ops/sec)
> > Whence  Result
> > HOLE    36864
> > # xfs_io -c fsync -c "seek -h 0" /mnt/file 
> > Whence  Result
> > HOLE    4096
> > 
> > I initially thought this patch might fix it, but on further digging it
> > doesn't appear to. What looks like is going on here is that the
> > 'page->index > end' check doesn't actually catch this case of the page
> > being just beyond the current unwritten extent (perhaps the check should
> > be 'page->index >= end'), so lastoff is set beyond end and we skip the
> > hole.
> 
> Right, that looks like another bug in the implementation. Will you send a
> fix or should I?
> 

It'd be great if you could tack it onto this series..? Otherwise I'll
send one a bit later..

Brian

> 								Honza
> 
> 
> > 
> > Brian
> > 
> > > @@ -1150,21 +1143,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
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

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

[Jan Kara]

On Mon 15-05-17 09:28:37, Brian Foster wrote:
> On Thu, May 11, 2017 at 06:50:23PM +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>
> > ---
> >  fs/xfs/xfs_file.c | 24 ++++++++----------------
> >  1 file changed, 8 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index df51c025adfe..719923b99ba1 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
> >  				break;
> >  
> >  			ASSERT(type == HOLE_OFF);
> > -			if (lastoff == startoff || lastoff < endoff) {
> > -				found = true;
> > -				*offset = lastoff;
> > -			}
> >  			break;
> >  		}
> >  
> > @@ -1080,11 +1076,8 @@ xfs_find_get_desired_pgoff(
> >  		 * At least we found one page. If the current offset is smaller
> >  		 * than the first page offset, a hole was found.
> >  		 */
> > -		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
> > -			found = true;
> > -			*offset = lastoff;
> > +		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0]))
> >  			break;
> > -		}
> >  
> >  		for (i = 0; i < nr_pages; i++) {
> >  			struct page	*page = pvec.pages[i];
> 
> FWIW, it looks like there's still a bug here that the first patch
> doesn't address:
> 
> # xfs_io -fc "truncate 0" -c "falloc 0 32k" -c "pwrite 0 4k" -c "pwrite 32k 4k" -c "seek -h 0" /mnt/file
> wrote 4096/4096 bytes at offset 0
> 4 KiB, 1 ops; 0.0000 sec (126.008 MiB/sec and 32258.0645 ops/sec)
> wrote 4096/4096 bytes at offset 32768
> 4 KiB, 1 ops; 0.0000 sec (102.796 MiB/sec and 26315.7895 ops/sec)
> Whence  Result
> HOLE    36864
> # xfs_io -c fsync -c "seek -h 0" /mnt/file 
> Whence  Result
> HOLE    4096
> 
> I initially thought this patch might fix it, but on further digging it
> doesn't appear to. What looks like is going on here is that the
> 'page->index > end' check doesn't actually catch this case of the page
> being just beyond the current unwritten extent (perhaps the check should
> be 'page->index >= end'), so lastoff is set beyond end and we skip the
> hole.

I've added a test for this in the new SEEK_HOLE/SEEK_DATA tests I was just
sending to Eryu.

								Honza

> > @@ -1150,21 +1143,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
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Mon 15-05-17 10:55:42, Brian Foster wrote:
> On Mon, May 15, 2017 at 04:44:21PM +0200, Jan Kara wrote:
> > On Mon 15-05-17 09:28:37, Brian Foster wrote:
> > > On Thu, May 11, 2017 at 06:50:23PM +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>
> > > > ---
> > > >  fs/xfs/xfs_file.c | 24 ++++++++----------------
> > > >  1 file changed, 8 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > index df51c025adfe..719923b99ba1 100644
> > > > --- a/fs/xfs/xfs_file.c
> > > > +++ b/fs/xfs/xfs_file.c
> > > > @@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
> > > >  				break;
> > > >  
> > > >  			ASSERT(type == HOLE_OFF);
> > > > -			if (lastoff == startoff || lastoff < endoff) {
> > > > -				found = true;
> > > > -				*offset = lastoff;
> > > > -			}
> > > >  			break;
> > > >  		}
> > > >  
> > > > @@ -1080,11 +1076,8 @@ xfs_find_get_desired_pgoff(
> > > >  		 * At least we found one page. If the current offset is smaller
> > > >  		 * than the first page offset, a hole was found.
> > > >  		 */
> > > > -		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0])) {
> > > > -			found = true;
> > > > -			*offset = lastoff;
> > > > +		if (type == HOLE_OFF && lastoff < page_offset(pvec.pages[0]))
> > > >  			break;
> > > > -		}
> > > >  
> > > >  		for (i = 0; i < nr_pages; i++) {
> > > >  			struct page	*page = pvec.pages[i];
> > > 
> > > FWIW, it looks like there's still a bug here that the first patch
> > > doesn't address:
> > > 
> > > # xfs_io -fc "truncate 0" -c "falloc 0 32k" -c "pwrite 0 4k" -c "pwrite 32k 4k" -c "seek -h 0" /mnt/file
> > > wrote 4096/4096 bytes at offset 0
> > > 4 KiB, 1 ops; 0.0000 sec (126.008 MiB/sec and 32258.0645 ops/sec)
> > > wrote 4096/4096 bytes at offset 32768
> > > 4 KiB, 1 ops; 0.0000 sec (102.796 MiB/sec and 26315.7895 ops/sec)
> > > Whence  Result
> > > HOLE    36864
> > > # xfs_io -c fsync -c "seek -h 0" /mnt/file 
> > > Whence  Result
> > > HOLE    4096
> > > 
> > > I initially thought this patch might fix it, but on further digging it
> > > doesn't appear to. What looks like is going on here is that the
> > > 'page->index > end' check doesn't actually catch this case of the page
> > > being just beyond the current unwritten extent (perhaps the check should
> > > be 'page->index >= end'), so lastoff is set beyond end and we skip the
> > > hole.
> > 
> > Right, that looks like another bug in the implementation. Will you send a
> > fix or should I?
> > 
> 
> It'd be great if you could tack it onto this series..? Otherwise I'll
> send one a bit later..

OK, I'll take care of this. Looking at it now, the function seems to have
even more problems. Like completely fails to verify found indices are
contiguous...

								Honza

> > > > @@ -1150,21 +1143,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
> > -- 
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Jan Kara]

On Fri 12-05-17 09:23:02, Darrick J. Wong wrote:
> On Thu, May 11, 2017 at 06:50:23PM +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>
> > ---
> >  fs/xfs/xfs_file.c | 24 ++++++++----------------
> >  1 file changed, 8 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index df51c025adfe..719923b99ba1 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1069,10 +1069,6 @@ xfs_find_get_desired_pgoff(
> >  				break;
> >  
> >  			ASSERT(type == HOLE_OFF);
> > -			if (lastoff == startoff || lastoff < endoff) {
> > -				found = true;
> > -				*offset = lastoff;
> > -			}
> >  			break;
> >  		}
> 
> Hm.  This leaves the following weird looking hunk:
> 
> if (nr_pages == 0) {
> 	/* Data search found nothing */
> 	if (type == DATA_OFF)
> 		break;
> 
> 	ASSERT(type == HOLE_OFF);
> 	break;
> }
> 
> Which could be simplified to:
> 
> if (nr_pages == 0) {
> 	ASSERT(type == HOLE_OFF || type == DATA_OFF);
> 	break;
> }
> 
> Right?  Maybe a better cleanup would be to name the enum defining
> {HOLE,DATA}_OFF and change the xfs_find_get_desired_pgoff prototype to
> use that enum, and then we also get compiler type checking.

Well, given this function is called only from one place, enum looks like an
overkill and even the assert is weird. I've just simplified the condition
to:

if (nr_pages == 0)
	break;

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