On 05/16/2018 02:23 PM, Dave Chinner wrote:
On Wed, May 16, 2018 at 11:06:15AM -0700, Darrick J. Wong wrote:
On Wed, May 16, 2018 at 10:34:36AM -0700, Allison Henderson wrote:
Ok, with the points Dave made:
Reviewed by: Allison Henderson <allison.henderson@xxxxxxxxxx>
On 05/16/2018 12:56 AM, Dave Chinner wrote:
On Tue, May 15, 2018 at 03:33:58PM -0700, Darrick J. Wong wrote:
From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Add some helpers to assemble a list of fs block extents. Generally,
repair functions will iterate the rmapbt to make a list (1) of all
extents owned by the nominal owner of the metadata structure; then they
will iterate all other structures with the same rmap owner to make a
list (2) of active blocks; and finally we have a subtraction function to
subtract all the blocks in (2) from (1), with the result that (1) is now
a list of blocks that were owned by the old btree and must be disposed.
Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
---
fs/xfs/scrub/repair.c | 207 +++++++++++++++++++++++++++++++++++++++++++++++++
fs/xfs/scrub/repair.h | 31 +++++++
2 files changed, 238 insertions(+)
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 72f04a717150..8e8ecddd7537 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -361,3 +361,210 @@ xfs_repair_init_btblock(
return 0;
}
+
+/* Collect a dead btree extent for later disposal. */
+int
+xfs_repair_collect_btree_extent(
+ struct xfs_scrub_context *sc,
+ struct xfs_repair_extent_list *exlist,
+ xfs_fsblock_t fsbno,
+ xfs_extlen_t len)
+{
+ struct xfs_repair_extent *rex;
+
+ trace_xfs_repair_collect_btree_extent(sc->mp,
+ XFS_FSB_TO_AGNO(sc->mp, fsbno),
+ XFS_FSB_TO_AGBNO(sc->mp, fsbno), len);
+
+ rex = kmem_alloc(sizeof(struct xfs_repair_extent), KM_MAYFAIL);
+ if (!rex)
+ return -ENOMEM;
Is this in transaction context? Regardless, I think we need to run
the entire of scrub/repair in a memalloc_nofs_save() context so we
don't have memory reclaim recursion issues...
[...]
+/* Compare two btree extents. */
+static int
+xfs_repair_btree_extent_cmp(
+ void *priv,
+ struct list_head *a,
+ struct list_head *b)
+{
+ struct xfs_repair_extent *ap;
+ struct xfs_repair_extent *bp;
+
+ ap = container_of(a, struct xfs_repair_extent, list);
+ bp = container_of(b, struct xfs_repair_extent, list);
+
+ if (ap->fsbno > bp->fsbno)
+ return 1;
+ else if (ap->fsbno < bp->fsbno)
+ return -1;
No need for the else there.
Well, I think he meant to return 0 in the case of ap->fsbno == bp->fsbno?
Am i reading that right? caller expects 1 for greater than, -1 for less
than and 0 on equivalence?
Correct. I think Dave was pointing out that else-after-return is
unnecessary, since the following behaves equivalently:
if (a > b)
return 1;
if (a < b)
return -1;
return 0;
Note that gcc (7.3, anyway) generates the same asm for either version so
I assume this is mostly stylistic cleanup to make the comparisons line
up? I don't have a preference either way. :)
It's a style and maintenance thing. The else is redundant and could
lead to future confusion, so IMO it is best to leave it out....
Cheers,
Dave.
Alrighty then, thx for the clarification! :-)
Allison
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
