Re: [PATCH v11 2/2] xfs: add FALLOC_FL_WRITE_ZEROES to XFS code base

[Lukas Herbolt <lukas@herbolt.com>]

> Honestly, this "prealloc, set file size, then convert and zero" thing
> seems a bit ... clunky.
> 
> I mean, this is trying to work around an issue with
> xfs_falloc_setsize() operating on written extents beyond EOF to set
> EOF. But why are we even using a generic truncate operation to set
> the EOF when we have already done most of the complex truncate work
> (exclusion, page cache invalidation, extent removal, etc) already?
> 
> My logic:
> 
> xfs_bmapi_write(off, len, XFS_BMAPI_ZERO) will allocate all the
> holes and delalloc extents over the given range as written extents
> that are initialised to contain zeroes.
> 
> xfs_bmapi_write(off, len, XFS_BMAPI_CONVERT) will convert all the
> unwritten extents over a given range to written extents, but will
> not touch the data in those extents.
> 
> xfs_bmapi_write(off, len, XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO) will
> allocate holes and delalloc extents and convert unwritten extents
> all to written extents, and it will write zeros to all of those
> extents.
> 
> This latter case is exactly what we want, and if gives us zeroes on
> disk to the end of the extent beyond the new EOF. i.e. we've
> completed all the data IO, and so now all we need
> to do is update the file size transactionally, just like we do in
> IO completion.
> 
> Indeed, xfs_iomap_write_unwritten() integrates this size update
> into post-IO unwritten extent conversion. i.e we already have code
> that does exactly what we need, except for tweaking the
> xfs_bmapi_write() flags for the zeroing behaviour we need.
> 
> So, a helper function in fs/xfs/xfs_bmap_util.c such as this:
> 
> /*
>  * This function is used to allocate written extents over holes
>  * and/or convert unwritten extents to written extents based on the
>  * @flags passed to it.
>  *
>  * If @flags is zero, if will allocate written extents for holes and
>  * delalloc extents across the range.
>  *
>  * If XFS_BMAPI_CONVERT is specified in @flags, then it will also do
>  * conversion of unwritten extents in the range to written extents.
>  *
>  * If XFS_BMAPI_ZERO is specified in @flags, then both newly
>  * allocated extents and converted unwritten extents will be
>  * initialised to contain zeroes.
>  *
>  * If @update_isize is true, then if the range we are operating on
>  * extends beyond the current EOF, extend i_size to offset+len
>  * incrementally as extents in the range are allocated/converted.
>  */
> int
> xfs_bmap_alloc_or_convert_range(
> 	struct xfs_inode	*ip,
> 	xfs_off_t		offset,
> 	xfs_off_t		len,
> 	uint32_t		flags,
> 	bool			update_isize)
> {
> 	< guts of xfs_iomap_write_unwritten >
> }
> 
> Then xfs_iomap_write_unwritten() can be factored to a one-liner
> (or replaced altogether), and this new fallocate op pretty much
> becomes:
> 
> 	error = xfs_bmap_alloc_or_convert_range(ip, offset, len,
> 			XFS_BMAPI_CONVERT | XFS_BMAPI_ZERO,
> 			new_size ? true : false);
> 
> The name of the function sucks but I can't think of anything better,
> so if you've got a better idea then change it. :)
> 
> -Dave.

When I first start looking into it the xfs_iomap_write_unwritten() was 
first place I was exploring, but did not think of the the above. I 
started working on it but I will be travelling this week so after that 
you may
expect v13.

-- 
-lhe