| Message ID | 25a2ebbc-0ec9-f5dd-eba0-4101c80837dd@sandeen.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: don't fragment files with ZERO_RANGE calls | expand |
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.
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. >
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. > >
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
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 >
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.
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
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>