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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
