Re: [PATCH v3 08/18] xfs: helpers to convert holemask to/from generic bitmap

Dave Chinner
2015-02-08T23:58:05Z

On Fri, Feb 06, 2015 at 02:52:55PM -0500, Brian Foster wrote:
> The inobt record holemask field is a condensed data type designed to fit
> into the existing on-disk record and is zero based (allocated regions
> are set to 0, sparse regions are set to 1) to provide backwards
> compatibility. This makes the type somewhat complex for use in higher
> level inode manipulations such as individual inode allocation, etc.
>
> Rather than foist the complexity of dealing with this field to every bit
> of logic that requires inode granular information, create the
> xfs_inobt_ialloc_bitmap() helper to convert the holemask to an inode
> allocation bitmap. The inode allocation bitmap is inode granularity
> similar to the inobt record free mask and indicates which inodes of the
> chunk are physically allocated on disk, irrespective of whether the
> inode is considered allocated or free by the filesystem. The generic
> bitmap type requires the use of generic kernel bitmap APIs.
>
> The bitmap_to_u64() helper is provided to convert the bitmap back to a
> native 64-bit type (for native bitwise operations). This is required
> because the generic bitmap code represents a bitmap as an array of
> unsigned long in a little endian style (though each array value is cpu
> order). To ensure compatibility with various word sizes and endianness',
> bitmap_to_u64() exports the bitmap to little endian and swaps back to
> cpu byte order.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> fs/xfs/libxfs/xfs_ialloc.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 69 insertions(+)
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 72ade0e..fc001d9 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -39,6 +39,8 @@
> #include "xfs_icache.h"
> #include "xfs_trace.h"
>
> +STATIC void
> +xfs_inobt_ialloc_bitmap(unsigned long *, struct xfs_inobt_rec_incore *);

xfs_inobt_irec_to_allocmap() seems more appropriate for what it does.
I'd also suggest it should be in xfs_ialloc_btree.c, too.

> /*
> * Allocation group level functions.
> @@ -739,6 +741,73 @@ xfs_ialloc_get_rec(
> return 0;
> }
>
> +static inline uint64_t
> +bitmap_to_u64(
> + const unsigned long *bitmap,
> + int nbits)
> +{
> + __le64 lebitmap;
> +
> + ASSERT(nbits <= 64);
> +
> + /*
> + * The bitmap format depends on the native word size. E.g., bit 0 could
> + * land in the middle of the 64 bits on a big endian 32-bit arch (see
> + * arch/powerpc/include/asm/bitops.h). To handle this, export the bitmap
> + * as 64-bit little endian and convert back to native byte order.
> + */
> + bitmap_copy_le(&lebitmap, bitmap, nbits);
> + return le64_to_cpu(lebitmap);
> +}

Ok, so this is for doing logic operations on the bitmap, such as
inverting or masking out a bunch of bits?

The first use of this function is xfs_inobt_first_free_inode(), and
the other use is in xfs_difree_inobt() to build the mask of inodes
in the chunk that need to be freed, and in both those cases they do

xfs_inobt_ialloc_bitmap(alloc, rec)
allocmask = bitmap_to_u64(alloc, 64);

and the local stack bitmap is otherwise unused. So, wouldn't a
helper like:

/*
* Return a host format mask of all the allocated inodes in the
* chunk. The bitmap format depends on the native word size (e.g.
* see arch/powerpc/include/asm/bitops.h) and so we have to marshall
* the bitmap through a defined endian conversion so that we can
* perform host native 64 bit logic operations on the resultant
* allocation mask.
*
* A bit value of 1 means the inode is allocated, a value of 0 means it
* is free.
*/
u64
xfs_inobt_irec_to_allocmask(
struct xfs_inobt_rec_incore *irec)
{
DECLARE_BITMAP(allocmap, 64),
__le64 lebitmap;

xfs_inobt_rec_to_allocmap(&allocmap, irec);
bitmap_copy_le(&lebitmap, allocmap, 64);
return le64_to_cpu(lebitmap);
}

> +
> +/*
> + * Convert the inode record holemask to an inode allocation bitmap. The inode
> + * allocation bitmap is inode granularity and specifies whether an inode is
> + * physically allocated on disk (not whether the inode is considered allocated
> + * or free by the fs).
> + *
> + * We have to be careful with regard to byte order and word size since the
> + * generic bitmap code uses primitive types.

" a bit value of 1 means the inode is allocated, a value of 0 means
it is free"

> + */
> +STATIC void
> +xfs_inobt_ialloc_bitmap(
> + unsigned long *allocbmap,
> + struct xfs_inobt_rec_incore *rec)
> +{
> + int nextbit;
> + DECLARE_BITMAP(holemask, 16);
> +
> + bitmap_zero(allocbmap, 64);
> +
> + /*
> + * bitmaps are represented as an array of unsigned long (each in cpu
> + * byte order). ir_holemask is only 16 bits, so direct assignment is
> + * safe.
> + */
> + ASSERT(sizeof(rec->ir_holemask) <= sizeof(holemask[0]));

BUILD_BUG_ON(sizeof(rec->ir_holemask) <= sizeof(holemask[0]));

i.e. if we come across a platform where this fails, break the build
rather than waiting for the unlikely event of someone running a
debug kernel on that platform...

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com