xfs: check on-disk (not incore) btree root size in dfrag.c

xfs_swap_extents_check_format() contains checks to make sure that original and the temporary files during defrag are compatible; Gabriel VLASIU ran into a case where xfs_fsr returned EINVAL because the tests found the btree root to be of size 120, while the fork offset was only 104; IOW, they overlapped.

However, this is just due to an error in the xfs_swap_extents_check_format() tests, because it is checking the in-memory btree root size against the on-disk fork offset. We should be checking the on-disk sizes in both cases.

This patch adds a new macro to calculate this size, and uses it in the tests.

With this change, the filesystem image provided by Gabriel allows for proper file degragmentation.

Reported-by Gabriel VLASIU <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_bmap_btree.h +3 -1

index 0e66c4e..d9eeb64 100644
@@ -189,12 +189,14 @@ typedef __be64 xfs_bmbt_ptr_t, xfs_bmdr_ptr_t;
 #define XFS_BMAP_BROOT_SPACE_CALC(nrecs) \
 	(int)(XFS_BTREE_LBLOCK_LEN + \
 	       ((nrecs) * (sizeof(xfs_bmbt_key_t) + sizeof(xfs_bmbt_ptr_t))))
-
 #define XFS_BMAP_BROOT_SPACE(bb) \
 	(XFS_BMAP_BROOT_SPACE_CALC(be16_to_cpu((bb)->bb_numrecs)))
+
 #define XFS_BMDR_SPACE_CALC(nrecs) \
 	(int)(sizeof(xfs_bmdr_block_t) + \
 	       ((nrecs) * (sizeof(xfs_bmbt_key_t) + sizeof(xfs_bmbt_ptr_t))))
+#define XFS_BMAP_BMDR_SPACE(bb) \
+	(XFS_BMDR_SPACE_CALC(be16_to_cpu((bb)->bb_numrecs)))
 
 /*
  * Maximum number of bmap btree levels.

fs/xfs/xfs_dfrag.c +4 -1

index dd974a5..2707d86 100644
@@ -26,6 +26,9 @@
 #include "xfs_ag.h"
 #include "xfs_mount.h"
 #include "xfs_bmap_btree.h"
+#include "xfs_alloc_btree.h"
+#include "xfs_ialloc_btree.h"
+#include "xfs_btree.h"
 #include "xfs_dinode.h"
 #include "xfs_inode.h"
 #include "xfs_inode_item.h"

@@ -184,7 +187,7 @@ xfs_swap_extents_check_format(
 	 */
 	if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE) {
 		if (XFS_IFORK_BOFF(ip) &&
-		    tip->i_df.if_broot_bytes > XFS_IFORK_BOFF(ip))
+		    XFS_BMAP_BMDR_SPACE(tip->i_df.if_broot) > XFS_IFORK_BOFF(ip))
 			return EINVAL;
 		if (XFS_IFORK_NEXTENTS(tip, XFS_DATA_FORK) <=
 		    XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK))

Quoted 11 lines · Eric Sandeen

xfs_swap_extents_check_format() contains checks to make sure that original and the temporary files during defrag are compatible; Gabriel VLASIU ran into a case where xfs_fsr returned EINVAL because the tests found the btree root to be of size 120, while the fork offset was only 104; IOW, they overlapped.

However, this is just due to an error in the xfs_swap_extents_check_format() tests, because it is checking the in-memory btree root size against the on-disk fork offset. We should be checking the on-disk sizes in both cases.

Quoted 5 lines

This patch adds a new macro to calculate this size, and uses it in the tests.

With this change, the filesystem image provided by Gabriel allows for proper file degragmentation.

Hm, as usually happens right after finalizing this I stumbled on something else. xfs_iroot_realloc() does essentially the same test, but uses a funky macro to resolve the incore/ondisk size difference:

        ASSERT(ifp->if_broot_bytes <=
                XFS_IFORK_SIZE(ip, whichfork) + XFS_BROOT_SIZE_ADJ);

so dfrag.c could be fixed up the same way, I suppose, using XFS_BROOT_SIZE_ADJ if desired (though I have no real love for that undocumented macro!)

XFS_BROOT_SIZE_ADJ is an undocumented macro which accounts for the difference in size between the on-disk and in-core btree root. It's much clearer to just use the newly-added XFS_BMAP_BMDR_SPACE macro which gives us the on-disk size directly.

In one case, we must test that the if_broot exists before applying the macro, however.

Signed-off-by Eric Sandeen <[email protected]>

xfs_swap_extents_check_format() contains checks to make sure that original and the temporary files during defrag are compatible; Gabriel VLASIU ran into a case where xfs_fsr returned EINVAL because the tests found the btree root to be of size 120, while the fork offset was only 104; IOW, they overlapped.

However, this is just due to an error in the xfs_swap_extents_check_format() tests, because it is checking the in-memory btree root size against the on-disk fork offset. We should be checking the on-disk sizes in both cases.

This patch adds a new macro to calculate this size, and uses it in the tests.

With this change, the filesystem image provided by Gabriel allows for proper file degragmentation.

Reported-by Gabriel VLASIU <[email protected]>

Signed-off-by Eric Sandeen <[email protected]>

---

fs/xfs/xfs_bmap_btree.h +3 -1

index 0e66c4e..d9eeb64 100644
@@ -189,12 +189,14 @@ typedef __be64 xfs_bmbt_ptr_t, xfs_bmdr_ptr_t;
 #define XFS_BMAP_BROOT_SPACE_CALC(nrecs) \
 	(int)(XFS_BTREE_LBLOCK_LEN + \
 	       ((nrecs) * (sizeof(xfs_bmbt_key_t) + sizeof(xfs_bmbt_ptr_t))))
-
 #define XFS_BMAP_BROOT_SPACE(bb) \
 	(XFS_BMAP_BROOT_SPACE_CALC(be16_to_cpu((bb)->bb_numrecs)))
+
 #define XFS_BMDR_SPACE_CALC(nrecs) \
 	(int)(sizeof(xfs_bmdr_block_t) + \
 	       ((nrecs) * (sizeof(xfs_bmbt_key_t) + sizeof(xfs_bmbt_ptr_t))))
+#define XFS_BMAP_BMDR_SPACE(bb) \
+	(XFS_BMDR_SPACE_CALC(be16_to_cpu((bb)->bb_numrecs)))
 
 /*
  * Maximum number of bmap btree levels.

fs/xfs/xfs_dfrag.c +5 -3

index dd974a5..2707d86 100644
@@ -26,6 +26,9 @@
 #include "xfs_ag.h"
 #include "xfs_mount.h"
 #include "xfs_bmap_btree.h"
+#include "xfs_alloc_btree.h"
+#include "xfs_ialloc_btree.h"
+#include "xfs_btree.h"
 #include "xfs_dinode.h"
 #include "xfs_inode.h"
 #include "xfs_inode_item.h"

@@ -184,7 +187,7 @@ xfs_swap_extents_check_format(
 	 */
 	if (tip->i_d.di_format == XFS_DINODE_FMT_BTREE) {
 		if (XFS_IFORK_BOFF(ip) &&
-		    tip->i_df.if_broot_bytes > XFS_IFORK_BOFF(ip))
+		    XFS_BMAP_BMDR_SPACE(tip->i_df.if_broot) > XFS_IFORK_BOFF(ip))
 			return EINVAL;
 		if (XFS_IFORK_NEXTENTS(tip, XFS_DATA_FORK) <=
 		    XFS_IFORK_MAXEXT(ip, XFS_DATA_FORK))

@@ -194,9 +197,8 @@ xfs_swap_extents_check_format(
 	/* Reciprocal target->temp btree format checks */
 	if (ip->i_d.di_format == XFS_DINODE_FMT_BTREE) {
 		if (XFS_IFORK_BOFF(tip) &&
-		    ip->i_df.if_broot_bytes > XFS_IFORK_BOFF(tip))
+		    XFS_BMAP_BMDR_SPACE(ip->i_df.if_broot) > XFS_IFORK_BOFF(tip))
 			return EINVAL;
-
 		if (XFS_IFORK_NEXTENTS(ip, XFS_DATA_FORK) <=
 		    XFS_IFORK_MAXEXT(tip, XFS_DATA_FORK))
 			return EINVAL;

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Quoted 14 lines · Eric Sandeen

xfs_swap_extents_check_format() contains checks to make sure that original and the temporary files during defrag are compatible; Gabriel VLASIU ran into a case where xfs_fsr returned EINVAL because the tests found the btree root to be of size 120, while the fork offset was only 104; IOW, they overlapped.

However, this is just due to an error in the xfs_swap_extents_check_format() tests, because it is checking the in-memory btree root size against the on-disk fork offset. We should be checking the on-disk sizes in both cases.

This patch adds a new macro to calculate this size, and uses it in the tests.

Quoted 2 lines

With this change, the filesystem image provided by Gabriel allows for proper file degragmentation.

I think this and the followup patch 2/1 got lost.

Ben, any idea?

Quoted 15 lines · Eric Sandeen

Quoted 14 lines · Eric Sandeen

xfs_swap_extents_check_format() contains checks to make sure that original and the temporary files during defrag are compatible; Gabriel VLASIU ran into a case where xfs_fsr returned EINVAL because the tests found the btree root to be of size 120, while the fork offset was only 104; IOW, they overlapped.

However, this is just due to an error in the xfs_swap_extents_check_format() tests, because it is checking the in-memory btree root size against the on-disk fork offset. We should be checking the on-disk sizes in both cases.

This patch adds a new macro to calculate this size, and uses it in the tests.

Quoted 6 lines

Quoted 2 lines

With this change, the filesystem image provided by Gabriel allows for proper file degragmentation.

I think this and the followup patch 2/1 got lost.

Ben, any idea?

Yeah. Sorry Eric.

Quoted 19 lines · Ben Myers

Quoted 18 lines · Eric Sandeen

Quoted 16 lines · Eric Sandeen

xfs_swap_extents_check_format() contains checks to make sure that original and the temporary files during defrag are compatible; Gabriel VLASIU ran into a case where xfs_fsr returned EINVAL because the tests found the btree root to be of size 120, while the fork offset was only 104; IOW, they overlapped.

However, this is just due to an error in the xfs_swap_extents_check_format() tests, because it is checking the in-memory btree root size against the on-disk fork offset. We should be checking the on-disk sizes in both cases.

This patch adds a new macro to calculate this size, and uses it in the tests.

With this change, the filesystem image provided by Gabriel allows for proper file degragmentation.

Quoted 5 lines

Quoted 3 lines

I think this and the followup patch 2/1 got lost.

Ben, any idea?

Yeah. Sorry Eric.

I see the first patch is now merged. Can you please also merge the 2nd patch? It is also reviewed already.