On Mon, Jan 16, 2012 at 04:45:27PM -0600, Ben Myers wrote:
> 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?
Here is what I propose to use here:
@@ -5322,7 +5319,8 @@ xfs_bunmapi(
*/
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 &&
+ XFS_IFORK_NEXTENTS(ip, whichfork) >= /* Note the >= */
+ XFS_IFORK_MAXEXT(ip, whichfork) &&
del.br_startoff > got.br_startoff &&
del.br_startoff + del.br_blockcount <
got.br_startoff + got.br_blockcount) {
-Ben
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
