Re: [PATCH][v3] Btrfs: add a extent ref verify tool
On Fri, Sep 01, 2017 at 03:09:30AM -0400, jo...@toxicpanda.com wrote:
> From: Josef Bacik
>
> We were having corruption issues that were tied back to problems with the
> extent
> tree. In order to track them down I built this tool to try and find the
> culprit, which was pretty successful. If you compile with this tool on it
> will
> live verify every ref update that the fs makes and make sure it is consistent
> and valid. I've run this through with xfstests and haven't gotten any false
> positives. Thanks,
>
> Signed-off-by: Josef Bacik
> ---
> v2->v3:
> - fix use after free in an error case
Can you please split the patch into more parts? This is just too big.
* the parameter changes, one per patch
* add ref-veriy.*
* new mount option and enabling the ref-verify
Apart from the specific comments written inline, here's list of thing
that I saw repeatd in several places:
printk instead of the btrfs_* error helpers - the bare printk will not
tell youw which filesystem is affected so it's not helpful when there
are several btrfs filesytems active
please don't split long strings
please don't use %Lu or %Ld format string, %llu
GFP_NOFS -- it's used on the open_ctree path so GFP_KERNEL is the right
and safe flag
misc small coding style issues
I'm half way through reviewing it from the functional side, so far it
looks good.
> fs/btrfs/Kconfig | 10 +
> fs/btrfs/Makefile |1 +
> fs/btrfs/ctree.c |2 +-
> fs/btrfs/ctree.h | 14 +-
> fs/btrfs/disk-io.c | 15 +
> fs/btrfs/extent-tree.c | 44 ++-
> fs/btrfs/file.c| 10 +-
> fs/btrfs/inode.c |9 +-
> fs/btrfs/ioctl.c |2 +-
> fs/btrfs/ref-verify.c | 1019
>
> fs/btrfs/ref-verify.h | 51 +++
> fs/btrfs/relocation.c | 14 +-
> fs/btrfs/super.c | 17 +
> fs/btrfs/tree-log.c|2 +-
> 14 files changed, 1178 insertions(+), 32 deletions(-)
> create mode 100644 fs/btrfs/ref-verify.c
> create mode 100644 fs/btrfs/ref-verify.h
>
> diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
> index 80e9c18..77d7f74 100644
> --- a/fs/btrfs/Kconfig
> +++ b/fs/btrfs/Kconfig
> @@ -89,3 +89,13 @@ config BTRFS_ASSERT
> any of the assertions trip. This is meant for btrfs developers only.
>
> If unsure, say N.
> +
> +config BTRFS_FS_REF_VERIFY
> + bool "Btrfs with the ref verify tool compiled in"
> + depends on BTRFS_FS
must be N by default
> + help
> + Enable run-time extent reference verification instrumentation. This
> + is meant to be used by btrfs developers for tracking down extent
> + reference problems or verifying they didn't break something.
> +
> + If unsure, say N.
> diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
> index 128ce17..3172751 100644
> --- a/fs/btrfs/Makefile
> +++ b/fs/btrfs/Makefile
> @@ -13,6 +13,7 @@ btrfs-y += super.o ctree.o extent-tree.o print-tree.o
> root-tree.o dir-item.o \
>
> btrfs-$(CONFIG_BTRFS_FS_POSIX_ACL) += acl.o
> btrfs-$(CONFIG_BTRFS_FS_CHECK_INTEGRITY) += check-integrity.o
> +btrfs-$(CONFIG_BTRFS_FS_REF_VERIFY) += ref-verify.o
>
> btrfs-$(CONFIG_BTRFS_FS_RUN_SANITY_TESTS) += tests/free-space-tests.o \
> tests/extent-buffer-tests.o tests/btrfs-tests.o \
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 6d49db7..a4812ce 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -192,7 +192,7 @@ struct extent_buffer *btrfs_lock_root_node(struct
> btrfs_root *root)
> * tree until you end up with a lock on the root. A locked buffer
> * is returned, with a reference held.
> */
> -static struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root
> *root)
> +struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root)
> {
> struct extent_buffer *eb;
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index d49b045..4fa3ddd 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1098,6 +1098,12 @@ struct btrfs_fs_info {
> u32 nodesize;
> u32 sectorsize;
> u32 stripesize;
> +
> +#ifdef CONFIG_BTRFS_FS_REF_VERIFY
> + spinlock_t ref_verify_lock;
> + struct rb_root block_tree;
> + bool ref_verify_enabled;
the on/off bit can be added to the fs_info::flags instead
> +#endif
> };
>
> static inline struct btrfs_fs_info *btrfs_sb(struct super_block *sb)
> @@ -1336,6 +1342,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct
> btrfs_fs_info *info)
> #define BTRFS_MOUNT_FRAGMENT_METADATA(1 << 25)
> #define BTRFS_MOUNT_FREE_SPACE_TREE (1 << 26)
> #define BTRFS_MOUNT_NOLOGREPLAY (1 << 27)
> +#define BTRFS_MOUNT_REF_VERIFY (1 << 28)
>
> #define BTRFS_DEFAULT_COMMIT_INTERVAL(30)
> #define BTRFS_DEFAULT_MAX_INLINE (2048)
> @@ -2627,7 +2634,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle
> *trans,
> struct extent_buffer *buf,
>
[PATCH][v3] Btrfs: add a extent ref verify tool
From: Josef Bacik
We were having corruption issues that were tied back to problems with the extent
tree. In order to track them down I built this tool to try and find the
culprit, which was pretty successful. If you compile with this tool on it will
live verify every ref update that the fs makes and make sure it is consistent
and valid. I've run this through with xfstests and haven't gotten any false
positives. Thanks,
Signed-off-by: Josef Bacik
---
v2->v3:
- fix use after free in an error case
fs/btrfs/Kconfig | 10 +
fs/btrfs/Makefile |1 +
fs/btrfs/ctree.c |2 +-
fs/btrfs/ctree.h | 14 +-
fs/btrfs/disk-io.c | 15 +
fs/btrfs/extent-tree.c | 44 ++-
fs/btrfs/file.c| 10 +-
fs/btrfs/inode.c |9 +-
fs/btrfs/ioctl.c |2 +-
fs/btrfs/ref-verify.c | 1019
fs/btrfs/ref-verify.h | 51 +++
fs/btrfs/relocation.c | 14 +-
fs/btrfs/super.c | 17 +
fs/btrfs/tree-log.c|2 +-
14 files changed, 1178 insertions(+), 32 deletions(-)
create mode 100644 fs/btrfs/ref-verify.c
create mode 100644 fs/btrfs/ref-verify.h
diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
index 80e9c18..77d7f74 100644
--- a/fs/btrfs/Kconfig
+++ b/fs/btrfs/Kconfig
@@ -89,3 +89,13 @@ config BTRFS_ASSERT
any of the assertions trip. This is meant for btrfs developers only.
If unsure, say N.
+
+config BTRFS_FS_REF_VERIFY
+ bool "Btrfs with the ref verify tool compiled in"
+ depends on BTRFS_FS
+ help
+ Enable run-time extent reference verification instrumentation. This
+ is meant to be used by btrfs developers for tracking down extent
+ reference problems or verifying they didn't break something.
+
+ If unsure, say N.
diff --git a/fs/btrfs/Makefile b/fs/btrfs/Makefile
index 128ce17..3172751 100644
--- a/fs/btrfs/Makefile
+++ b/fs/btrfs/Makefile
@@ -13,6 +13,7 @@ btrfs-y += super.o ctree.o extent-tree.o print-tree.o
root-tree.o dir-item.o \
btrfs-$(CONFIG_BTRFS_FS_POSIX_ACL) += acl.o
btrfs-$(CONFIG_BTRFS_FS_CHECK_INTEGRITY) += check-integrity.o
+btrfs-$(CONFIG_BTRFS_FS_REF_VERIFY) += ref-verify.o
btrfs-$(CONFIG_BTRFS_FS_RUN_SANITY_TESTS) += tests/free-space-tests.o \
tests/extent-buffer-tests.o tests/btrfs-tests.o \
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 6d49db7..a4812ce 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -192,7 +192,7 @@ struct extent_buffer *btrfs_lock_root_node(struct
btrfs_root *root)
* tree until you end up with a lock on the root. A locked buffer
* is returned, with a reference held.
*/
-static struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root)
+struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root)
{
struct extent_buffer *eb;
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index d49b045..4fa3ddd 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1098,6 +1098,12 @@ struct btrfs_fs_info {
u32 nodesize;
u32 sectorsize;
u32 stripesize;
+
+#ifdef CONFIG_BTRFS_FS_REF_VERIFY
+ spinlock_t ref_verify_lock;
+ struct rb_root block_tree;
+ bool ref_verify_enabled;
+#endif
};
static inline struct btrfs_fs_info *btrfs_sb(struct super_block *sb)
@@ -1336,6 +1342,7 @@ static inline u32 BTRFS_MAX_XATTR_SIZE(const struct
btrfs_fs_info *info)
#define BTRFS_MOUNT_FRAGMENT_METADATA (1 << 25)
#define BTRFS_MOUNT_FREE_SPACE_TREE(1 << 26)
#define BTRFS_MOUNT_NOLOGREPLAY(1 << 27)
+#define BTRFS_MOUNT_REF_VERIFY (1 << 28)
#define BTRFS_DEFAULT_COMMIT_INTERVAL (30)
#define BTRFS_DEFAULT_MAX_INLINE (2048)
@@ -2627,7 +2634,7 @@ void btrfs_free_tree_block(struct btrfs_trans_handle
*trans,
struct extent_buffer *buf,
u64 parent, int last_ref);
int btrfs_alloc_reserved_file_extent(struct btrfs_trans_handle *trans,
-u64 root_objectid, u64 owner,
+struct btrfs_root *root, u64 owner,
u64 offset, u64 ram_bytes,
struct btrfs_key *ins);
int btrfs_alloc_logged_file_extent(struct btrfs_trans_handle *trans,
@@ -2646,7 +2653,7 @@ int btrfs_set_disk_extent_flags(struct btrfs_trans_handle
*trans,
u64 bytenr, u64 num_bytes, u64 flags,
int level, int is_data);
int btrfs_free_extent(struct btrfs_trans_handle *trans,
- struct btrfs_fs_info *fs_info,
+ struct btrfs_root *root,
u64 bytenr, u64 num_bytes, u64 parent, u64 root_objectid,
u64 owner, u64 offset);
@@ -2658,7 +2665,7 @@ void btrfs_prepare_extent_commit(struct btrfs_fs_info
*fs_info);
int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans,
