On Tue, Nov 15, 2011 at 03:14:08PM -0500, Christoph Hellwig wrote:
> With Dmitry fsstress updates I've seen very reproducible crashes in
> xfs_attr_shortform_remove because xfs_attr_shortform_bytesfit claims that
> the attributes would not fit inline into the inode after removing an
> attribute. It turns out that we were operating on an inode with lots
> of delalloc extents, and thus an if_bytes values for the data fork that
> is larger than biggest possible on-disk storage for it which utterly
> confuses the code near the end of xfs_attr_shortform_bytesfit.
We have a test that stresses allocated extents vs attributes in the
xfs_fsr swapext test (227), but that does not take into account
delalloc extents. It sounds like it would be relatively easy to
write a regression test for this particular case - create a file
with a bunch of attributes, then create a number of delalloc data
extents, then remove the attributes to trigger the condition in
xfs_attr_shortform_remove()....
> Fix this by always allowing the current attribute fork, like we already
> do for the attr1 format, given that delalloc conversion will take care
> for moving either the data or attribute area out of line if it doesn't
> fit at that point - or making the point moot by merging extents at this
> point.
>
> Also document the function better, and clean up some lose bits.
While you are touching that function, can you fix all the whitespace
damage as well? (lots of trailing whitespace). There's a couple of
typos I noticed in your changes (below), but otherwise looks good.
Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> Index: linux-2.6/fs/xfs/xfs_attr_leaf.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/xfs_attr_leaf.c 2011-11-04 13:46:59.481655134 +0100
> +++ linux-2.6/fs/xfs/xfs_attr_leaf.c 2011-11-04 15:58:06.480155275 +0100
> @@ -110,6 +110,7 @@ xfs_attr_namesp_match(int arg_flags, int
> /*
> * Query whether the requested number of additional bytes of extended
> * attribute space will be able to fit inline.
> + *
> * Returns zero if not, else the di_forkoff fork offset to be used in the
> * literal area for attribute data once the new bytes have been added.
> *
> @@ -136,11 +137,26 @@ xfs_attr_shortform_bytesfit(xfs_inode_t
> return (offset >= minforkoff) ? minforkoff : 0;
> }
>
> - if (!(mp->m_flags & XFS_MOUNT_ATTR2)) {
> - if (bytes <= XFS_IFORK_ASIZE(dp))
> - return dp->i_d.di_forkoff;
> + /*
> + * If the requested numbers of bytes is smaller or equal to the
> + * current attribute fork size we can always proceed.
> + *
> + * Note that if_bytes in the data fork might actually be larger than
> + * the current data fork size is due to delalloc extents. In that
> + * case either the extent count will go down when they are converted
> + * to ral extents, or the delalloc conversion will take care of the
real
> + * literal area rebalancing.
> + */
> + if (bytes <= XFS_IFORK_ASIZE(dp))
> + return dp->i_d.di_forkoff;
> +
> + /*
> + * For attr2 we can try to move the forkoff if there is space in the
> + * literal area, but for the old format we are done if there is no
> + * space in the fixes attribute fork.
> + */
> + if (!(mp->m_flags & XFS_MOUNT_ATTR2))
> return 0;
> - }
>
> dsize = dp->i_df.if_bytes;
>
> @@ -157,10 +173,9 @@ xfs_attr_shortform_bytesfit(xfs_inode_t
> xfs_default_attroffset(dp))
> dsize = XFS_BMDR_SPACE_CALC(MINDBTPTRS);
> break;
> -
> case XFS_DINODE_FMT_BTREE:
> /*
> - * If have data btree then keep forkoff if we have one,
> + * If have a data btree then keep forkoff if we have one,
If we have
> * otherwise we are adding a new attr, so then we set
> * minforkoff to where the btree root can finish so we have
> * plenty of room for attrs
> @@ -168,10 +183,9 @@ xfs_attr_shortform_bytesfit(xfs_inode_t
> if (dp->i_d.di_forkoff) {
> if (offset < dp->i_d.di_forkoff)
> return 0;
> - else
> - return dp->i_d.di_forkoff;
> - } else
> - dsize = XFS_BMAP_BROOT_SPACE(dp->i_df.if_broot);
> + return dp->i_d.di_forkoff;
> + }
> + dsize = XFS_BMAP_BROOT_SPACE(dp->i_df.if_broot);
> break;
> }
>
> @@ -186,10 +200,10 @@ xfs_attr_shortform_bytesfit(xfs_inode_t
> maxforkoff = XFS_LITINO(mp) - XFS_BMDR_SPACE_CALC(MINABTPTRS);
> maxforkoff = maxforkoff >> 3; /* rounded down */
>
> - if (offset >= minforkoff && offset < maxforkoff)
> - return offset;
> if (offset >= maxforkoff)
> return maxforkoff;
> + if (offset >= minforkoff)
> + return offset;
> return 0;
> }
>
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
>
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
