| Message ID | 20210128073708.25572-3-ddouwsma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfsprogs: xfs_logprint misc log decoding issues | expand |
On Thu, Jan 28, 2021 at 06:37:08PM +1100, Donald Douwsma wrote: > Back when the way superblocks are logged changed, logprint wasnt > updated updated. Currently logprint displays incorrect accounting Double 'updated'. > information. > > SUPER BLOCK Buffer: > icount: 6360863066640355328 ifree: 262144 fdblks: 0 frext: 0 > > $ printf "0x%x\n" 6360863066640355328 > 0x5846534200001000 > > Part of this decodes as 'XFSB', the xfs superblock magic number and not > the free space accounting. > > Fix this by looking at the entire superblock buffer and using the format > structure as is done for the other allocation group headers. > > Signed-off-by: Donald Douwsma <ddouwsma@redhat.com> > --- > logprint/log_misc.c | 22 +++++++++------------- > logprint/log_print_all.c | 23 ++++++++++------------- > 2 files changed, 19 insertions(+), 26 deletions(-) > > diff --git a/logprint/log_misc.c b/logprint/log_misc.c > index d44e9ff7..929842d0 100644 > --- a/logprint/log_misc.c > +++ b/logprint/log_misc.c > @@ -243,25 +243,21 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) > xlog_print_op_header(head, *i, ptr); > if (super_block) { > printf(_("SUPER BLOCK Buffer: ")); > - if (be32_to_cpu(head->oh_len) < 4*8) { > + if (be32_to_cpu(head->oh_len) < sizeof(struct xfs_sb)) { > printf(_("Out of space\n")); > } else { > - __be64 a, b; > + struct xfs_sb *sb, sb_s; > > printf("\n"); > - /* > - * memmove because *ptr may not be 8-byte aligned > - */ > - memmove(&a, *ptr, sizeof(__be64)); > - memmove(&b, *ptr+8, sizeof(__be64)); > + /* memmove because *ptr may not be 8-byte aligned */ > + sb = &sb_s; > + memmove(sb, *ptr, sizeof(struct xfs_sb)); > printf(_("icount: %llu ifree: %llu "), > - (unsigned long long) be64_to_cpu(a), > - (unsigned long long) be64_to_cpu(b)); > - memmove(&a, *ptr+16, sizeof(__be64)); > - memmove(&b, *ptr+24, sizeof(__be64)); > + be64_to_cpu(sb->sb_icount), > + be64_to_cpu(sb->sb_ifree) ); Extra space at the end ..................................^ > printf(_("fdblks: %llu frext: %llu\n"), > - (unsigned long long) be64_to_cpu(a), > - (unsigned long long) be64_to_cpu(b)); > + be64_to_cpu(sb->sb_fdblocks), > + be64_to_cpu(sb->sb_frextents)); > } > super_block = 0; > } else if (be32_to_cpu(*(__be32 *)(*ptr)) == XFS_AGI_MAGIC) { > diff --git a/logprint/log_print_all.c b/logprint/log_print_all.c > index 2b9e810d..8ff87068 100644 > --- a/logprint/log_print_all.c > +++ b/logprint/log_print_all.c > @@ -91,22 +91,19 @@ xlog_recover_print_buffer( > len = item->ri_buf[i].i_len; > i++; > if (blkno == 0) { /* super block */ > - printf(_(" SUPER Block Buffer:\n")); > + struct xfs_sb *sb = (struct xfs_sb *)p; > + printf(_(" Super Block Buffer: (XFSB)\n")); > if (!print_buffer) > continue; > - printf(_(" icount:%llu ifree:%llu "), > - (unsigned long long) > - be64_to_cpu(*(__be64 *)(p)), > - (unsigned long long) > - be64_to_cpu(*(__be64 *)(p+8))); > - printf(_("fdblks:%llu frext:%llu\n"), > - (unsigned long long) > - be64_to_cpu(*(__be64 *)(p+16)), > - (unsigned long long) > - be64_to_cpu(*(__be64 *)(p+24))); > + printf(_(" icount:%llu ifree:%llu "), > + be64_to_cpu(sb->sb_icount), > + be64_to_cpu(sb->sb_ifree)); > + printf(_("fdblks:%llu frext:%llu\n"), > + be64_to_cpu(sb->sb_fdblocks), > + be64_to_cpu(sb->sb_frextents)); > printf(_(" sunit:%u swidth:%u\n"), > - be32_to_cpu(*(__be32 *)(p+56)), > - be32_to_cpu(*(__be32 *)(p+60))); > + be32_to_cpu(sb->sb_unit), > + be32_to_cpu(sb->sb_width)); /me wonders if these nearly identical decoder routines ought to be refactored into a common helper? Also, what happens if logprint encounters a log from before whenever we changed superblock encoding in the log? When was that, anyway? --D > } else if (be32_to_cpu(*(__be32 *)p) == XFS_AGI_MAGIC) { > int bucket, buckets; > agi = (xfs_agi_t *)p; > -- > 2.27.0 >
On 29/01/2021 04:28, Darrick J. Wong wrote: > On Thu, Jan 28, 2021 at 06:37:08PM +1100, Donald Douwsma wrote: >> Back when the way superblocks are logged changed, logprint wasnt >> updated updated. Currently logprint displays incorrect accounting > > Double 'updated'. Oops > >> information. >> >> SUPER BLOCK Buffer: >> icount: 6360863066640355328 ifree: 262144 fdblks: 0 frext: 0 >> >> $ printf "0x%x\n" 6360863066640355328 >> 0x5846534200001000 >> >> Part of this decodes as 'XFSB', the xfs superblock magic number and not >> the free space accounting. >> >> Fix this by looking at the entire superblock buffer and using the format >> structure as is done for the other allocation group headers. >> >> Signed-off-by: Donald Douwsma <ddouwsma@redhat.com> >> --- >> logprint/log_misc.c | 22 +++++++++------------- >> logprint/log_print_all.c | 23 ++++++++++------------- >> 2 files changed, 19 insertions(+), 26 deletions(-) >> >> diff --git a/logprint/log_misc.c b/logprint/log_misc.c >> index d44e9ff7..929842d0 100644 >> --- a/logprint/log_misc.c >> +++ b/logprint/log_misc.c >> @@ -243,25 +243,21 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) >> xlog_print_op_header(head, *i, ptr); >> if (super_block) { >> printf(_("SUPER BLOCK Buffer: ")); >> - if (be32_to_cpu(head->oh_len) < 4*8) { >> + if (be32_to_cpu(head->oh_len) < sizeof(struct xfs_sb)) { >> printf(_("Out of space\n")); >> } else { >> - __be64 a, b; >> + struct xfs_sb *sb, sb_s; >> >> printf("\n"); >> - /* >> - * memmove because *ptr may not be 8-byte aligned >> - */ >> - memmove(&a, *ptr, sizeof(__be64)); >> - memmove(&b, *ptr+8, sizeof(__be64)); >> + /* memmove because *ptr may not be 8-byte aligned */ >> + sb = &sb_s; >> + memmove(sb, *ptr, sizeof(struct xfs_sb)); >> printf(_("icount: %llu ifree: %llu "), >> - (unsigned long long) be64_to_cpu(a), >> - (unsigned long long) be64_to_cpu(b)); >> - memmove(&a, *ptr+16, sizeof(__be64)); >> - memmove(&b, *ptr+24, sizeof(__be64)); >> + be64_to_cpu(sb->sb_icount), >> + be64_to_cpu(sb->sb_ifree) ); > > Extra space at the end ..................................^ ack > >> printf(_("fdblks: %llu frext: %llu\n"), >> - (unsigned long long) be64_to_cpu(a), >> - (unsigned long long) be64_to_cpu(b)); >> + be64_to_cpu(sb->sb_fdblocks), >> + be64_to_cpu(sb->sb_frextents)); >> } >> super_block = 0; >> } else if (be32_to_cpu(*(__be32 *)(*ptr)) == XFS_AGI_MAGIC) { >> diff --git a/logprint/log_print_all.c b/logprint/log_print_all.c >> index 2b9e810d..8ff87068 100644 >> --- a/logprint/log_print_all.c >> +++ b/logprint/log_print_all.c >> @@ -91,22 +91,19 @@ xlog_recover_print_buffer( >> len = item->ri_buf[i].i_len; >> i++; >> if (blkno == 0) { /* super block */ >> - printf(_(" SUPER Block Buffer:\n")); >> + struct xfs_sb *sb = (struct xfs_sb *)p; >> + printf(_(" Super Block Buffer: (XFSB)\n")); >> if (!print_buffer) >> continue; >> - printf(_(" icount:%llu ifree:%llu "), >> - (unsigned long long) >> - be64_to_cpu(*(__be64 *)(p)), >> - (unsigned long long) >> - be64_to_cpu(*(__be64 *)(p+8))); >> - printf(_("fdblks:%llu frext:%llu\n"), >> - (unsigned long long) >> - be64_to_cpu(*(__be64 *)(p+16)), >> - (unsigned long long) >> - be64_to_cpu(*(__be64 *)(p+24))); >> + printf(_(" icount:%llu ifree:%llu "), >> + be64_to_cpu(sb->sb_icount), >> + be64_to_cpu(sb->sb_ifree)); >> + printf(_("fdblks:%llu frext:%llu\n"), >> + be64_to_cpu(sb->sb_fdblocks), >> + be64_to_cpu(sb->sb_frextents)); >> printf(_(" sunit:%u swidth:%u\n"), >> - be32_to_cpu(*(__be32 *)(p+56)), >> - be32_to_cpu(*(__be32 *)(p+60))); >> + be32_to_cpu(sb->sb_unit), >> + be32_to_cpu(sb->sb_width)); > > /me wonders if these nearly identical decoder routines ought to be > refactored into a common helper? log_print_all.c seems to be coupled to the log recovery code in libxfs when -t is used, but xfs_log_misc.c does its own thing to decode the records outside of recovery. Its a bit messy. > > Also, what happens if logprint encounters a log from before whenever we > changed superblock encoding in the log? When was that, anyway? ... um bad things, it will dump information from further along in the log, possibly beyond the end of the buffer and crash. I _think_ it was when lazy superblock updates went in, tracking that down would be a better way to work out how to handle this, I'll do some research. > > --D > >> } else if (be32_to_cpu(*(__be32 *)p) == XFS_AGI_MAGIC) { >> int bucket, buckets; >> agi = (xfs_agi_t *)p; >> -- >> 2.27.0 >> >
diff --git a/logprint/log_misc.c b/logprint/log_misc.c index d44e9ff7..929842d0 100644 --- a/logprint/log_misc.c +++ b/logprint/log_misc.c @@ -243,25 +243,21 @@ xlog_print_trans_buffer(char **ptr, int len, int *i, int num_ops) xlog_print_op_header(head, *i, ptr); if (super_block) { printf(_("SUPER BLOCK Buffer: ")); - if (be32_to_cpu(head->oh_len) < 4*8) { + if (be32_to_cpu(head->oh_len) < sizeof(struct xfs_sb)) { printf(_("Out of space\n")); } else { - __be64 a, b; + struct xfs_sb *sb, sb_s; printf("\n"); - /* - * memmove because *ptr may not be 8-byte aligned - */ - memmove(&a, *ptr, sizeof(__be64)); - memmove(&b, *ptr+8, sizeof(__be64)); + /* memmove because *ptr may not be 8-byte aligned */ + sb = &sb_s; + memmove(sb, *ptr, sizeof(struct xfs_sb)); printf(_("icount: %llu ifree: %llu "), - (unsigned long long) be64_to_cpu(a), - (unsigned long long) be64_to_cpu(b)); - memmove(&a, *ptr+16, sizeof(__be64)); - memmove(&b, *ptr+24, sizeof(__be64)); + be64_to_cpu(sb->sb_icount), + be64_to_cpu(sb->sb_ifree) ); printf(_("fdblks: %llu frext: %llu\n"), - (unsigned long long) be64_to_cpu(a), - (unsigned long long) be64_to_cpu(b)); + be64_to_cpu(sb->sb_fdblocks), + be64_to_cpu(sb->sb_frextents)); } super_block = 0; } else if (be32_to_cpu(*(__be32 *)(*ptr)) == XFS_AGI_MAGIC) { diff --git a/logprint/log_print_all.c b/logprint/log_print_all.c index 2b9e810d..8ff87068 100644 --- a/logprint/log_print_all.c +++ b/logprint/log_print_all.c @@ -91,22 +91,19 @@ xlog_recover_print_buffer( len = item->ri_buf[i].i_len; i++; if (blkno == 0) { /* super block */ - printf(_(" SUPER Block Buffer:\n")); + struct xfs_sb *sb = (struct xfs_sb *)p; + printf(_(" Super Block Buffer: (XFSB)\n")); if (!print_buffer) continue; - printf(_(" icount:%llu ifree:%llu "), - (unsigned long long) - be64_to_cpu(*(__be64 *)(p)), - (unsigned long long) - be64_to_cpu(*(__be64 *)(p+8))); - printf(_("fdblks:%llu frext:%llu\n"), - (unsigned long long) - be64_to_cpu(*(__be64 *)(p+16)), - (unsigned long long) - be64_to_cpu(*(__be64 *)(p+24))); + printf(_(" icount:%llu ifree:%llu "), + be64_to_cpu(sb->sb_icount), + be64_to_cpu(sb->sb_ifree)); + printf(_("fdblks:%llu frext:%llu\n"), + be64_to_cpu(sb->sb_fdblocks), + be64_to_cpu(sb->sb_frextents)); printf(_(" sunit:%u swidth:%u\n"), - be32_to_cpu(*(__be32 *)(p+56)), - be32_to_cpu(*(__be32 *)(p+60))); + be32_to_cpu(sb->sb_unit), + be32_to_cpu(sb->sb_width)); } else if (be32_to_cpu(*(__be32 *)p) == XFS_AGI_MAGIC) { int bucket, buckets; agi = (xfs_agi_t *)p;
Back when the way superblocks are logged changed, logprint wasnt updated updated. Currently logprint displays incorrect accounting information. SUPER BLOCK Buffer: icount: 6360863066640355328 ifree: 262144 fdblks: 0 frext: 0 $ printf "0x%x\n" 6360863066640355328 0x5846534200001000 Part of this decodes as 'XFSB', the xfs superblock magic number and not the free space accounting. Fix this by looking at the entire superblock buffer and using the format structure as is done for the other allocation group headers. Signed-off-by: Donald Douwsma <ddouwsma@redhat.com> --- logprint/log_misc.c | 22 +++++++++------------- logprint/log_print_all.c | 23 ++++++++++------------- 2 files changed, 19 insertions(+), 26 deletions(-)