Re: [PATCH v2] xfs: account finobt blocks properly in perag reservation

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

On Thu, Jan 11, 2018 at 02:40:24PM -0500, Brian Foster wrote:
> XFS started using the perag metadata reservation pool for free inode
> btree blocks in commit 76d771b4cbe33 ("xfs: use per-AG reservations
> for the finobt"). To handle backwards compatibility, finobt blocks
> are accounted against the pool so long as the full reservation is
> available at mount time. Otherwise the ->m_inotbt_nores flag is set
> and the filesystem falls back to the traditional per-transaction
> finobt reservation.
> 
> This commit has two problems:
> 
> - finobt blocks are always accounted against the metadata
>   reservation on allocation, regardless of ->m_inotbt_nores state
> - finobt blocks are never returned to the reservation pool on free
> 
> The first problem affects reflink+finobt filesystems where the full
> finobt reservation is not available at mount time. finobt blocks are
> essentially stolen from the reflink reservation, putting refcountbt
> management at risk of allocation failure. The second problem is an
> unconditional leak of metadata reservation whenever finobt is
> enabled.
> 
> Update the finobt block allocation callouts to consider
> ->m_inotbt_nores and account blocks appropriately. Blocks should be
> consistently accounted against the metadata pool when
> ->m_inotbt_nores is false and otherwise tagged as RESV_NONE.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok, will test...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

> ---
> 
> v2:
> - Update both allocation and free paths to consider ->m_inotbt_nores.
> v1: https://marc.info/?l=linux-xfs&m=151552296126950&w=2
> 
>  fs/xfs/libxfs/xfs_ialloc_btree.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
> index 47f44d624cb1..af197a5f3a82 100644
> --- a/fs/xfs/libxfs/xfs_ialloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
> @@ -141,21 +141,42 @@ xfs_finobt_alloc_block(
>  	union xfs_btree_ptr	*new,
>  	int			*stat)
>  {
> +	if (cur->bc_mp->m_inotbt_nores)
> +		return xfs_inobt_alloc_block(cur, start, new, stat);
>  	return __xfs_inobt_alloc_block(cur, start, new, stat,
>  			XFS_AG_RESV_METADATA);
>  }
>  
>  STATIC int
> -xfs_inobt_free_block(
> +__xfs_inobt_free_block(
>  	struct xfs_btree_cur	*cur,
> -	struct xfs_buf		*bp)
> +	struct xfs_buf		*bp,
> +	enum xfs_ag_resv_type	resv)
>  {
>  	struct xfs_owner_info	oinfo;
>  
>  	xfs_rmap_ag_owner(&oinfo, XFS_RMAP_OWN_INOBT);
>  	return xfs_free_extent(cur->bc_tp,
>  			XFS_DADDR_TO_FSB(cur->bc_mp, XFS_BUF_ADDR(bp)), 1,
> -			&oinfo, XFS_AG_RESV_NONE);
> +			&oinfo, resv);
> +}
> +
> +STATIC int
> +xfs_inobt_free_block(
> +	struct xfs_btree_cur	*cur,
> +	struct xfs_buf		*bp)
> +{
> +	return __xfs_inobt_free_block(cur, bp, XFS_AG_RESV_NONE);
> +}
> +
> +STATIC int
> +xfs_finobt_free_block(
> +	struct xfs_btree_cur	*cur,
> +	struct xfs_buf		*bp)
> +{
> +	if (cur->bc_mp->m_inotbt_nores)
> +		return xfs_inobt_free_block(cur, bp);
> +	return __xfs_inobt_free_block(cur, bp, XFS_AG_RESV_METADATA);
>  }
>  
>  STATIC int
> @@ -380,7 +401,7 @@ static const struct xfs_btree_ops xfs_finobt_ops = {
>  	.dup_cursor		= xfs_inobt_dup_cursor,
>  	.set_root		= xfs_finobt_set_root,
>  	.alloc_block		= xfs_finobt_alloc_block,
> -	.free_block		= xfs_inobt_free_block,
> +	.free_block		= xfs_finobt_free_block,
>  	.get_minrecs		= xfs_inobt_get_minrecs,
>  	.get_maxrecs		= xfs_inobt_get_maxrecs,
>  	.init_key_from_rec	= xfs_inobt_init_key_from_rec,
> -- 
> 2.13.6
> 
> --
> 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