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

Mark Tinguely
2012-01-17T17:04:45Z

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.