Hey Christoph,
On Fri, Jan 06, 2012 at 10:58:18AM -0600, Ben Myers wrote:
> On Sun, Dec 18, 2011 at 03:00:07PM -0500, Christoph Hellwig wrote:
> > We spent a lot of effort to maintain this field, but it always equalts to the
> equals the
> > fork size divided by the constant size of an extent. The prime use of it is
> > to assert that the two stay in sync. Just divide the fork size by the extent
> > size in the few places that we actually use it and remove the overhead
> > of maintaining it. Also introduce a few helpers to consolidate the places
> > where we actually care about the value.
> >
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>
>
> After reviewing this patch it's not crystal clear to me why we were
> putting all that effort into keeping this counter uptodate on the inode
> instead of using helpers like you've implemented. Maybe a question of
> integer division as Dave suggested. This is a nice improvement.
>
> > Index: xfs/fs/xfs/xfs_bmap.c
> > ===================================================================
> > --- xfs.orig/fs/xfs/xfs_bmap.c 2011-12-12 10:33:55.748696870 -0800
> > +++ xfs/fs/xfs/xfs_bmap.c 2011-12-14 05:15:20.612373687 -0800
> > @@ -249,7 +249,27 @@ xfs_bmbt_lookup_ge(
> > }
> >
> > /*
> > -* Update the record referred to by cur to the value given
> > + * Check if the inode needs to be converted to btree format.
> > + */
> > +static inline bool xfs_bmap_needs_btree(struct xfs_inode *ip, int whichfork)
> > +{
> > + return XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS &&
> > + XFS_IFORK_NEXTENTS(ip, whichfork) >
> > + XFS_IFORK_MAXEXT(ip, whichfork);
> > +}
> > +
> > +/*
> > + * Check if the inode should be converted to extent format.
> > + */
> > +static inline bool xfs_bmap_wants_extents(struct xfs_inode *ip, int whichfork)
> > +{
> > + return XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_BTREE &&
> > + XFS_IFORK_NEXTENTS(ip, whichfork) <=
> > + XFS_IFORK_MAXEXT(ip, whichfork);
> > +}
>
> The logic in these two appears to be equivalent to the code you've
> replaced in all but one case...
>
> ...
>
> > @@ -5321,8 +5318,7 @@ xfs_bunmapi(
> > * will be dirty.
> > */
> > if (!wasdel && xfs_trans_get_block_res(tp) == 0 &&
> > - XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_EXTENTS &&
> > - XFS_IFORK_NEXTENTS(ip, whichfork) >= ifp->if_ext_max &&
> ^^
> All other tests for this were:
> XFS_IFORK_NEXTENTS(ip, whichfork) > ifp->if_ext_max
>
> Did you just fix a lurking off-by-one or insert one?
>
> xfs_bmap_needs_btree needs ip->i_d.di_nextents to have been incremented
> already in order to detect that we need to convert to btree format. In
> this case we haven't done that yet and are checking to see if doing so
> would require conversion to btree format...
>
> Looks to me like we can't use xfs_bmap_needs_btree here and should use
> the old logic. Right?
HCH, I have a question for you here that I feel needs to be resolved.
Can you take a look?
Thanks,
Ben
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
