xfs
[Top] [All Lists]

Re: [PATCH 1/4] xfs: fix attr2 vs large data fork assert

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: fix attr2 vs large data fork assert
From: Ben Myers <bpm@xxxxxxx>
Date: Mon, 28 Nov 2011 12:01:36 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111128081925.458494771@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111128081732.350228200@xxxxxxxxxxxxxxxxxxxxxx> <20111128081925.458494771@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
Hey Christoph,

On Mon, Nov 28, 2011 at 03:17:33AM -0500, Christoph Hellwig wrote:
> With Dmitrys 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.
> 
> 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.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

This is missing some cleanups that you did for v2.  I'll use that
version, unless you have an objection.

Thanks,
        Ben


> Index: xfs/fs/xfs/xfs_attr_leaf.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_attr_leaf.c   2011-11-04 16:31:32.244656675 +0100
> +++ xfs/fs/xfs/xfs_attr_leaf.c        2011-11-05 09:01:32.613995075 +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
> +      * 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,
>                * 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

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