[PATCH 0/2] xfs: don't fragment files with ZERO_RANGE calls

[PATCH 0/2] xfs: don't fragment files with ZERO_RANGE calls

[Eric Sandeen]

the ZERO_RANGE fallocate mode currently punches out
the requested range, then reallocates it as unwritten extents.

This is fine, but the re-allocation fragments the file when we could
be converting in place:

$ rm -rf moo ; fallocate -l 16m moo ; xfs_io -c "bmap -vp" -c 'fzero 1m 64k' -c "bmap -vp"  moo
moo:
 EXT: FILE-OFFSET      BLOCK-RANGE          AG AG-OFFSET            TOTAL FLAGS
   0: [0..32767]:      187007640..187040407  2 (52789912..52822679) 32768 10000
moo:
 EXT: FILE-OFFSET      BLOCK-RANGE          AG AG-OFFSET            TOTAL FLAGS
   0: [0..2047]:       187007640..187009687  2 (52789912..52791959)  2048 10000
   1: [2048..2175]:    187040408..187040535  2 (52822680..52822807)   128 10000
   2: [2176..32767]:   187009816..187040407  2 (52792088..52822679) 30592 10000


$ rm -rf moo ; xfs_io -f -c "pwrite 0 16m" -c "bmap -vp"  -c 'fzero 1m 64k' -c "bmap -vp" moo
wrote 16777216/16777216 bytes at offset 0
16 MiB, 4096 ops; 0.0000 sec (124.504 MiB/sec and 31873.0060 ops/sec)
moo:
 EXT: FILE-OFFSET      BLOCK-RANGE          AG AG-OFFSET            TOTAL FLAGS
   0: [0..32767]:      187605400..187638167  2 (53387672..53420439) 32768 00000
moo:
 EXT: FILE-OFFSET      BLOCK-RANGE          AG AG-OFFSET            TOTAL FLAGS
   0: [0..2047]:       187605400..187607447  2 (53387672..53389719)  2048 00000
   1: [2048..2175]:    187670808..187670935  2 (53453080..53453207)   128 10000
   2: [2176..65407]:   187607576..187670807  2 (53389848..53453079) 63232 00000

We did it this way in part to utilize xfs_free_file_space's partial block 
zeroing capabilities, so factor that out, then use it + xfs_alloc_file_space
to convert in place.

[PATCH 1/2] xfs: factor range zeroing out of xfs_free_file_space

[Eric Sandeen]

Factor out the range zeroing part of xfs_free_file_space into
a new function, xfs_zero_range, which is a wrapper around
iomap_zero_range + EOF handling.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 2a7e10f26bb0..0a96c4d1718e 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1061,6 +1061,37 @@ xfs_flush_unmap_range(
 	return 0;
 }
 
+/*
+ * Ensure that the specified range is all zeros.
+ * Unmapped or unwritten blocks are left alone by the iomap mechanism,
+ * as they already return zero on read.
+ * This will happily extend i_size if asked to do so.
+ */
+static int
+xfs_zero_range(
+	struct xfs_inode	*ip,
+	xfs_off_t		offset,
+	xfs_off_t		len)
+{
+	int			error;
+
+	error = iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
+	if (error)
+		return error;
+	/*
+	 * If we zeroed right up to EOF and EOF straddles a page boundary we
+	 * must make sure that the post-EOF area is also zeroed because the
+	 * page could be mmap'd and iomap_zero_range doesn't do that for us.
+	 * Writeback of the eof page will do this, albeit clumsily.
+	 */
+	if (offset + len >= XFS_ISIZE(ip) && offset_in_page(offset + len) > 0) {
+		error = filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
+				round_down(offset + len, PAGE_SIZE), LLONG_MAX);
+	}
+
+	return error;
+}
+
 int
 xfs_free_file_space(
 	struct xfs_inode	*ip,
@@ -1101,28 +1132,15 @@ xfs_free_file_space(
 	}
 
 	/*
-	 * Now that we've unmap all full blocks we'll have to zero out any
-	 * partial block at the beginning and/or end.  iomap_zero_range is smart
-	 * enough to skip any holes, including those we just created, but we
-	 * must take care not to zero beyond EOF and enlarge i_size.
-	 */
-	if (offset >= XFS_ISIZE(ip))
-		return 0;
-	if (offset + len > XFS_ISIZE(ip))
-		len = XFS_ISIZE(ip) - offset;
-	error = iomap_zero_range(VFS_I(ip), offset, len, NULL, &xfs_iomap_ops);
-	if (error)
-		return error;
-
-	/*
-	 * If we zeroed right up to EOF and EOF straddles a page boundary we
-	 * must make sure that the post-EOF area is also zeroed because the
-	 * page could be mmap'd and iomap_zero_range doesn't do that for us.
-	 * Writeback of the eof page will do this, albeit clumsily.
+	 * Now that we've unmapped all full blocks we'll have to zero out any
+	 * partial block at the beginning and/or end.  xfs_zero_range is smart
+	 * enough to skip any holes, including those we just created.  We don't
+	 * want to zero beyond EOF and enlarge i_size.
 	 */
-	if (offset + len >= XFS_ISIZE(ip) && offset_in_page(offset + len) > 0) {
-		error = filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
-				round_down(offset + len, PAGE_SIZE), LLONG_MAX);
+	if (offset < XFS_ISIZE(ip)) {
+		if (offset + len > XFS_ISIZE(ip))
+			len = XFS_ISIZE(ip) - offset;
+		error = xfs_zero_range(ip, offset, len);
 	}
 
 	return error;

[PATCH 2/2] xfs: convert extents in place for ZERO_RANGE

[Eric Sandeen]

Rather than completely removing and re-allocating a range
during ZERO_RANGE fallocate calls, convert whole blocks in the
range using xfs_alloc_file_space(XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT)
and then zero the edges with xfs_zero_range()

(Note that this changes the rounding direction of the
xfs_alloc_file_space range, because we only want to hit whole
blocks within the range.)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

<currently running fsx ad infinitum, so far so good>

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 0a96c4d1718e..eae202bfe134 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -1164,23 +1164,25 @@ xfs_zero_file_space(
 
 	blksize = 1 << mp->m_sb.sb_blocklog;
 
+	error = xfs_flush_unmap_range(ip, offset, len);
+	if (error)
+		return error;
 	/*
-	 * Punch a hole and prealloc the range. We use hole punch rather than
-	 * unwritten extent conversion for two reasons:
-	 *
-	 * 1.) Hole punch handles partial block zeroing for us.
-	 *
-	 * 2.) If prealloc returns ENOSPC, the file range is still zero-valued
-	 * by virtue of the hole punch.
+	 * Convert whole blocks in the range to unwritten, then call iomap
+	 * via xfs_zero_range to zero the range.  iomap will skip holes and
+	 * unwritten extents, and just zero the edges if needed.  If conversion
+	 * fails, iomap will simply write zeros to the whole range.
+	 * nb: always_cow doesn't support unwritten extents.
 	 */
-	error = xfs_free_file_space(ip, offset, len);
-	if (error || xfs_is_always_cow_inode(ip))
-		return error;
+	if (!xfs_is_always_cow_inode(ip))
+		xfs_alloc_file_space(ip, round_up(offset, blksize),
+				     round_down(offset + len, blksize) -
+				     round_up(offset, blksize),
+				     XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT);
 
-	return xfs_alloc_file_space(ip, round_down(offset, blksize),
-				     round_up(offset + len, blksize) -
-				     round_down(offset, blksize),
-				     XFS_BMAPI_PREALLOC);
+	error = xfs_zero_range(ip, offset, len);
+
+	return error;
 }
 
 static int

Re: [PATCH 2/2] xfs: convert extents in place for ZERO_RANGE

[Dave Chinner]

On Mon, Jun 24, 2019 at 07:48:11PM -0500, Eric Sandeen wrote:
> Rather than completely removing and re-allocating a range
> during ZERO_RANGE fallocate calls, convert whole blocks in the
> range using xfs_alloc_file_space(XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT)
> and then zero the edges with xfs_zero_range()

That's what I originally used to implement ZERO_RANGE and that
had problems with zeroing the partial blocks either side and
unexpected inode size changes. See commit:

5d11fb4b9a1d xfs: rework zero range to prevent invalid i_size updates

I also remember discussion about zero range being inefficient on
sparse files and fragmented files - the current implementation
effectively defragments such files, whilst using XFS_BMAPI_CONVERT
just leaves all the fragments behind.

> (Note that this changes the rounding direction of the
> xfs_alloc_file_space range, because we only want to hit whole
> blocks within the range.)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> <currently running fsx ad infinitum, so far so good>
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 0a96c4d1718e..eae202bfe134 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1164,23 +1164,25 @@ xfs_zero_file_space(
>  
>  	blksize = 1 << mp->m_sb.sb_blocklog;
>  
> +	error = xfs_flush_unmap_range(ip, offset, len);
> +	if (error)
> +		return error;
>  	/*
> -	 * Punch a hole and prealloc the range. We use hole punch rather than
> -	 * unwritten extent conversion for two reasons:
> -	 *
> -	 * 1.) Hole punch handles partial block zeroing for us.
> -	 *
> -	 * 2.) If prealloc returns ENOSPC, the file range is still zero-valued
> -	 * by virtue of the hole punch.
> +	 * Convert whole blocks in the range to unwritten, then call iomap
> +	 * via xfs_zero_range to zero the range.  iomap will skip holes and
> +	 * unwritten extents, and just zero the edges if needed.  If conversion
> +	 * fails, iomap will simply write zeros to the whole range.
> +	 * nb: always_cow doesn't support unwritten extents.
>  	 */
> -	error = xfs_free_file_space(ip, offset, len);
> -	if (error || xfs_is_always_cow_inode(ip))
> -		return error;
> +	if (!xfs_is_always_cow_inode(ip))
> +		xfs_alloc_file_space(ip, round_up(offset, blksize),
> +				     round_down(offset + len, blksize) -
> +				     round_up(offset, blksize),
> +				     XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT);

If this fails with, say, corruption we should abort with an error,
not ignore it. I think we can only safely ignore ENOSPC and maybe
EDQUOT here...

> -	return xfs_alloc_file_space(ip, round_down(offset, blksize),
> -				     round_up(offset + len, blksize) -
> -				     round_down(offset, blksize),
> -				     XFS_BMAPI_PREALLOC);
> +	error = xfs_zero_range(ip, offset, len);

What prevents xfs_zero_range() from changing the file size if
offset + len is beyond EOF and there are allocated extents (from
delalloc conversion) beyond EOF? (i.e. FALLOC_FL_KEEP_SIZE is set by
the caller).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: convert extents in place for ZERO_RANGE

[Eric Sandeen]

On 6/24/19 9:39 PM, Dave Chinner wrote:
> On Mon, Jun 24, 2019 at 07:48:11PM -0500, Eric Sandeen wrote:
>> Rather than completely removing and re-allocating a range
>> during ZERO_RANGE fallocate calls, convert whole blocks in the
>> range using xfs_alloc_file_space(XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT)
>> and then zero the edges with xfs_zero_range()
> 
> That's what I originally used to implement ZERO_RANGE and that
> had problems with zeroing the partial blocks either side and
> unexpected inode size changes. See commit:
> 
> 5d11fb4b9a1d xfs: rework zero range to prevent invalid i_size updates

Yep I did see that.  It had a lot of hand-rolled partial block stuff
that seems more complex than this, no?  That commit didn't indicate
what the root cause of the failure actually was, AFAICT.

(funny thought that I skimmed that commit just to see why we had
what we have, but didn't really intentionally re-implement it...
even though I guess I almost did...)

> I also remember discussion about zero range being inefficient on
> sparse files and fragmented files - the current implementation
> effectively defragments such files, whilst using XFS_BMAPI_CONVERT
> just leaves all the fragments behind.

That's true - and it fragments unfragmented files.  Is ZERO_RANGE
supposed to be a defragmenter?

>> (Note that this changes the rounding direction of the
>> xfs_alloc_file_space range, because we only want to hit whole
>> blocks within the range.)
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> <currently running fsx ad infinitum, so far so good>

<still running, so far so good (4k blocks)>

>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 0a96c4d1718e..eae202bfe134 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -1164,23 +1164,25 @@ xfs_zero_file_space(
>>  
>>  	blksize = 1 << mp->m_sb.sb_blocklog;
>>  
>> +	error = xfs_flush_unmap_range(ip, offset, len);
>> +	if (error)
>> +		return error;
>>  	/*
>> -	 * Punch a hole and prealloc the range. We use hole punch rather than
>> -	 * unwritten extent conversion for two reasons:
>> -	 *
>> -	 * 1.) Hole punch handles partial block zeroing for us.
>> -	 *
>> -	 * 2.) If prealloc returns ENOSPC, the file range is still zero-valued
>> -	 * by virtue of the hole punch.
>> +	 * Convert whole blocks in the range to unwritten, then call iomap
>> +	 * via xfs_zero_range to zero the range.  iomap will skip holes and
>> +	 * unwritten extents, and just zero the edges if needed.  If conversion
>> +	 * fails, iomap will simply write zeros to the whole range.
>> +	 * nb: always_cow doesn't support unwritten extents.
>>  	 */
>> -	error = xfs_free_file_space(ip, offset, len);
>> -	if (error || xfs_is_always_cow_inode(ip))
>> -		return error;
>> +	if (!xfs_is_always_cow_inode(ip))
>> +		xfs_alloc_file_space(ip, round_up(offset, blksize),
>> +				     round_down(offset + len, blksize) -
>> +				     round_up(offset, blksize),
>> +				     XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT);
> 
> If this fails with, say, corruption we should abort with an error,
> not ignore it. I think we can only safely ignore ENOSPC and maybe
> EDQUOT here...

Yes, I suppose so, though if this encounters corruption I'd guess
xfs_zero_range probably would as well but that's just handwaving.

>> -	return xfs_alloc_file_space(ip, round_down(offset, blksize),
>> -				     round_up(offset + len, blksize) -
>> -				     round_down(offset, blksize),
>> -				     XFS_BMAPI_PREALLOC);
>> +	error = xfs_zero_range(ip, offset, len);
> 
> What prevents xfs_zero_range() from changing the file size if
> offset + len is beyond EOF and there are allocated extents (from
> delalloc conversion) beyond EOF? (i.e. FALLOC_FL_KEEP_SIZE is set by
> the caller).

nothing, but AFAIK it does the same today... even w/o extents past
EOF:

$ xfs_io -f -c "truncate 0" -c "fzero 0 1m" testfile

$ ls -lh testfile
-rw-------. 1 sandeen sandeen 1.0M Jun 24 21:48 testfile

$ xfs_bmap -vvp testfile
testfile:
 EXT: FILE-OFFSET      BLOCK-RANGE          AG AG-OFFSET            TOTAL FLAGS
   0: [0..2047]:       183206064..183208111  2 (48988336..48990383)  2048 10000
 FLAG Values:
    010000 Unwritten preallocated extent
    001000 Doesn't begin on stripe unit
    000100 Doesn't end   on stripe unit
    000010 Doesn't begin on stripe width
    000001 Doesn't end   on stripe width

At the end of the day it's just one allocation behavior over another,
it's not a correctness issue, so if there are concerns I don't have
to push it...

-Eric
 
> Cheers,
> 
> Dave.
> 

Re: [PATCH 2/2] xfs: convert extents in place for ZERO_RANGE

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 09:52:03PM -0500, Eric Sandeen wrote:
> On 6/24/19 9:39 PM, Dave Chinner wrote:
> > On Mon, Jun 24, 2019 at 07:48:11PM -0500, Eric Sandeen wrote:
> >> Rather than completely removing and re-allocating a range
> >> during ZERO_RANGE fallocate calls, convert whole blocks in the
> >> range using xfs_alloc_file_space(XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT)
> >> and then zero the edges with xfs_zero_range()
> > 
> > That's what I originally used to implement ZERO_RANGE and that
> > had problems with zeroing the partial blocks either side and
> > unexpected inode size changes. See commit:
> > 
> > 5d11fb4b9a1d xfs: rework zero range to prevent invalid i_size updates
> 
> Yep I did see that.  It had a lot of hand-rolled partial block stuff
> that seems more complex than this, no?  That commit didn't indicate
> what the root cause of the failure actually was, AFAICT.
> 
> (funny thought that I skimmed that commit just to see why we had
> what we have, but didn't really intentionally re-implement it...
> even though I guess I almost did...)

FWIW the complaint I had about the fragmentary behavior really only
applied to fun and games when one fallocated an ext4 image and then ran
mkfs.ext4 which uses zero range which fragmented the image...

> > I also remember discussion about zero range being inefficient on
> > sparse files and fragmented files - the current implementation
> > effectively defragments such files, whilst using XFS_BMAPI_CONVERT
> > just leaves all the fragments behind.
> 
> That's true - and it fragments unfragmented files.  Is ZERO_RANGE
> supposed to be a defragmenter?

...so please remember, the key point we were talking about when we
discussed this a year ago was that if the /entire/ zero range maps to a
single extent within eof then maybe we ought to just convert it to
unwritten.

Note also that for pmem there's a slightly different optimization --
if the entire range is mapped by written extents (not necessarily
contiguous, just no holes/cow/delalloc/unwritten bits) then we can use
blkdev_issue_zeroout to zero memory and clear hwpoison cheaply.

> >> (Note that this changes the rounding direction of the
> >> xfs_alloc_file_space range, because we only want to hit whole
> >> blocks within the range.)
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> ---
> >>
> >> <currently running fsx ad infinitum, so far so good>
> 
> <still running, so far so good (4k blocks)>
> 
> >> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> >> index 0a96c4d1718e..eae202bfe134 100644
> >> --- a/fs/xfs/xfs_bmap_util.c
> >> +++ b/fs/xfs/xfs_bmap_util.c
> >> @@ -1164,23 +1164,25 @@ xfs_zero_file_space(
> >>  
> >>  	blksize = 1 << mp->m_sb.sb_blocklog;
> >>  
> >> +	error = xfs_flush_unmap_range(ip, offset, len);
> >> +	if (error)
> >> +		return error;
> >>  	/*
> >> -	 * Punch a hole and prealloc the range. We use hole punch rather than
> >> -	 * unwritten extent conversion for two reasons:
> >> -	 *
> >> -	 * 1.) Hole punch handles partial block zeroing for us.
> >> -	 *
> >> -	 * 2.) If prealloc returns ENOSPC, the file range is still zero-valued
> >> -	 * by virtue of the hole punch.
> >> +	 * Convert whole blocks in the range to unwritten, then call iomap
> >> +	 * via xfs_zero_range to zero the range.  iomap will skip holes and
> >> +	 * unwritten extents, and just zero the edges if needed.  If conversion
> >> +	 * fails, iomap will simply write zeros to the whole range.
> >> +	 * nb: always_cow doesn't support unwritten extents.
> >>  	 */
> >> -	error = xfs_free_file_space(ip, offset, len);
> >> -	if (error || xfs_is_always_cow_inode(ip))
> >> -		return error;
> >> +	if (!xfs_is_always_cow_inode(ip))
> >> +		xfs_alloc_file_space(ip, round_up(offset, blksize),
> >> +				     round_down(offset + len, blksize) -
> >> +				     round_up(offset, blksize),
> >> +				     XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT);
> > 
> > If this fails with, say, corruption we should abort with an error,
> > not ignore it. I think we can only safely ignore ENOSPC and maybe
> > EDQUOT here...
> 
> Yes, I suppose so, though if this encounters corruption I'd guess
> xfs_zero_range probably would as well but that's just handwaving.

<nod>

> >> -	return xfs_alloc_file_space(ip, round_down(offset, blksize),
> >> -				     round_up(offset + len, blksize) -
> >> -				     round_down(offset, blksize),
> >> -				     XFS_BMAPI_PREALLOC);
> >> +	error = xfs_zero_range(ip, offset, len);
> > 
> > What prevents xfs_zero_range() from changing the file size if
> > offset + len is beyond EOF and there are allocated extents (from
> > delalloc conversion) beyond EOF? (i.e. FALLOC_FL_KEEP_SIZE is set by
> > the caller).
> 
> nothing, but AFAIK it does the same today... even w/o extents past
> EOF:
> 
> $ xfs_io -f -c "truncate 0" -c "fzero 0 1m" testfile

fzero -k ?

--D

> 
> $ ls -lh testfile
> -rw-------. 1 sandeen sandeen 1.0M Jun 24 21:48 testfile
> 
> $ xfs_bmap -vvp testfile
> testfile:
>  EXT: FILE-OFFSET      BLOCK-RANGE          AG AG-OFFSET            TOTAL FLAGS
>    0: [0..2047]:       183206064..183208111  2 (48988336..48990383)  2048 10000
>  FLAG Values:
>     010000 Unwritten preallocated extent
>     001000 Doesn't begin on stripe unit
>     000100 Doesn't end   on stripe unit
>     000010 Doesn't begin on stripe width
>     000001 Doesn't end   on stripe width
> 
> At the end of the day it's just one allocation behavior over another,
> it's not a correctness issue, so if there are concerns I don't have
> to push it...
> 
> -Eric
>  
> > Cheers,
> > 
> > Dave.
> > 

Re: [PATCH 2/2] xfs: convert extents in place for ZERO_RANGE

[Eric Sandeen]

On 6/24/19 10:00 PM, Darrick J. Wong wrote:
> On Mon, Jun 24, 2019 at 09:52:03PM -0500, Eric Sandeen wrote:
>> On 6/24/19 9:39 PM, Dave Chinner wrote:
>>> On Mon, Jun 24, 2019 at 07:48:11PM -0500, Eric Sandeen wrote:
>>>> Rather than completely removing and re-allocating a range
>>>> during ZERO_RANGE fallocate calls, convert whole blocks in the
>>>> range using xfs_alloc_file_space(XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT)
>>>> and then zero the edges with xfs_zero_range()
>>>
>>> That's what I originally used to implement ZERO_RANGE and that
>>> had problems with zeroing the partial blocks either side and
>>> unexpected inode size changes. See commit:
>>>
>>> 5d11fb4b9a1d xfs: rework zero range to prevent invalid i_size updates
>>
>> Yep I did see that.  It had a lot of hand-rolled partial block stuff
>> that seems more complex than this, no?  That commit didn't indicate
>> what the root cause of the failure actually was, AFAICT.
>>
>> (funny thought that I skimmed that commit just to see why we had
>> what we have, but didn't really intentionally re-implement it...
>> even though I guess I almost did...)
> 
> FWIW the complaint I had about the fragmentary behavior really only
> applied to fun and games when one fallocated an ext4 image and then ran
> mkfs.ext4 which uses zero range which fragmented the image...
> 
>>> I also remember discussion about zero range being inefficient on
>>> sparse files and fragmented files - the current implementation
>>> effectively defragments such files, whilst using XFS_BMAPI_CONVERT
>>> just leaves all the fragments behind.
>>
>> That's true - and it fragments unfragmented files.  Is ZERO_RANGE
>> supposed to be a defragmenter?
> 
> ...so please remember, the key point we were talking about when we
> discussed this a year ago was that if the /entire/ zero range maps to a
> single extent within eof then maybe we ought to just convert it to
> unwritten.

I remember you mentioning that, but honestly it seems like
overcomplication to say "ZERO_RANGE will also defragment extents
in the process, if it can..."

> Note also that for pmem there's a slightly different optimization --
> if the entire range is mapped by written extents (not necessarily
> contiguous, just no holes/cow/delalloc/unwritten bits) then we can use
> blkdev_issue_zeroout to zero memory and clear hwpoison cheaply.
> 
>>>> (Note that this changes the rounding direction of the
>>>> xfs_alloc_file_space range, because we only want to hit whole
>>>> blocks within the range.)
>>>>
>>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>>> ---
>>>>
>>>> <currently running fsx ad infinitum, so far so good>
>>
>> <still running, so far so good (4k blocks)>
>>
>>>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>>>> index 0a96c4d1718e..eae202bfe134 100644
>>>> --- a/fs/xfs/xfs_bmap_util.c
>>>> +++ b/fs/xfs/xfs_bmap_util.c
>>>> @@ -1164,23 +1164,25 @@ xfs_zero_file_space(
>>>>  
>>>>  	blksize = 1 << mp->m_sb.sb_blocklog;
>>>>  
>>>> +	error = xfs_flush_unmap_range(ip, offset, len);
>>>> +	if (error)
>>>> +		return error;
>>>>  	/*
>>>> -	 * Punch a hole and prealloc the range. We use hole punch rather than
>>>> -	 * unwritten extent conversion for two reasons:
>>>> -	 *
>>>> -	 * 1.) Hole punch handles partial block zeroing for us.
>>>> -	 *
>>>> -	 * 2.) If prealloc returns ENOSPC, the file range is still zero-valued
>>>> -	 * by virtue of the hole punch.
>>>> +	 * Convert whole blocks in the range to unwritten, then call iomap
>>>> +	 * via xfs_zero_range to zero the range.  iomap will skip holes and
>>>> +	 * unwritten extents, and just zero the edges if needed.  If conversion
>>>> +	 * fails, iomap will simply write zeros to the whole range.
>>>> +	 * nb: always_cow doesn't support unwritten extents.
>>>>  	 */
>>>> -	error = xfs_free_file_space(ip, offset, len);
>>>> -	if (error || xfs_is_always_cow_inode(ip))
>>>> -		return error;
>>>> +	if (!xfs_is_always_cow_inode(ip))
>>>> +		xfs_alloc_file_space(ip, round_up(offset, blksize),
>>>> +				     round_down(offset + len, blksize) -
>>>> +				     round_up(offset, blksize),
>>>> +				     XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT);
>>>
>>> If this fails with, say, corruption we should abort with an error,
>>> not ignore it. I think we can only safely ignore ENOSPC and maybe
>>> EDQUOT here...
>>
>> Yes, I suppose so, though if this encounters corruption I'd guess
>> xfs_zero_range probably would as well but that's just handwaving.
> 
> <nod>
> 
>>>> -	return xfs_alloc_file_space(ip, round_down(offset, blksize),
>>>> -				     round_up(offset + len, blksize) -
>>>> -				     round_down(offset, blksize),
>>>> -				     XFS_BMAPI_PREALLOC);
>>>> +	error = xfs_zero_range(ip, offset, len);
>>>
>>> What prevents xfs_zero_range() from changing the file size if
>>> offset + len is beyond EOF and there are allocated extents (from
>>> delalloc conversion) beyond EOF? (i.e. FALLOC_FL_KEEP_SIZE is set by
>>> the caller).
>>
>> nothing, but AFAIK it does the same today... even w/o extents past
>> EOF:
>>
>> $ xfs_io -f -c "truncate 0" -c "fzero 0 1m" testfile
> 
> fzero -k ?

$ xfs_io -f -c "truncate 0" -c "fzero -k 0 1m" testfile

$ ls -lh testfile
-rw-------. 1 sandeen sandeen 0 Jun 24 22:02 testfile

with or without my patches.

(with or without it also seems to allocate 1M past EOF...)

-Eric

Re: [PATCH 2/2] xfs: convert extents in place for ZERO_RANGE

[Darrick J. Wong]

On Mon, Jun 24, 2019 at 10:05:51PM -0500, Eric Sandeen wrote:
> On 6/24/19 10:00 PM, Darrick J. Wong wrote:
> > On Mon, Jun 24, 2019 at 09:52:03PM -0500, Eric Sandeen wrote:
> >> On 6/24/19 9:39 PM, Dave Chinner wrote:
> >>> On Mon, Jun 24, 2019 at 07:48:11PM -0500, Eric Sandeen wrote:
> >>>> Rather than completely removing and re-allocating a range
> >>>> during ZERO_RANGE fallocate calls, convert whole blocks in the
> >>>> range using xfs_alloc_file_space(XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT)
> >>>> and then zero the edges with xfs_zero_range()
> >>>
> >>> That's what I originally used to implement ZERO_RANGE and that
> >>> had problems with zeroing the partial blocks either side and
> >>> unexpected inode size changes. See commit:
> >>>
> >>> 5d11fb4b9a1d xfs: rework zero range to prevent invalid i_size updates
> >>
> >> Yep I did see that.  It had a lot of hand-rolled partial block stuff
> >> that seems more complex than this, no?  That commit didn't indicate
> >> what the root cause of the failure actually was, AFAICT.
> >>
> >> (funny thought that I skimmed that commit just to see why we had
> >> what we have, but didn't really intentionally re-implement it...
> >> even though I guess I almost did...)
> > 
> > FWIW the complaint I had about the fragmentary behavior really only
> > applied to fun and games when one fallocated an ext4 image and then ran
> > mkfs.ext4 which uses zero range which fragmented the image...
> > 
> >>> I also remember discussion about zero range being inefficient on
> >>> sparse files and fragmented files - the current implementation
> >>> effectively defragments such files, whilst using XFS_BMAPI_CONVERT
> >>> just leaves all the fragments behind.
> >>
> >> That's true - and it fragments unfragmented files.  Is ZERO_RANGE
> >> supposed to be a defragmenter?
> > 
> > ...so please remember, the key point we were talking about when we
> > discussed this a year ago was that if the /entire/ zero range maps to a
> > single extent within eof then maybe we ought to just convert it to
> > unwritten.
> 
> I remember you mentioning that, but honestly it seems like
> overcomplication to say "ZERO_RANGE will also defragment extents
> in the process, if it can..."

Well we could just do what we usually do and not write anything down
anywhere so 2022 Eric can argue with 2022 Dave and 2022 me about WTF
zero range is supposed to do.

Really, zero range doesn't specify the effects on the physical mapping.
All it says is that subsequent reads will return zeroes; that holes
will be filled with preallocations; and that preferably it converts to
unwritten extents.

It's that last part where it seems weird that we'd go out of our way to
punch and reallocate for a simple corner case where we could just
convert.

> > Note also that for pmem there's a slightly different optimization --
> > if the entire range is mapped by written extents (not necessarily
> > contiguous, just no holes/cow/delalloc/unwritten bits) then we can use
> > blkdev_issue_zeroout to zero memory and clear hwpoison cheaply.
> > 
> >>>> (Note that this changes the rounding direction of the
> >>>> xfs_alloc_file_space range, because we only want to hit whole
> >>>> blocks within the range.)
> >>>>
> >>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >>>> ---
> >>>>
> >>>> <currently running fsx ad infinitum, so far so good>
> >>
> >> <still running, so far so good (4k blocks)>
> >>
> >>>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> >>>> index 0a96c4d1718e..eae202bfe134 100644
> >>>> --- a/fs/xfs/xfs_bmap_util.c
> >>>> +++ b/fs/xfs/xfs_bmap_util.c
> >>>> @@ -1164,23 +1164,25 @@ xfs_zero_file_space(
> >>>>  
> >>>>  	blksize = 1 << mp->m_sb.sb_blocklog;
> >>>>  
> >>>> +	error = xfs_flush_unmap_range(ip, offset, len);
> >>>> +	if (error)
> >>>> +		return error;
> >>>>  	/*
> >>>> -	 * Punch a hole and prealloc the range. We use hole punch rather than
> >>>> -	 * unwritten extent conversion for two reasons:
> >>>> -	 *
> >>>> -	 * 1.) Hole punch handles partial block zeroing for us.
> >>>> -	 *
> >>>> -	 * 2.) If prealloc returns ENOSPC, the file range is still zero-valued
> >>>> -	 * by virtue of the hole punch.
> >>>> +	 * Convert whole blocks in the range to unwritten, then call iomap
> >>>> +	 * via xfs_zero_range to zero the range.  iomap will skip holes and
> >>>> +	 * unwritten extents, and just zero the edges if needed.  If conversion
> >>>> +	 * fails, iomap will simply write zeros to the whole range.
> >>>> +	 * nb: always_cow doesn't support unwritten extents.
> >>>>  	 */
> >>>> -	error = xfs_free_file_space(ip, offset, len);
> >>>> -	if (error || xfs_is_always_cow_inode(ip))
> >>>> -		return error;
> >>>> +	if (!xfs_is_always_cow_inode(ip))
> >>>> +		xfs_alloc_file_space(ip, round_up(offset, blksize),
> >>>> +				     round_down(offset + len, blksize) -
> >>>> +				     round_up(offset, blksize),
> >>>> +				     XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT);
> >>>
> >>> If this fails with, say, corruption we should abort with an error,
> >>> not ignore it. I think we can only safely ignore ENOSPC and maybe
> >>> EDQUOT here...
> >>
> >> Yes, I suppose so, though if this encounters corruption I'd guess
> >> xfs_zero_range probably would as well but that's just handwaving.
> > 
> > <nod>
> > 
> >>>> -	return xfs_alloc_file_space(ip, round_down(offset, blksize),
> >>>> -				     round_up(offset + len, blksize) -
> >>>> -				     round_down(offset, blksize),
> >>>> -				     XFS_BMAPI_PREALLOC);
> >>>> +	error = xfs_zero_range(ip, offset, len);
> >>>
> >>> What prevents xfs_zero_range() from changing the file size if
> >>> offset + len is beyond EOF and there are allocated extents (from
> >>> delalloc conversion) beyond EOF? (i.e. FALLOC_FL_KEEP_SIZE is set by
> >>> the caller).
> >>
> >> nothing, but AFAIK it does the same today... even w/o extents past
> >> EOF:
> >>
> >> $ xfs_io -f -c "truncate 0" -c "fzero 0 1m" testfile
> > 
> > fzero -k ?
> 
> $ xfs_io -f -c "truncate 0" -c "fzero -k 0 1m" testfile
> 
> $ ls -lh testfile
> -rw-------. 1 sandeen sandeen 0 Jun 24 22:02 testfile
> 
> with or without my patches.
> 
> (with or without it also seems to allocate 1M past EOF...)

ok cool.

--D

> -Eric
> 

Re: [PATCH 2/2] xfs: convert extents in place for ZERO_RANGE

[Dave Chinner]

On Mon, Jun 24, 2019 at 10:05:51PM -0500, Eric Sandeen wrote:
> On 6/24/19 10:00 PM, Darrick J. Wong wrote:
> > On Mon, Jun 24, 2019 at 09:52:03PM -0500, Eric Sandeen wrote:
> >> On 6/24/19 9:39 PM, Dave Chinner wrote:
> >>> On Mon, Jun 24, 2019 at 07:48:11PM -0500, Eric Sandeen wrote:
> >>>> Rather than completely removing and re-allocating a range
> >>>> during ZERO_RANGE fallocate calls, convert whole blocks in the
> >>>> range using xfs_alloc_file_space(XFS_BMAPI_PREALLOC|XFS_BMAPI_CONVERT)
> >>>> and then zero the edges with xfs_zero_range()
> >>>
> >>> That's what I originally used to implement ZERO_RANGE and that
> >>> had problems with zeroing the partial blocks either side and
> >>> unexpected inode size changes. See commit:
> >>>
> >>> 5d11fb4b9a1d xfs: rework zero range to prevent invalid i_size updates
> >>
> >> Yep I did see that.  It had a lot of hand-rolled partial block stuff
> >> that seems more complex than this, no?  That commit didn't indicate
> >> what the root cause of the failure actually was, AFAICT.
> >>
> >> (funny thought that I skimmed that commit just to see why we had
> >> what we have, but didn't really intentionally re-implement it...
> >> even though I guess I almost did...)
> > 
> > FWIW the complaint I had about the fragmentary behavior really only
> > applied to fun and games when one fallocated an ext4 image and then ran
> > mkfs.ext4 which uses zero range which fragmented the image...
> > 
> >>> I also remember discussion about zero range being inefficient on
> >>> sparse files and fragmented files - the current implementation
> >>> effectively defragments such files, whilst using XFS_BMAPI_CONVERT
> >>> just leaves all the fragments behind.
> >>
> >> That's true - and it fragments unfragmented files.  Is ZERO_RANGE
> >> supposed to be a defragmenter?
> > 
> > ...so please remember, the key point we were talking about when we
> > discussed this a year ago was that if the /entire/ zero range maps to a
> > single extent within eof then maybe we ought to just convert it to
> > unwritten.
> 
> I remember you mentioning that, but honestly it seems like
> overcomplication to say "ZERO_RANGE will also defragment extents
> in the process, if it can..."

Keep in mind that my original implementation of ZERO_RANGE was
for someone who explicitly requested zeroing of preallocated VM
image files without reallocating them. Hence the XFS_BMAPI_CONVERT
implementation. They'd been careful about initial allocation, and
they wanted to reuse image files for new VMs without perturbing
their initial careful preallocation patterns.

I think the punch+reallocate is a more generally useful behaviour
because people are far less careful about image file layout (e.g.
might be using extent size hints or discard within the VM) and so
we're more likely to see somewhat fragmented files for zeroing than
we are fully intact.

SO, yeah, I can see arguments for both cases, and situations where
one behaviour would be preferred over the other.

Random thought: KEEP_SIZE == I know what I'm doing, just convert in
place because the layout is as I want it. !KEEP_SIZE = punch and
preallocate because we are likely changing the file size anyway?

> >>> What prevents xfs_zero_range() from changing the file size if
> >>> offset + len is beyond EOF and there are allocated extents (from
> >>> delalloc conversion) beyond EOF? (i.e. FALLOC_FL_KEEP_SIZE is set by
> >>> the caller).
> >>
> >> nothing, but AFAIK it does the same today... even w/o extents past
> >> EOF:
> >>
> >> $ xfs_io -f -c "truncate 0" -c "fzero 0 1m" testfile
> > 
> > fzero -k ?
> 
> $ xfs_io -f -c "truncate 0" -c "fzero -k 0 1m" testfile
> 
> $ ls -lh testfile
> -rw-------. 1 sandeen sandeen 0 Jun 24 22:02 testfile
> 
> with or without my patches.

My concern was about files with pre-existing extents beyond EOF.
i.e. something like this (on a vanilla kernel):

$ xfs_io -f -c "truncate 0"  -c "pwrite 0 16m" -c "fsync" -c "bmap -vp" -c "fzero -k 0 32m" -c "bmap -vp" -c "stat" testfile
wrote 16777216/16777216 bytes at offset 0
16 MiB, 4096 ops; 0.0000 sec (700.556 MiB/sec and 179342.3530 ops/sec)
testfile:
 EXT: FILE-OFFSET      BLOCK-RANGE            AG AG-OFFSET            TOTAL FLAGS
   0: [0..65407]:      1145960728..1146026135 10 (47331608..47397015) 65408 000000
testfile:
 EXT: FILE-OFFSET      BLOCK-RANGE            AG AG-OFFSET            TOTAL FLAGS
   0: [0..65535]:      1146026136..1146091671 10 (47397016..47462551) 65536 010000
fd.path = "testfile"
fd.flags = non-sync,non-direct,read-write
stat.ino = 1342366656
stat.type = regular file
stat.size = 16777216
stat.blocks = 65536
fsxattr.xflags = 0x2 [-p--------------]
fsxattr.projid = 0
fsxattr.extsize = 0
fsxattr.cowextsize = 0
fsxattr.nextents = 1
fsxattr.naextents = 0
dioattr.mem = 0x200
dioattr.miniosz = 512
dioattr.maxiosz = 2147483136
$

So you can see it is 16MiB in size, but has 32MB of blocks allocated
to it, and it's a different 32MB of blocks that were allocated by
the delalloc because we punched and reallocated it as an unwritten
extent.

That's where I'm concerned - that range beyond EOF is no longer
punched away by this new code, an dit's not unwritten so the
zero_range is going to iterate and zero it, right?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com