xfs
[Top] [All Lists]

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

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 15/18] xfs: only free allocated regions of inode chunks
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 25 Jul 2014 09:24:47 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1406211788-63206-16-git-send-email-bfoster@xxxxxxxxxx>
References: <1406211788-63206-1-git-send-email-bfoster@xxxxxxxxxx> <1406211788-63206-16-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>