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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, May 22, 2020 at 07:27:22AM -0400, Brian Foster wrote:
> On Thu, May 21, 2020 at 07:53:09PM -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 |   37 +++++++++++++++++++++++++------------
> >  1 file changed, 25 insertions(+), 12 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> > index ac970b13b1f8..6a308af93893 100644
> > --- a/fs/xfs/xfs_iomap.c
> > +++ b/fs/xfs/xfs_iomap.c
> > @@ -377,15 +377,17 @@ xfs_iomap_prealloc_size(
> >  	loff_t			count,
> >  	struct xfs_iext_cursor	*icur)
> >  {
> > +	struct xfs_iext_cursor	ncur = *icur; /* struct copy */
> > +	struct xfs_bmbt_irec	prev, got;
> >  	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;
> > +	int			shift = 0;
> > +	int			qshift = 0;
> >  
> >  	if (offset + count <= XFS_ISIZE(ip))
> >  		return 0;
> > @@ -413,16 +415,27 @@ 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 preceding data extents as the basis
> > +	 * for the preallocation size. Note that we don't care if the previous
> > +	 * extents are written or not.
> > +	 *
> > +	 * If the size of the extents 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;
> > -	else
> > +	plen = prev.br_blockcount;
> 
> If prev is initialized by peeking the previous extent, then it looks
> like the first iteration of this loop compares the immediately previous
> extent with itself..

D'oh.  I misported that when I was munging patches around.  Since we
copy *icur to ncur, we can replace the previous peek against icur with a
call to xfs_iext_prev_extent on ncur.

> > +	while (xfs_iext_prev_extent(ifp, &ncur, &got)) {
> > +		if (plen > MAXEXTLEN / 2 ||
> > +		    got.br_startoff + got.br_blockcount != prev.br_startoff ||
> > +		    got.br_startblock + got.br_blockcount != prev.br_startblock)
> 
> We should probably check for nullstartblock (delalloc) extents
> explicitly here rather than rely on the calculation to fail.

Ok.

> > +			break;
> > +		plen += got.br_blockcount;
> 
> 
> 
> > +		prev = got;
> > +	}
> > +	alloc_blocks = plen * 2;
> 
> Why do we replace the bit shifts with division/multiplication? I'd
> prefer to see the former for obvious power of 2 operations, even if this
> happens to be 32-bit arithmetic. I don't see any particular reason to
> change it in this patch.

Fair enough.  Thanks for catching those things.

--D

> Brian
> 
> > +	if (alloc_blocks > MAXEXTLEN)
> >  		alloc_blocks = XFS_B_TO_FSB(mp, offset);
> >  	if (!alloc_blocks)
> >  		goto check_writeio;
> > 
>