Re: [PATCH 04/11] xfs: remove the if_ext_max field in struct xfs_ifork

[Mark Tinguely <tinguely@sgi.com>]

On 01/17/12 09:16, Ben Myers wrote:
> 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@lst.de>
>>>> Reviewed-by: Dave Chinner<dchinner@redhat.com>
>>>
>>> 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...
>>>


I am coming late into this review party, and I know this is time 
sensitive. I am looking at this from the big picture and you can ignore me.

Looking at the INTENTION of the tests, IMO, we are asking: "is it time 
to change format?" IMO, you do not want to flip between format without 
data count change - in other words, the two tests should NOT overlap.

/*
  * 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_NEXTENTS(ip, whichfork) <  /* less */
			XFS_IFORK_MAXEXT(ip, whichfork);
}


>>> ...
>>>
>>>> @@ -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


The original "XFS_IFORK_NEXTENTS(ip, whichfork)>= ifp->if_ext_max" is 
important because the removal of the blocks in xfs_bmap_del_extent() 
will create a hole that requires an insertion.


--Mark Tinguely
   tinguely@sgi.com


--Mark Tinguely.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs