| Message ID | 20170929013702.17814-3-quwenruo.btrfs@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29.09.2017 04:36, Qu Wenruo wrote: > Use inline function to replace macro since we don't need > stringification. > (Macro still exist until all caller get updated) > > And add more info about the error. > > For nr_items error, report if it's too large or too small, and output > valid value range. > > For blk pointer, added a new alignment checker. > > For key order, also output the next key to make the problem more > obvious. > > Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com> > --- > fs/btrfs/tree-checker.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 61 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 301243a69dea..a51f2503acc4 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -37,6 +37,48 @@ > btrfs_header_level(eb) == 0 ? "leaf" : "node", \ > reason, btrfs_header_bytenr(eb), root->objectid, slot) > > +/* > + * Error message should follow the format below: > + * corrupt <type>: <identifier>, <reason>[, <bad_value>] > + * > + * @type: Either leaf or node > + * @identifier: The necessary info to locate the leaf/node. > + * It's recommened to decode key.objecitd/offset if it's > + * meaningful. > + * @reason: What's wrong > + * @bad_value: Optional, it's recommened to output bad value and its > + * expected value (range). > + * > + * Since comma is used to separate the components, only SPACE is allowed > + * inside each component. > + */ > + > +/* > + * Append the generic "corrupt leaf/node root=%llu block=%llu slot=%d: " to > + * @fmt. > + * Allowing user to customize their output. > + */ > +__printf(4, 5) > +static void generic_err(const struct btrfs_root *root, > + const struct extent_buffer *eb, > + int slot, const char *fmt, ...) > +{ > + struct va_format vaf; > + va_list args; > + > + va_start(args, fmt); > + > + vaf.fmt = fmt; > + vaf.va = &args; > + > + btrfs_crit(root->fs_info, > + "corrupt %s: root=%llu block=%llu slot=%d, %pV", > + btrfs_header_level(eb) == 0 ? "leaf" : "node", > + root->objectid, btrfs_header_bytenr(eb), slot, > + &vaf); > + va_end(args); > +} > + > static int check_extent_data_item(struct btrfs_root *root, > struct extent_buffer *leaf, > struct btrfs_key *key, int slot) > @@ -282,8 +324,10 @@ int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) > > if (nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(root->fs_info)) { > btrfs_crit(root->fs_info, > - "corrupt node: block %llu root %llu nritems %lu", > - node->start, root->objectid, nr); > + "corrupt node: root=%llu block=%llu, nritems too %s, have %lu expect range [1,%u]", > + root->objectid, node->start, > + nr == 0 ? "small" : "large", nr, > + BTRFS_NODEPTRS_PER_BLOCK(root->fs_info)); > return -EIO; This is separate from this patch but : Why not EUCLEAN, could we get this error because of corrupted data and not necessarily EIO ? Your other patches consistently use EUCLEAN ? > } > > @@ -293,13 +337,26 @@ int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) > btrfs_node_key_to_cpu(node, &next_key, slot + 1); > > if (!bytenr) { > - CORRUPT("invalid item slot", node, root, slot); > + generic_err(root, node, slot, > + "invalid node pointer, have %llu shouldn't be 0", > + bytenr); nit: Perhaps just say "Invalid null node pointer", if we trigger this assert it means bytenr is 0 so I see no reason why we should be doing any special formatting. It's not a big deal so might not be worth it a resend unless there are other comments. > ret = -EIO; Ditto w.r.t EIO ? > goto out; > } > + if (!IS_ALIGNED(bytenr, root->fs_info->sectorsize)) { > + generic_err(root, node, slot, > + "unaligned pointer, have %llu should be aligned to %u", > + bytenr, root->fs_info->sectorsize); > + ret = -EUCLEAN; > + goto out; > + } > > if (btrfs_comp_cpu_keys(&key, &next_key) >= 0) { > - CORRUPT("bad key order", node, root, slot); > + generic_err(root, node, slot, > + "bad key order, current key (%llu %u %llu) next key (%llu %u %llu)", > + key.objectid, key.type, key.offset, > + next_key.objectid, next_key.type, > + next_key.offset); > ret = -EIO; Ditto w.r.t return code? > goto out; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017年09月29日 14:05, Nikolay Borisov wrote: > > > On 29.09.2017 04:36, Qu Wenruo wrote: >> Use inline function to replace macro since we don't need >> stringification. >> (Macro still exist until all caller get updated) >> >> And add more info about the error. >> >> For nr_items error, report if it's too large or too small, and output >> valid value range. >> >> For blk pointer, added a new alignment checker. >> >> For key order, also output the next key to make the problem more >> obvious. >> >> Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com> >> --- >> fs/btrfs/tree-checker.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 61 insertions(+), 4 deletions(-) >> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >> index 301243a69dea..a51f2503acc4 100644 >> --- a/fs/btrfs/tree-checker.c >> +++ b/fs/btrfs/tree-checker.c >> @@ -37,6 +37,48 @@ >> btrfs_header_level(eb) == 0 ? "leaf" : "node", \ >> reason, btrfs_header_bytenr(eb), root->objectid, slot) >> >> +/* >> + * Error message should follow the format below: >> + * corrupt <type>: <identifier>, <reason>[, <bad_value>] >> + * >> + * @type: Either leaf or node >> + * @identifier: The necessary info to locate the leaf/node. >> + * It's recommened to decode key.objecitd/offset if it's >> + * meaningful. >> + * @reason: What's wrong >> + * @bad_value: Optional, it's recommened to output bad value and its >> + * expected value (range). >> + * >> + * Since comma is used to separate the components, only SPACE is allowed >> + * inside each component. >> + */ >> + >> +/* >> + * Append the generic "corrupt leaf/node root=%llu block=%llu slot=%d: " to >> + * @fmt. >> + * Allowing user to customize their output. >> + */ >> +__printf(4, 5) >> +static void generic_err(const struct btrfs_root *root, >> + const struct extent_buffer *eb, >> + int slot, const char *fmt, ...) >> +{ >> + struct va_format vaf; >> + va_list args; >> + >> + va_start(args, fmt); >> + >> + vaf.fmt = fmt; >> + vaf.va = &args; >> + >> + btrfs_crit(root->fs_info, >> + "corrupt %s: root=%llu block=%llu slot=%d, %pV", >> + btrfs_header_level(eb) == 0 ? "leaf" : "node", >> + root->objectid, btrfs_header_bytenr(eb), slot, >> + &vaf); >> + va_end(args); >> +} >> + >> static int check_extent_data_item(struct btrfs_root *root, >> struct extent_buffer *leaf, >> struct btrfs_key *key, int slot) >> @@ -282,8 +324,10 @@ int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) >> >> if (nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(root->fs_info)) { >> btrfs_crit(root->fs_info, >> - "corrupt node: block %llu root %llu nritems %lu", >> - node->start, root->objectid, nr); >> + "corrupt node: root=%llu block=%llu, nritems too %s, have %lu expect range [1,%u]", >> + root->objectid, node->start, >> + nr == 0 ? "small" : "large", nr, >> + BTRFS_NODEPTRS_PER_BLOCK(root->fs_info)); >> return -EIO; > > This is separate from this patch but : > > Why not EUCLEAN, could we get this error because of corrupted data and > not necessarily EIO ? Your other patches consistently use EUCLEAN ? Just forgot that. Old code I didn't modify, but since it's moved to new place, EUCLEAN makes sense. I'll update the patchset (if there is any). Thanks for pointing this out, Qu > >> } >> >> @@ -293,13 +337,26 @@ int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) >> btrfs_node_key_to_cpu(node, &next_key, slot + 1); >> >> if (!bytenr) { >> - CORRUPT("invalid item slot", node, root, slot); >> + generic_err(root, node, slot, >> + "invalid node pointer, have %llu shouldn't be 0", >> + bytenr); > > nit: Perhaps just say "Invalid null node pointer", if we trigger this > assert it means bytenr is 0 so I see no reason why we should be doing > any special formatting. It's not a big deal so might not be worth it a > resend unless there are other comments. > >> ret = -EIO; > > Ditto w.r.t EIO ? > >> goto out; >> } >> + if (!IS_ALIGNED(bytenr, root->fs_info->sectorsize)) { >> + generic_err(root, node, slot, >> + "unaligned pointer, have %llu should be aligned to %u", >> + bytenr, root->fs_info->sectorsize); >> + ret = -EUCLEAN; >> + goto out; >> + } >> >> if (btrfs_comp_cpu_keys(&key, &next_key) >= 0) { >> - CORRUPT("bad key order", node, root, slot); >> + generic_err(root, node, slot, >> + "bad key order, current key (%llu %u %llu) next key (%llu %u %llu)", >> + key.objectid, key.type, key.offset, >> + next_key.objectid, next_key.type, >> + next_key.offset); >> ret = -EIO; > > Ditto w.r.t return code? > >> goto out; >> } >> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 301243a69dea..a51f2503acc4 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -37,6 +37,48 @@ btrfs_header_level(eb) == 0 ? "leaf" : "node", \ reason, btrfs_header_bytenr(eb), root->objectid, slot) +/* + * Error message should follow the format below: + * corrupt <type>: <identifier>, <reason>[, <bad_value>] + * + * @type: Either leaf or node + * @identifier: The necessary info to locate the leaf/node. + * It's recommened to decode key.objecitd/offset if it's + * meaningful. + * @reason: What's wrong + * @bad_value: Optional, it's recommened to output bad value and its + * expected value (range). + * + * Since comma is used to separate the components, only SPACE is allowed + * inside each component. + */ + +/* + * Append the generic "corrupt leaf/node root=%llu block=%llu slot=%d: " to + * @fmt. + * Allowing user to customize their output. + */ +__printf(4, 5) +static void generic_err(const struct btrfs_root *root, + const struct extent_buffer *eb, + int slot, const char *fmt, ...) +{ + struct va_format vaf; + va_list args; + + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = &args; + + btrfs_crit(root->fs_info, + "corrupt %s: root=%llu block=%llu slot=%d, %pV", + btrfs_header_level(eb) == 0 ? "leaf" : "node", + root->objectid, btrfs_header_bytenr(eb), slot, + &vaf); + va_end(args); +} + static int check_extent_data_item(struct btrfs_root *root, struct extent_buffer *leaf, struct btrfs_key *key, int slot) @@ -282,8 +324,10 @@ int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) if (nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(root->fs_info)) { btrfs_crit(root->fs_info, - "corrupt node: block %llu root %llu nritems %lu", - node->start, root->objectid, nr); + "corrupt node: root=%llu block=%llu, nritems too %s, have %lu expect range [1,%u]", + root->objectid, node->start, + nr == 0 ? "small" : "large", nr, + BTRFS_NODEPTRS_PER_BLOCK(root->fs_info)); return -EIO; } @@ -293,13 +337,26 @@ int btrfs_check_node(struct btrfs_root *root, struct extent_buffer *node) btrfs_node_key_to_cpu(node, &next_key, slot + 1); if (!bytenr) { - CORRUPT("invalid item slot", node, root, slot); + generic_err(root, node, slot, + "invalid node pointer, have %llu shouldn't be 0", + bytenr); ret = -EIO; goto out; } + if (!IS_ALIGNED(bytenr, root->fs_info->sectorsize)) { + generic_err(root, node, slot, + "unaligned pointer, have %llu should be aligned to %u", + bytenr, root->fs_info->sectorsize); + ret = -EUCLEAN; + goto out; + } if (btrfs_comp_cpu_keys(&key, &next_key) >= 0) { - CORRUPT("bad key order", node, root, slot); + generic_err(root, node, slot, + "bad key order, current key (%llu %u %llu) next key (%llu %u %llu)", + key.objectid, key.type, key.offset, + next_key.objectid, next_key.type, + next_key.offset); ret = -EIO; goto out; }
Use inline function to replace macro since we don't need stringification. (Macro still exist until all caller get updated) And add more info about the error. For nr_items error, report if it's too large or too small, and output valid value range. For blk pointer, added a new alignment checker. For key order, also output the next key to make the problem more obvious. Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com> --- fs/btrfs/tree-checker.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 61 insertions(+), 4 deletions(-)