Proposed patch for xfsprogs

Proposed patch for xfsprogs

[C. Linus Hicks]

During my recent experience with having to reconstruct parts of an XFS
filesystem that got corrupted as a result of several bad blocks, I found
that some of the information displayed using "blockget -v" was pretty
useless, and I am proposing the following code change to introduce a
slight summarization.

Repeating lines of "setting block <foo> to <bar>" and "setting inode to
<foo> for {,rt}block <bar>" will be summarized down to two lines.



--- a/xfsprogs-3.1.1/db/check.c	2010-01-29 14:46:13.000000000 -0500
+++ b/xfsprogs-3.1.1/db/check.c	2010-02-27 17:02:14.111418960 -0500
@@ -1509,6 +1509,7 @@
 {
 	xfs_extlen_t	i;
 	int		mayprint;
+	int		isfirst = 1;
 	char		*p;
 
 	if (!check_range(agno, agbno, len))  {
@@ -1520,10 +1521,15 @@
 	mayprint = verbose | blist_size;
 	for (i = 0, p = &dbmap[agno][agbno]; i < len; i++, p++) {
 		*p = (char)type2;
-		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
+		if (isfirst && mayprint && (verbose || CHECK_BLISTA(agno, agbno + i))) {
 			dbprintf(_("setting block %u/%u to %s\n"), agno, agbno + i,
 				typename[type2]);
+			isfirst = 0;
+		}
 	}
+	if ((len > 1) && mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
+		dbprintf(_("    ... until %u/%u\n"),
+				agno, agbno + len - 1);
 }
 
 static void
@@ -4544,6 +4550,7 @@
 {
 	xfs_extlen_t	i;
 	inodata_t	**idp;
+	int		isfirst = 1;
 	int		mayprint;
 
 	if (!check_inomap(agno, agbno, len, id->ino))
@@ -4551,11 +4558,16 @@
 	mayprint = verbose | id->ilist | blist_size;
 	for (i = 0, idp = &inomap[agno][agbno]; i < len; i++, idp++) {
 		*idp = id;
-		if (mayprint &&
-		    (verbose || id->ilist || CHECK_BLISTA(agno, agbno + i)))
+		if (isfirst && mayprint &&
+		    (verbose || id->ilist || CHECK_BLISTA(agno, agbno + i))) {
 			dbprintf(_("setting inode to %lld for block %u/%u\n"),
 				id->ino, agno, agbno + i);
+			isfirst = 0;
+		}
 	}
+	if ((len > 1) && mayprint && 
+	    (verbose || id->ilist || CHECK_BLISTA(agno, agbno + i)))
+		dbprintf(_("\t\t      ... until %u/%u\n"), agno, agbno + len - 1);
 }
 
 static void
@@ -4575,6 +4587,7 @@
 {
 	xfs_extlen_t	i;
 	inodata_t	**idp;
+	int		isfirst = 1;
 	int		mayprint;
 
 	if (!check_rinomap(bno, len, id->ino))
@@ -4584,10 +4597,16 @@
 	     i < len;
 	     i++, idp++) {
 		*idp = id;
-		if (mayprint && (verbose || id->ilist || CHECK_BLIST(bno + i)))
+		if (isfirst && mayprint &&
+		    (verbose || id->ilist || CHECK_BLIST(bno + i))) {
 			dbprintf(_("setting inode to %lld for rtblock %llu\n"),
 				id->ino, bno + i);
+			isfirst = 0;
+		}
 	}
+	if ((len > 1) && mayprint &&
+	    (verbose || id->ilist || CHECK_BLIST(bno + i)))
+		dbprintf(_("\t\t      ... until %llu\n"), bno + len - 1);
 }
 
 static void


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

Re: Proposed patch for xfsprogs

[Dave Chinner]

On Sat, Feb 27, 2010 at 05:54:10PM -0500, C. Linus Hicks wrote:
> During my recent experience with having to reconstruct parts of an XFS
> filesystem that got corrupted as a result of several bad blocks, I found
> that some of the information displayed using "blockget -v" was pretty
> useless, and I am proposing the following code change to introduce a
> slight summarization.
> 
> Repeating lines of "setting block <foo> to <bar>" and "setting inode to
> <foo> for {,rt}block <bar>" will be summarized down to two lines.

Agreed, that would certainly help reduce the verbosity of the output.
However, I don't think the patch is correct.

> --- a/xfsprogs-3.1.1/db/check.c	2010-01-29 14:46:13.000000000 -0500
> +++ b/xfsprogs-3.1.1/db/check.c	2010-02-27 17:02:14.111418960 -0500
> @@ -1509,6 +1509,7 @@
>  {
>  	xfs_extlen_t	i;
>  	int		mayprint;
> +	int		isfirst = 1;
>  	char		*p;
>  
>  	if (!check_range(agno, agbno, len))  {
> @@ -1520,10 +1521,15 @@
>  	mayprint = verbose | blist_size;
>  	for (i = 0, p = &dbmap[agno][agbno]; i < len; i++, p++) {
>  		*p = (char)type2;
> -		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
> +		if (isfirst && mayprint && (verbose || CHECK_BLISTA(agno, agbno + i))) {
>  			dbprintf(_("setting block %u/%u to %s\n"), agno, agbno + i,
>  				typename[type2]);
> +			isfirst = 0;
> +		}
>  	}
> +	if ((len > 1) && mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
> +		dbprintf(_("    ... until %u/%u\n"),
> +				agno, agbno + len - 1);
>  }

This doesn't take into account that not all blocks in the extent
range might be bad and the change is actually doing something. i.e.
only is CHECK_BLISTA() evaluating as true is there a printout
occurring (unless you specify verbose mode). Hence if only interior
portions are being changed or there are multiple ranges being
changed, this output won't reflect the errors being detected
accurately.

The other sections of the patch have the same issue.

I think that changing it to something like:

	int	last_print == -1;
.....
		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i))) {
			if (i > 0 && last_print == i - 1) {
				last_print = i;
				continue;
			}
			if (last_print != i - 1) {
				dbprintf(_("    ... until %u/%u\n"),
					agno, agbno + last_print);
				last_print == -1;
			}
			if (last_print == -1) {
				dbprintf(_("setting block %u/%u to %s\n"),
					agno, agbno + i, typename[type2]);
				last_print = i;
			}
		}
	}
	if (len > 1 && last_print != -1)
		dbprintf(_("    ... until %u/%u\n"), agno, agbno + last_print);

would do the trick. What do you think?

Cheers,

Dave.



-- 
Dave Chinner
david@fromorbit.com

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

Re: Proposed patch for xfsprogs

[C Linus Hicks]

On Mon, 2010-03-01 at 14:16 +1100, Dave Chinner wrote:
> On Sat, Feb 27, 2010 at 05:54:10PM -0500, C. Linus Hicks wrote:
> > During my recent experience with having to reconstruct parts of an XFS
> > filesystem that got corrupted as a result of several bad blocks, I found
> > that some of the information displayed using "blockget -v" was pretty
> > useless, and I am proposing the following code change to introduce a
> > slight summarization.
> > 
> > Repeating lines of "setting block <foo> to <bar>" and "setting inode to
> > <foo> for {,rt}block <bar>" will be summarized down to two lines.
> 
> Agreed, that would certainly help reduce the verbosity of the output.
> However, I don't think the patch is correct.
> 
> > --- a/xfsprogs-3.1.1/db/check.c	2010-01-29 14:46:13.000000000 -0500
> > +++ b/xfsprogs-3.1.1/db/check.c	2010-02-27 17:02:14.111418960 -0500
> > @@ -1509,6 +1509,7 @@
> >  {
> >  	xfs_extlen_t	i;
> >  	int		mayprint;
> > +	int		isfirst = 1;
> >  	char		*p;
> >  
> >  	if (!check_range(agno, agbno, len))  {
> > @@ -1520,10 +1521,15 @@
> >  	mayprint = verbose | blist_size;
> >  	for (i = 0, p = &dbmap[agno][agbno]; i < len; i++, p++) {
> >  		*p = (char)type2;
> > -		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
> > +		if (isfirst && mayprint && (verbose || CHECK_BLISTA(agno, agbno + i))) {
> >  			dbprintf(_("setting block %u/%u to %s\n"), agno, agbno + i,
> >  				typename[type2]);
> > +			isfirst = 0;
> > +		}
> >  	}
> > +	if ((len > 1) && mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
> > +		dbprintf(_("    ... until %u/%u\n"),
> > +				agno, agbno + len - 1);
> >  }
> 
> This doesn't take into account that not all blocks in the extent
> range might be bad and the change is actually doing something. i.e.
> only is CHECK_BLISTA() evaluating as true is there a printout
> occurring (unless you specify verbose mode). Hence if only interior
> portions are being changed or there are multiple ranges being
> changed, this output won't reflect the errors being detected
> accurately.
> 
> The other sections of the patch have the same issue.
> 
> I think that changing it to something like:
> 
> 	int	last_print == -1;
> .....
> 		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i))) {
> 			if (i > 0 && last_print == i - 1) {
> 				last_print = i;
> 				continue;
> 			}
> 			if (last_print != i - 1) {
> 				dbprintf(_("    ... until %u/%u\n"),
> 					agno, agbno + last_print);
> 				last_print == -1;
> 			}
> 			if (last_print == -1) {
> 				dbprintf(_("setting block %u/%u to %s\n"),
> 					agno, agbno + i, typename[type2]);
> 				last_print = i;
> 			}
> 		}
> 	}
> 	if (len > 1 && last_print != -1)
> 		dbprintf(_("    ... until %u/%u\n"), agno, agbno + last_print);
> 
> would do the trick. What do you think?

You're quite right, I'm glad you looked close enough to spot that. Sorry
I missed it. There's one case your code doesn't handle correctly, where
len > 0 and last_print == -1 and i == len - 1. Let me rework the code
and do some testing then I will re-submit.

Linus


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

Re: Proposed patch for xfsprogs

[C Linus Hicks]

This patch is for reducing the verbosity of the xfs_db "blockget"
command especially when using "-v" by listing only start and end block
numbers for a range, rather than listing all individual blocks. For a
sample 50gb filesystem with around 50% free space and about 20 corrupted
sectors, the logfile size drops from 900mb to 3mb.

Here's my fixed patch:

--- a/xfsprogs-3.1.1/db/check.c	2010-01-29 14:46:13.000000000 -0500
+++ b/xfsprogs-3.1.1/db/check.c	2010-03-01 17:47:01.000000000 -0500
@@ -1509,6 +1509,8 @@
 {
 	xfs_extlen_t	i;
 	int		mayprint;
+	int		pcnt = 0;
+	int		first_prnt = -1;
 	char		*p;
 
 	if (!check_range(agno, agbno, len))  {
@@ -1520,10 +1522,25 @@
 	mayprint = verbose | blist_size;
 	for (i = 0, p = &dbmap[agno][agbno]; i < len; i++, p++) {
 		*p = (char)type2;
-		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i)))
-			dbprintf(_("setting block %u/%u to %s\n"), agno, agbno + i,
-				typename[type2]);
+		if (mayprint && (verbose || CHECK_BLISTA(agno, agbno + i))) {
+			if (first_prnt == -1) {
+				dbprintf(_("setting block %u/%u to %s\n"),
+					 agno, agbno + i, typename[type2]);
+				first_prnt = i;
+			}
+			else if ((first_prnt + pcnt + 1) == i) {
+				pcnt++;
+			}
+			else {
+				dbprintf(_("    ... until %u/%u\n"),
+					agno, agbno + first_prnt + pcnt);
+				first_prnt = -1;
+				pcnt = 0;
+			}
+		}
 	}
+	if (pcnt > 0)
+		dbprintf(_("    ... until %u/%u\n"), agno, agbno + len - 1);
 }
 
 static void
@@ -4544,6 +4561,8 @@
 {
 	xfs_extlen_t	i;
 	inodata_t	**idp;
+	int		pcnt = 0;
+	int		first_prnt = -1;
 	int		mayprint;
 
 	if (!check_inomap(agno, agbno, len, id->ino))
@@ -4552,10 +4571,26 @@
 	for (i = 0, idp = &inomap[agno][agbno]; i < len; i++, idp++) {
 		*idp = id;
 		if (mayprint &&
-		    (verbose || id->ilist || CHECK_BLISTA(agno, agbno + i)))
-			dbprintf(_("setting inode to %lld for block %u/%u\n"),
-				id->ino, agno, agbno + i);
+		    (verbose || id->ilist || CHECK_BLISTA(agno, agbno + i))) {
+			if (first_prnt == -1) {
+				dbprintf(_("setting inode to %#llx for block "
+					   "%u/%u\n"),
+					id->ino, agno, agbno + i);
+				first_prnt = i;
+			}
+			else if ((first_prnt + pcnt + 1) == i) {
+				pcnt++;
+			}
+			else {
+				dbprintf(_("\t\t      ... until %u/%u\n"),
+					agno, agbno + first_prnt + pcnt);
+				first_prnt = -1;
+				pcnt = 0;
+			}
+		}
 	}
+	if (pcnt > 0)
+		dbprintf(_("\t\t      ... until %u/%u\n"), agno, agbno + len - 1);
 }
 
 static void
@@ -4575,6 +4610,8 @@
 {
 	xfs_extlen_t	i;
 	inodata_t	**idp;
+	int		pcnt = 0;
+	int		first_prnt = -1;
 	int		mayprint;
 
 	if (!check_rinomap(bno, len, id->ino))
@@ -4584,10 +4621,26 @@
 	     i < len;
 	     i++, idp++) {
 		*idp = id;
-		if (mayprint && (verbose || id->ilist || CHECK_BLIST(bno + i)))
-			dbprintf(_("setting inode to %lld for rtblock %llu\n"),
+		if (mayprint &&
+		    (verbose || id->ilist || CHECK_BLIST(bno + i))) {
+		    if (first_prnt == -1) {
+			dbprintf(_("setting inode to %#llx for rtblock %llu\n"),
 				id->ino, bno + i);
+			first_prnt = i;
+		    }
+		    else if ((first_prnt + pcnt + 1) == i) {
+			pcnt++;
+		    }
+		    else {
+			dbprintf(_("\t\t      ... until %llu\n"),
+				bno + first_prnt + pcnt);
+			first_prnt = -1;
+			pcnt = 0;
+		    }
+		}
 	}
+	if (pcnt > 0)
+		dbprintf(_("\t\t      ... until %llu\n"), bno + len - 1);
 }
 
 static void


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