Re: [PATCH 3/3] xfs: measure all contiguous previous extents for prealloc size

[Brian Foster <bfoster@redhat.com>]

On Mon, May 18, 2020 at 05:49:23PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're estimating a new speculative preallocation length for an
> extending write, we should walk backwards through the extent list to
> determine the number of number of blocks that are physically and
> logically contiguous with the write offset, and use that as an input to
> the preallocation size computation.
> 
> This way, preallocation length is truly measured by the effectiveness of
> the allocator in giving us contiguous allocations without being
> influenced by the state of a given extent.  This fixes both the problem
> where ZERO_RANGE within an EOF can reduce preallocation, and prevents
> the unnecessary shrinkage of preallocation when delalloc extents are
> turned into unwritten extents.
> 
> This was found as a regression in xfs/014 after changing delalloc writes
> to create unwritten extents during writeback.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/xfs_iomap.c |   63 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 52 insertions(+), 11 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index ac970b13b1f8..2dffd56a433c 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -351,6 +351,46 @@ xfs_quota_calc_throttle(
>  	}
>  }
>  
> +/*
> + * Determine if the previous extent's range of offsets is contiguous with
> + * @offset_fsb.  If so, set @prev_contig to the number of blocks that are
> + * physically contiguous with that previous extent and return true.  If there
> + * is no previous extent or there's a hole right before @offset_fsb, return
> + * false.
> + *
> + * Note that we don't care if the previous extents are written or not.

Why? :) Might be helpful to elaborate on why we require this algorithm
now...

> + */
> +static inline bool
> +xfs_iomap_prev_contiguous(
> +	struct xfs_ifork	*ifp,
> +	struct xfs_iext_cursor	*cur,
> +	xfs_fileoff_t		offset_fsb,
> +	xfs_extlen_t		*prev_contig)
> +{
> +	struct xfs_iext_cursor	ncur = *cur;
> +	struct xfs_bmbt_irec	got, old;
> +
> +	xfs_iext_prev(ifp, &ncur);
> +	if (!xfs_iext_get_extent(ifp, &ncur, &old))
> +		return false;
> +	if (old.br_startoff + old.br_blockcount < offset_fsb)
> +		return false;
> +
> +	*prev_contig = old.br_blockcount;
> +
> +	xfs_iext_prev(ifp, &ncur);
> +	while (xfs_iext_get_extent(ifp, &ncur, &got) &&
> +	       got.br_blockcount + got.br_startoff == old.br_startoff &&
> +	       got.br_blockcount + got.br_startblock == old.br_startblock &&
> +	       *prev_contig <= MAXEXTLEN) {
> +		*prev_contig += got.br_blockcount;
> +		old = got; /* struct copy */
> +		xfs_iext_prev(ifp, &ncur);
> +	}
> +
> +	return true;
> +}
> +
>  /*
>   * If we are doing a write at the end of the file and there are no allocations
>   * past this one, then extend the allocation out to the file system's write
> @@ -380,12 +420,12 @@ xfs_iomap_prealloc_size(
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, whichfork);
>  	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
> -	struct xfs_bmbt_irec	prev;
>  	int			shift = 0;
>  	int64_t			freesp;
>  	xfs_fsblock_t		qblocks;
>  	int			qshift = 0;
>  	xfs_fsblock_t		alloc_blocks = 0;
> +	xfs_extlen_t		plen = 0;
>  
>  	if (offset + count <= XFS_ISIZE(ip))
>  		return 0;
> @@ -400,9 +440,9 @@ xfs_iomap_prealloc_size(
>  	 */
>  	if ((mp->m_flags & XFS_MOUNT_ALLOCSIZE) ||
>  	    XFS_ISIZE(ip) < XFS_FSB_TO_B(mp, mp->m_dalign) ||
> -	    !xfs_iext_peek_prev_extent(ifp, icur, &prev) ||
> -	    prev.br_startoff + prev.br_blockcount < offset_fsb)
> +	    !xfs_iomap_prev_contiguous(ifp, icur, offset_fsb, &plen)) {
>  		return mp->m_allocsize_blocks;
> +	}
>  
>  	/*
>  	 * Determine the initial size of the preallocation. We are beyond the
> @@ -413,15 +453,16 @@ xfs_iomap_prealloc_size(
>  	 * preallocation size.
>  	 *
>  	 * If the extent is a hole, then preallocation is essentially disabled.
> -	 * Otherwise we take the size of the preceding data extent as the basis
> -	 * for the preallocation size. If the size of the extent is greater than
> -	 * half the maximum extent length, then use the current offset as the
> -	 * basis. This ensures that for large files the preallocation size
> -	 * always extends to MAXEXTLEN rather than falling short due to things
> -	 * like stripe unit/width alignment of real extents.
> +	 * Otherwise we take the size of the contiguous preceding data extents
> +	 * as the basis for the preallocation size. If the size of the extent

I'd refer to it as the "size of the contiguous range" or some such since
we're now talking about aggregating many extents to form the prealloc
basis.

I am a little curious if there's any noticeable impact from having to
perform the worst case extent walk in this path. For example, suppose we
had a speculatively preallocated file where we started writing (i.e.
converting) every other unwritten block such that this path had to walk
an extent per block until hitting the 8g max (8g == 2097152 4k blocks).
I guess the hope is that either most of those blocks wouldn't have been
written back and converted by the time we'd trigger the next post-eof
prealloc or that it would just be a wash in the stream of staggered
writes falling into our max sized preallocations. Either way, I think
it's more important to maintain the existing heuristic so this seems
reasonable from that perspective.

This scenario also makes me wonder if we should consider continuing to
do write time file size extension zeroing in certain cases vs. leaving
around unwritten extents. For example, the current post-eof prealloc
behavior is that writes that jump over current EOF will zero (via
buffered writes) any allocated blocks (delalloc or physical) between
current EOF and the start of the write. That behavior doesn't change
with delalloc -> unwritten if the prealloc is still delalloc at the time
of the extending write, but we'd presumably skip those zeroing writes if
the prealloc had been converted to real+unwritten first. Hmm.. that does
seem a little random to me, particularly if somebody starts to see
unwritten extents sprinkled throughout a file that never explicitly saw
preallocation. Perhaps we should avoid converting post-eof blocks? OTOH,
unwritten probably makes sense for large jumps in EOF vs zeroing GBs of
blocks, so one could argue that we should convert such ranges (if still
delalloc) rather than zero them at all. Thoughts? We should probably
work this out one way or another before landing the delalloc ->
unwritten behavior..

Brian

> +	 * is greater than half the maximum extent length, then use the current
> +	 * offset as the basis. This ensures that for large files the
> +	 * preallocation size always extends to MAXEXTLEN rather than falling
> +	 * short due to things like stripe unit/width alignment of real
> +	 * extents.
>  	 */
> -	if (prev.br_blockcount <= (MAXEXTLEN >> 1))
> -		alloc_blocks = prev.br_blockcount << 1;
> +	if (plen <= (MAXEXTLEN >> 1))
> +		alloc_blocks = plen << 1;
>  	else
>  		alloc_blocks = XFS_B_TO_FSB(mp, offset);
>  	if (!alloc_blocks)
>