* Proposed patch for xfsprogs
@ 2010-02-27 22:54 C. Linus Hicks
2010-03-01 3:16 ` Dave Chinner
0 siblings, 1 reply; 4+ messages in thread
From: C. Linus Hicks @ 2010-02-27 22:54 UTC (permalink / raw)
To: xfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Proposed patch for xfsprogs
2010-02-27 22:54 Proposed patch for xfsprogs C. Linus Hicks
@ 2010-03-01 3:16 ` Dave Chinner
2010-03-01 14:37 ` C Linus Hicks
0 siblings, 1 reply; 4+ messages in thread
From: Dave Chinner @ 2010-03-01 3:16 UTC (permalink / raw)
To: C. Linus Hicks; +Cc: xfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Proposed patch for xfsprogs
2010-03-01 3:16 ` Dave Chinner
@ 2010-03-01 14:37 ` C Linus Hicks
2010-03-02 20:10 ` C Linus Hicks
0 siblings, 1 reply; 4+ messages in thread
From: C Linus Hicks @ 2010-03-01 14:37 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: Proposed patch for xfsprogs
2010-03-01 14:37 ` C Linus Hicks
@ 2010-03-02 20:10 ` C Linus Hicks
0 siblings, 0 replies; 4+ messages in thread
From: C Linus Hicks @ 2010-03-02 20:10 UTC (permalink / raw)
To: xfs
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-02 20:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-27 22:54 Proposed patch for xfsprogs C. Linus Hicks
2010-03-01 3:16 ` Dave Chinner
2010-03-01 14:37 ` C Linus Hicks
2010-03-02 20:10 ` C Linus Hicks
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox