Re: [PATCH 15/18] xfs: only free allocated regions of inode chunks

[Dave Chinner <david@fromorbit.com>]

On Thu, Jul 24, 2014 at 10:23:05AM -0400, Brian Foster wrote:
> An inode chunk is currently added to the transaction free list based on
> a simple fsb conversion and hardcoded chunk length. The nature of sparse
> chunks is such that the physical chunk of inodes on disk may consist of
> one or more discontiguous parts. Blocks that reside in the holes of the
> inode chunk are not inodes and could be allocated to any other use or
> not allocated at all.
> 
> Refactor the existing xfs_bmap_add_free() call into the
> xfs_difree_inode_chunk() helper. The new helper uses the existing
> calculation if a chunk is not sparse. Otherwise, use the inobt record
> holemask to free the contiguous regions of the chunk.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_ialloc.c | 64 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index f75f191..1be57b1 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1659,6 +1659,66 @@ out_error:
>  	return error;
>  }
>  
> +/*
> + * Free the blocks of an inode chunk. We must consider that the inode chunk
> + * might be sparse and only free the regions that are allocated as part of the
> + * chunk.
> + */
> +STATIC void
> +xfs_difree_inode_chunk(
> +	struct xfs_mount		*mp,
> +	xfs_agnumber_t			agno,
> +	struct xfs_inobt_rec_incore	*rec,
> +	struct xfs_bmap_free		*flist)
> +{
> +	xfs_agblock_t	sagbno = XFS_AGINO_TO_AGBNO(mp, rec->ir_startino);
> +	xfs_agblock_t	agbno;
> +	int		nextbit;
> +	int		contig, contigblk;
> +	__uint16_t	allocmask;
> +	uint		allocbitmap;
> +
> +	if (!xfs_inobt_issparse(rec)) {
> +		/* not sparse, calculate extent info directly */
> +		xfs_bmap_add_free(XFS_AGB_TO_FSB(mp, agno,
> +				  XFS_AGINO_TO_AGBNO(mp, rec->ir_startino)),
> +				  mp->m_ialloc_blks, flist, mp);
> +		return;
> +	}
> +
> +	/*
> +	 * The bit flip and type conversion are intentionally done separately
> +	 * here to zero-extend the bitmask.
> +	 */
> +	allocmask = ~rec->ir_holemask;
> +	allocbitmap = allocmask;

If we are going to keep this code, then a helper is absolutely
necessary....

> +
> +	/*
> +	 * We now have an allocation bitmap in units of inodes at sparse chunk
> +	 * granularity (e.g., more than one inode per bit). Use the bitmask
> +	 * functions to find each contigious range of bits in the map. For each
> +	 * range, convert the start bit and count to block values and use that
> +	 * data to add the associated extent to the free list.
> +	 */
> +	nextbit = xfs_next_bit(&allocbitmap, 1, 0);
> +	while (nextbit != -1) {
> +		agbno = (nextbit * XFS_INODES_PER_SPCHUNK) /
> +				mp->m_sb.sb_inopblock;
> +		agbno += sagbno;
> +
> +		contig = xfs_contig_bits(&allocbitmap, 1, nextbit);
> +		contigblk = (contig * XFS_INODES_PER_SPCHUNK) /
> +				mp->m_sb.sb_inopblock;
> +
> +		ASSERT(agbno % xfs_ialloc_cluster_alignment(mp) == 0);
> +		ASSERT(contigblk % xfs_ialloc_cluster_alignment(mp) == 0);
> +		xfs_bmap_add_free(XFS_AGB_TO_FSB(mp, agno, agbno), contigblk,
> +				  flist, mp);
> +
> +		nextbit = xfs_next_bit(&allocbitmap, 1, nextbit + contig + 1);
> +	}

Again, I think that the generic bitmap code is a better way to
implement this...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs