Re: [PATCH 02/10] xfs: rewrite xfs_da_grow_inode_int

[Brian Foster <bfoster@redhat.com>]

On Thu, Apr 13, 2017 at 10:05:09AM +0200, Christoph Hellwig wrote:
> Use one xfs_bmapi_write loop that just iterates if it didn't manage to
> allocate enough blocks.  Get rid of the separate contig call that just
> makes us call the allocator twice, and also get rid of the memory
> allocation for the map array that we don't actually need.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_da_btree.c | 94 +++++++++++++-------------------------------
>  1 file changed, 28 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 1bdf2888295b..00853d332bcd 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -2007,84 +2007,46 @@ xfs_da_grow_inode_int(
>  	xfs_fileoff_t		*bno,
>  	int			count)
>  {
> -	struct xfs_trans	*tp = args->trans;
> -	struct xfs_inode	*dp = args->dp;
>  	int			w = args->whichfork;
> -	xfs_rfsblock_t		nblks = dp->i_d.di_nblocks;
> -	struct xfs_bmbt_irec	map, *mapp;
> -	int			nmap, error, got, i, mapi;
> +	xfs_fileoff_t		b;
> +	int			error;
>  
>  	/*
>  	 * Find a spot in the file space to put the new block.
>  	 */
> -	error = xfs_bmap_first_unused(tp, dp, count, bno, w);
> +	error = xfs_bmap_first_unused(args->trans, args->dp, count, bno, w);
>  	if (error)
>  		return error;
>  
> -	/*
> -	 * Try mapping it in one filesystem block.
> -	 */
> -	nmap = 1;
> -	ASSERT(args->firstblock != NULL);
> -	error = xfs_bmapi_write(tp, dp, *bno, count,
> -			xfs_bmapi_aflag(w)|XFS_BMAPI_METADATA|XFS_BMAPI_CONTIG,
> -			args->firstblock, args->total, &map, &nmap,
> -			args->dfops);
> -	if (error)
> -		return error;
> +	b = *bno;
> +	do {
> +		xfs_rfsblock_t nblks = args->dp->i_d.di_nblocks;
> +		struct xfs_bmbt_irec imap;
> +		xfs_extlen_t len;
> +		int nmap = 1;
> +
> +		error = xfs_bmapi_write(args->trans, args->dp, b, count,
> +				xfs_bmapi_aflag(w) | XFS_BMAPI_METADATA,
> +				args->firstblock, args->total, &imap, &nmap,
> +				args->dfops);
> +		if (error)
> +			return error;
> +		if (!nmap)
> +			return -ENOSPC;
>  
> -	ASSERT(nmap <= 1);
> -	if (nmap == 1) {
> -		mapp = &map;
> -		mapi = 1;
> -	} else if (nmap == 0 && count > 1) {
> -		xfs_fileoff_t		b;
> -		int			c;
> +		len = imap.br_startoff + imap.br_blockcount - b;
> +		ASSERT(imap.br_startoff <= b);
> +		ASSERT(len > 0);
> +		ASSERT(len <= count);
>  
> -		/*
> -		 * If we didn't get it and the block might work if fragmented,
> -		 * try without the CONTIG flag.  Loop until we get it all.
> -		 */
> -		mapp = kmem_alloc(sizeof(*mapp) * count, KM_SLEEP);
> -		for (b = *bno, mapi = 0; b < *bno + count; ) {
> -			nmap = MIN(XFS_BMAP_MAX_NMAP, count);
> -			c = (int)(*bno + count - b);
> -			error = xfs_bmapi_write(tp, dp, b, c,
> -					xfs_bmapi_aflag(w)|XFS_BMAPI_METADATA,
> -					args->firstblock, args->total,
> -					&mapp[mapi], &nmap, args->dfops);
> -			if (error)
> -				goto out_free_map;
> -			if (nmap < 1)
> -				break;
> -			mapi += nmap;
> -			b = mapp[mapi - 1].br_startoff +
> -			    mapp[mapi - 1].br_blockcount;
> -		}
> -	} else {
> -		mapi = 0;
> -		mapp = NULL;
> -	}
> -
> -	/*
> -	 * Count the blocks we got, make sure it matches the total.
> -	 */
> -	for (i = 0, got = 0; i < mapi; i++)
> -		got += mapp[i].br_blockcount;
> -	if (got != count || mapp[0].br_startoff != *bno ||
> -	    mapp[mapi - 1].br_startoff + mapp[mapi - 1].br_blockcount !=
> -	    *bno + count) {
> -		error = -ENOSPC;
> -		goto out_free_map;
> -	}
> +		/* account for newly allocated blocks in reserved blocks total */
> +		args->total -= (args->dp->i_d.di_nblocks - nblks);
>  
> -	/* account for newly allocated blocks in reserved blocks total */
> -	args->total -= dp->i_d.di_nblocks - nblks;
> +		b += len;
> +		count -= len;
> +	} while (count > 0);
>  
> -out_free_map:
> -	if (mapp != &map)
> -		kmem_free(mapp);
> -	return error;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html