Re: [PATCH 2/2] xfs: use larger in-core attr firstused field and detect overflow

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 24, 2015 at 03:50:24PM -0500, Brian Foster wrote:
> The on-disk xfs_attr3_leaf_hdr structure firstused field is 16-bit and
> subject to overflow when fs block size is 64k. The field is typically
> initialized to block size when an attr leaf block is initialized. This
> problem is demonstrated by assert failures when running xfstests
> generic/117 on an fs with 64k blocks.
> 
> To support the existing attr leaf block algorithms for insertion,
> rebalance and entry movement, increase the size of the in-core firstused
> field to 32-bit and handle the potential overflow on conversion to/from
> the on-disk structure. If the overflow condition occurs, set a special
> value in the firstused field that is translated back on header read. The
> special value is only required in the case of an empty 64k attr block. A
> value of zero is used because firstused is initialized to the block size
> and grows backwards from there. Furthermore, the attribute block header
> occupies the first bytes of the block. Thus, a value of zero has no
> other legitimate meaning for this structure.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 36 +++++++++++++++++++++++++++++++++---
>  fs/xfs/libxfs/xfs_da_format.h |  8 +++++++-
>  2 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index 3337516..3277b40 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -106,6 +106,12 @@ xfs_attr3_leaf_hdr_from_disk(
>  		to->count = be16_to_cpu(hdr3->count);
>  		to->usedbytes = be16_to_cpu(hdr3->usedbytes);
>  		to->firstused = be16_to_cpu(hdr3->firstused);
> +		if (to->firstused == XFS_ATTR3_LEAF_NULLOFF) {
> +			/* only empty blocks when size overflows firstused! */
> +			ASSERT(!to->count && !to->usedbytes &&
> +			       geo->blksize > USHRT_MAX);
> +			to->firstused = geo->blksize;
> +		}
>  		to->holes = hdr3->holes;

So if it's already zero on disk, we set it to the block size....

>  		for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
> @@ -120,6 +126,12 @@ xfs_attr3_leaf_hdr_from_disk(
>  	to->count = be16_to_cpu(from->hdr.count);
>  	to->usedbytes = be16_to_cpu(from->hdr.usedbytes);
>  	to->firstused = be16_to_cpu(from->hdr.firstused);
> +	if (to->firstused == XFS_ATTR3_LEAF_NULLOFF) {
> +		/* only empty blocks when size overflows firstused! */
> +		ASSERT(!to->count && !to->usedbytes &&
> +		       geo->blksize > USHRT_MAX);
> +		to->firstused = geo->blksize;
> +	}
>  	to->holes = from->hdr.holes;

And duplicate the code here as well.

>  	for (i = 0; i < XFS_ATTR_LEAF_MAPSIZE; i++) {
> @@ -134,11 +146,29 @@ xfs_attr3_leaf_hdr_to_disk(
>  	struct xfs_attr_leafblock	*to,
>  	struct xfs_attr3_icleaf_hdr	*from)
>  {
> -	int	i;
> +	int				i;
> +	uint16_t			firstused;
>  
>  	ASSERT(from->magic == XFS_ATTR_LEAF_MAGIC ||
>  	       from->magic == XFS_ATTR3_LEAF_MAGIC);
>  
> +	/*
> +	 * Handle overflow of the on-disk firstused field. firstused is
> +	 * typically initialized to block size, but we only have 2-bytes in the
> +	 * on-disk structure. This means a 64k block size overflows the field.
> +	 *
> +	 * firstused should only match block size for an empty attr block so set
> +	 * a special value that the from_disk() variant can convert back to
> +	 * blocksize in the in-core structure.
> +	 */
> +	if (from->firstused > USHRT_MAX) {
> +		ASSERT(from->firstused == geo->blksize);
> +		firstused = XFS_ATTR3_LEAF_NULLOFF;
> +	} else {
> +		ASSERT(from->firstused != 0);
> +		firstused = from->firstused;
> +	}

Ok, that makes this a non-trivial sized function now :P

I think helpers might be appropriate...

> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 0a49b02..d2d0498 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -725,7 +725,7 @@ struct xfs_attr3_icleaf_hdr {
>  	__uint16_t	magic;
>  	__uint16_t	count;
>  	__uint16_t	usedbytes;
> -	__uint16_t	firstused;
> +	__uint32_t	firstused;

Needs a comment explaining why it's different to the on-disk
structure.

>  	__u8		holes;
>  	struct {
>  		__uint16_t	base;
> @@ -734,6 +734,12 @@ struct xfs_attr3_icleaf_hdr {
>  };
>  
>  /*
> + * Special value to represent fs block size in the leaf header firstused field.
> + * Only used when block size overflows the 2-bytes available on disk.
> + */
> +#define XFS_ATTR3_LEAF_NULLOFF	0
> +

I think declaring a pair of helper functions (e.g.
xfs_attr3_leaf_firstused_to/from_disk) here would be a good idea.
That way all the coversions, magic numbers and comments documenting
the behaviour are in the one place in the on-disk format
specification....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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