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@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...
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@xxxxxxx
--Mark Tinguely.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
