[PATCH 0/3] xfs_repair fixes for fragmented multiblock v2 dirs

[PATCH 0/3] xfs_repair fixes for fragmented multiblock v2 dirs

[Eric Sandeen]

3 fixes for xfs_repair on fragmented multiblock v2 dirs.

xfs_repair: Fix fragmented multiblock dir2 handling in blkmap_getn()
xfs_repair: Document & robustify blkmap_next_off()
xfs_repair: handle fragmented multiblock dir2 in process_leaf_node_dir2()

xfs_check seems to handle things just fine, interstingly enough.

-Eric

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

[PATCH 1/3] xfs_repair: Fix fragmented multiblock dir2 handling in blkmap_getn()

[Eric Sandeen]

blkmap_getn() contains a loop which populates an array of extents
with mapping information for a dir2 "block," which may consist
of multiple filesystem blocks.

As written, the loop re-allocates the array for each new extent,
leaking the previously allocated memory and worse, losing the
previously filled-in extent information.

Fix this by only allocating the array once, for the maximum
possible number of extents - the number of fs blocks in the dir
block.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/repair/bmap.c b/repair/bmap.c
index 2f1c307..c43ca7f 100644
--- a/repair/bmap.c
+++ b/repair/bmap.c
@@ -168,7 +168,8 @@ blkmap_getn(
 		/*
 		 * rare case - multiple extents for a single dir block
 		 */
-		bmp = malloc(nb * sizeof(bmap_ext_t));
+		if (!bmp)
+			bmp = malloc(nb * sizeof(bmap_ext_t));
 		if (!bmp)
 			do_error(_("blkmap_getn malloc failed (%" PRIu64 " bytes)\n"),
 						nb * sizeof(bmap_ext_t));

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

[PATCH 2/3] xfs_repair: Document & robustify blkmap_next_off()

[Eric Sandeen]

blkmap_next_off() was cryptic (to me), so document what it does.
Also catch cases when the passed in extent index 't' is beyond
the number of extents in the blkmap, so that:

        ext = blkmap->exts + *t;

doesn't walk off the end of the array into garbage.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/repair/bmap.c b/repair/bmap.c
index c43ca7f..85d66dc 100644
--- a/repair/bmap.c
+++ b/repair/bmap.c
@@ -206,8 +206,25 @@ blkmap_last_off(
 	return ext->startoff + ext->blockcount;
 }
 
-/*
- * Return the next offset in a block map.
+/**
+ * blkmap_next_off - Return next logical block offset in a block map.
+ * @blkmap:	blockmap to use
+ * @o:		current file logical block number
+ * @t:		current extent index into blockmap (in/out)
+ *
+ * Given a logical block offset in a file, return the next mapped logical offset
+ * The map index "t" tracks the current extent number in the block map, and
+ * is updated automatically if the returned offset resides within the next
+ * mapped extent.
+ *
+ * If the blockmap contains no extents, or no more logical offsets are mapped,
+ * or the extent index exceeds the number of extents in the map,
+ * return NULLDFILOFF.
+ *
+ * If offset o is beyond extent index t, the first offset in the next extent
+ * after extent t will be returned.
+ *
+ * Intended to be called starting with offset 0, index 0, and iterated.
  */
 xfs_dfiloff_t
 blkmap_next_off(
@@ -223,10 +240,12 @@ blkmap_next_off(
 		*t = 0;
 		return blkmap->exts[0].startoff;
 	}
+	if (*t >= blkmap->nexts)
+		return NULLDFILOFF;
 	ext = blkmap->exts + *t;
 	if (o < ext->startoff + ext->blockcount - 1)
 		return o + 1;
-	if (*t >= blkmap->nexts - 1)
+	if (*t == blkmap->nexts - 1)
 		return NULLDFILOFF;
 	(*t)++;
 	return ext[1].startoff;


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

[PATCH 3/3] xfs_repair: handle fragmented multiblock dir2 in process_leaf_node_dir2()

[Eric Sandeen]

process_leaf_node_dir2() had the following loop:

	while ((dbno = blkmap_next_off(blkmap, ndbno, &t)) < mp->m_dirleafblk) {
		...
		ndbno = dbno + mp->m_dirblkfsbs - 1;
		...
	}

which does not account for fragmented multiblock dir2.

ndbno was blindly being advanced by m_dirblkfsbs, but then
blkmap_next_off() would return the logical block of the next
mapped extent in blkmap, which may be within the current
(fragmented) dir2 multi-block, not the next multi-block,
because the extent index t hadn't been advanced.

Fix this by calling blkmap_next_off() until ndbno has advanced
into the next multiblock dir2 block, thereby keeping
the extent index t straight while properly advancing
ndbno.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/repair/dir2.c b/repair/dir2.c
index f9562d7..7a614a8 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -2003,7 +2003,11 @@ process_leaf_node_dir2(
 	ndbno = NULLDFILOFF;
 	while ((dbno = blkmap_next_off(blkmap, ndbno, &t)) < mp->m_dirleafblk) {
 		nex = blkmap_getn(blkmap, dbno, mp->m_dirblkfsbs, &bmp, &lbmp);
-		ndbno = dbno + mp->m_dirblkfsbs - 1;
+		/* Advance through map to last dfs block in this dir block */
+		ndbno = dbno;
+		while (ndbno < dbno + mp->m_dirblkfsbs - 1) {
+			ndbno = blkmap_next_off(blkmap, ndbno, &t);
+		}
 		if (nex == 0) {
 			do_warn(
 _("block %" PRIu64 " for directory inode %" PRIu64 " is missing\n"),


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

Re: [PATCH 0/3] xfs_repair fixes for fragmented multiblock v2 dirs

[Eric Sandeen]

On 5/21/12 5:07 PM, Eric Sandeen wrote:
> 3 fixes for xfs_repair on fragmented multiblock v2 dirs.
> 
> xfs_repair: Fix fragmented multiblock dir2 handling in blkmap_getn()
> xfs_repair: Document & robustify blkmap_next_off()
> xfs_repair: handle fragmented multiblock dir2 in process_leaf_node_dir2()
> 
> xfs_check seems to handle things just fine, interstingly enough.
> 
> -Eric

Grr.

Well, I did send the other two.

but it seems to be eaten/stuck/constipated.  Again.

Grr.

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

[PATCH 2/3 RESEND] xfs_repair fixes for fragmented multiblock v2 dirs

[Eric Sandeen]

blkmap_next_off() was cryptic (to me), so document what it does.
Also catch cases when the passed in extent index 't' is beyond
the number of extents in the blkmap, so that:

        ext = blkmap->exts + *t;

doesn't walk off the end of the array into garbage.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Resending, last one never made it through the mailing list gauntlet...

diff --git a/repair/bmap.c b/repair/bmap.c
index c43ca7f..85d66dc 100644
--- a/repair/bmap.c
+++ b/repair/bmap.c
@@ -206,8 +206,25 @@ blkmap_last_off(
 	return ext->startoff + ext->blockcount;
 }
 
-/*
- * Return the next offset in a block map.
+/**
+ * blkmap_next_off - Return next logical block offset in a block map.
+ * @blkmap:	blockmap to use
+ * @o:		current file logical block number
+ * @t:		current extent index into blockmap (in/out)
+ *
+ * Given a logical block offset in a file, return the next mapped logical offset
+ * The map index "t" tracks the current extent number in the block map, and
+ * is updated automatically if the returned offset resides within the next
+ * mapped extent.
+ *
+ * If the blockmap contains no extents, or no more logical offsets are mapped,
+ * or the extent index exceeds the number of extents in the map,
+ * return NULLDFILOFF.
+ *
+ * If offset o is beyond extent index t, the first offset in the next extent
+ * after extent t will be returned.
+ *
+ * Intended to be called starting with offset 0, index 0, and iterated.
  */
 xfs_dfiloff_t
 blkmap_next_off(
@@ -223,10 +240,12 @@ blkmap_next_off(
 		*t = 0;
 		return blkmap->exts[0].startoff;
 	}
+	if (*t >= blkmap->nexts)
+		return NULLDFILOFF;
 	ext = blkmap->exts + *t;
 	if (o < ext->startoff + ext->blockcount - 1)
 		return o + 1;
-	if (*t >= blkmap->nexts - 1)
+	if (*t == blkmap->nexts - 1)
 		return NULLDFILOFF;
 	(*t)++;
 	return ext[1].startoff;




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

[PATCH 3/3 RESEND] xfs_repair: handle fragmented multiblock dir2 in process_leaf_node_dir2()

[Eric Sandeen]

process_leaf_node_dir2() had the following loop:

	while ((dbno = blkmap_next_off(blkmap, ndbno, &t)) < mp->m_dirleafblk) {
		...
		ndbno = dbno + mp->m_dirblkfsbs - 1;
		...
	}

which does not account for fragmented multiblock dir2.

ndbno was blindly being advanced by m_dirblkfsbs, but then
blkmap_next_off() would return the logical block of the next
mapped extent in blkmap, which may be within the current
(fragmented) dir2 multi-block, not the next multi-block,
because the extent index t hadn't been advanced.

Fix this by calling blkmap_next_off() until ndbno has advanced
into the next multiblock dir2 block, thereby keeping
the extent index t straight while properly advancing
ndbno.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Resending, first one didn't make it through the mailing list gauntlet

diff --git a/repair/dir2.c b/repair/dir2.c
index f9562d7..7a614a8 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -2003,7 +2003,11 @@ process_leaf_node_dir2(
 	ndbno = NULLDFILOFF;
 	while ((dbno = blkmap_next_off(blkmap, ndbno, &t)) < mp->m_dirleafblk) {
 		nex = blkmap_getn(blkmap, dbno, mp->m_dirblkfsbs, &bmp, &lbmp);
-		ndbno = dbno + mp->m_dirblkfsbs - 1;
+		/* Advance through map to last dfs block in this dir block */
+		ndbno = dbno;
+		while (ndbno < dbno + mp->m_dirblkfsbs - 1) {
+			ndbno = blkmap_next_off(blkmap, ndbno, &t);
+		}
 		if (nex == 0) {
 			do_warn(
 _("block %" PRIu64 " for directory inode %" PRIu64 " is missing\n"),




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

Re: [PATCH 1/3] xfs_repair: Fix fragmented multiblock dir2 handling in blkmap_getn()

[Ben Myers]

Hey Eric,

On Mon, May 21, 2012 at 05:10:06PM -0500, Eric Sandeen wrote:
> blkmap_getn() contains a loop which populates an array of extents
> with mapping information for a dir2 "block," which may consist
> of multiple filesystem blocks.
> 
> As written, the loop re-allocates the array for each new extent,
> leaking the previously allocated memory and worse, losing the
> previously filled-in extent information.
> 
> Fix this by only allocating the array once, for the maximum
> possible number of extents - the number of fs blocks in the dir
> block.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/repair/bmap.c b/repair/bmap.c
> index 2f1c307..c43ca7f 100644
> --- a/repair/bmap.c
> +++ b/repair/bmap.c
> @@ -168,7 +168,8 @@ blkmap_getn(
>  		/*
>  		 * rare case - multiple extents for a single dir block
>  		 */
> -		bmp = malloc(nb * sizeof(bmap_ext_t));
> +		if (!bmp)
> +			bmp = malloc(nb * sizeof(bmap_ext_t));

Cool.  That's an area where we don't have much test coverage.  I don't see a
single '-n size' in xfstests.

This looks good to me.  FWICS all the callers free it later.

Reviewed-by: Ben Myers <bpm@sgi.com>

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

Re: [PATCH 2/3] xfs_repair: Document & robustify blkmap_next_off()

[Ben Myers]

On Mon, May 21, 2012 at 05:10:50PM -0500, Eric Sandeen wrote:
> blkmap_next_off() was cryptic (to me), so document what it does.
> Also catch cases when the passed in extent index 't' is beyond
> the number of extents in the blkmap, so that:
> 
>         ext = blkmap->exts + *t;
> 
> doesn't walk off the end of the array into garbage.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/repair/bmap.c b/repair/bmap.c
> index c43ca7f..85d66dc 100644
> --- a/repair/bmap.c
> +++ b/repair/bmap.c
> @@ -206,8 +206,25 @@ blkmap_last_off(
>  	return ext->startoff + ext->blockcount;
>  }
>  
> -/*
> - * Return the next offset in a block map.
> +/**
> + * blkmap_next_off - Return next logical block offset in a block map.
> + * @blkmap:	blockmap to use
> + * @o:		current file logical block number
> + * @t:		current extent index into blockmap (in/out)
> + *
> + * Given a logical block offset in a file, return the next mapped logical offset
> + * The map index "t" tracks the current extent number in the block map, and
> + * is updated automatically if the returned offset resides within the next
> + * mapped extent.
> + *
> + * If the blockmap contains no extents, or no more logical offsets are mapped,
> + * or the extent index exceeds the number of extents in the map,
> + * return NULLDFILOFF.
> + *
> + * If offset o is beyond extent index t, the first offset in the next extent
> + * after extent t will be returned.
> + *
> + * Intended to be called starting with offset 0, index 0, and iterated.
>   */
>  xfs_dfiloff_t
>  blkmap_next_off(
> @@ -223,10 +240,12 @@ blkmap_next_off(
>  		*t = 0;
>  		return blkmap->exts[0].startoff;
>  	}
> +	if (*t >= blkmap->nexts)
> +		return NULLDFILOFF;
>  	ext = blkmap->exts + *t;
>  	if (o < ext->startoff + ext->blockcount - 1)
>  		return o + 1;
> -	if (*t >= blkmap->nexts - 1)
> +	if (*t == blkmap->nexts - 1)
>  		return NULLDFILOFF;
>  	(*t)++;
>  	return ext[1].startoff;

Ok, so you have 50 extents, and when *t == 49 you don't want to go looking at
ext[50].startoff.  Looks good to me.

Reviewed-by: Ben Myers <bpm@sgi.com>

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

Re: [PATCH 3/3] xfs_repair: handle fragmented multiblock dir2 in process_leaf_node_dir2()

[Ben Myers]

On Mon, May 21, 2012 at 05:11:45PM -0500, Eric Sandeen wrote:
> process_leaf_node_dir2() had the following loop:
> 
> 	while ((dbno = blkmap_next_off(blkmap, ndbno, &t)) < mp->m_dirleafblk) {
> 		...
> 		ndbno = dbno + mp->m_dirblkfsbs - 1;
> 		...
> 	}
> 
> which does not account for fragmented multiblock dir2.
> 
> ndbno was blindly being advanced by m_dirblkfsbs, but then
> blkmap_next_off() would return the logical block of the next
> mapped extent in blkmap, which may be within the current
> (fragmented) dir2 multi-block, not the next multi-block,
> because the extent index t hadn't been advanced.
> 
> Fix this by calling blkmap_next_off() until ndbno has advanced
> into the next multiblock dir2 block, thereby keeping
> the extent index t straight while properly advancing
> ndbno.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/repair/dir2.c b/repair/dir2.c
> index f9562d7..7a614a8 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -2003,7 +2003,11 @@ process_leaf_node_dir2(
>  	ndbno = NULLDFILOFF;
>  	while ((dbno = blkmap_next_off(blkmap, ndbno, &t)) < mp->m_dirleafblk) {
>  		nex = blkmap_getn(blkmap, dbno, mp->m_dirblkfsbs, &bmp, &lbmp);
> -		ndbno = dbno + mp->m_dirblkfsbs - 1;
> +		/* Advance through map to last dfs block in this dir block */
> +		ndbno = dbno;
> +		while (ndbno < dbno + mp->m_dirblkfsbs - 1) {
> +			ndbno = blkmap_next_off(blkmap, ndbno, &t);
> +		}

Yep, looks good to me.  An important thing to notice is that blkmap_getn
handles all extents in a dir2 multiblock... 

Reviewed-by: Ben Myers <bpm@sgi.com>

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