All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition
@ 2021-10-19 11:29 Qu Wenruo
  2021-10-19 11:29 ` [PATCH 1/2] btrfs: make sizeof(struct btrfs_super_block) to match BTRFS_SUPER_INFO_SIZE Qu Wenruo
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Qu Wenruo @ 2021-10-19 11:29 UTC (permalink / raw)
  To: linux-btrfs

This patch is to enhance the definition of btrfs_super_block by:

- Unify sizeof(struct btrfs_super_block) and BTRFS_SUPER_INFO_SIZE
  In kernel, it's just 3 location allocating BTRFS_SUPER_INFO_SIZE
  (one of them is for selftest), so such change is not doing much
  difference.
  But for btrfs-progs, it would remove call sites like:

        char tmp[BTRFS_SUPER_INFO_SIZE];
        struct btrfs_super_block *buf = (struct btrfs_super_block *)tmp;
 
- Move btrfs_super_block definition to uapi/linux/btrfs_tree.h.
  Due to BTRFS_IOC_TREE_SEARCH ioctl, we're almost exposing all on-disk
  formats to the user space.
  Thus it's almost a perfect location to contain all on-disk schema.

Qu Wenruo (2):
  btrfs: make sizeof(struct btrfs_super_block) to match
    BTRFS_SUPER_INFO_SIZE
  btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h

 fs/btrfs/ctree.h                | 136 --------------------------------
 fs/btrfs/super.c                |   2 +
 include/uapi/linux/btrfs_tree.h | 135 +++++++++++++++++++++++++++++++
 3 files changed, 137 insertions(+), 136 deletions(-)

-- 
2.33.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] btrfs: make sizeof(struct btrfs_super_block) to match BTRFS_SUPER_INFO_SIZE
  2021-10-19 11:29 [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition Qu Wenruo
@ 2021-10-19 11:29 ` Qu Wenruo
  2021-10-19 15:46   ` David Sterba
  2021-10-19 11:29 ` [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h Qu Wenruo
  2021-10-19 14:08 ` [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition Josef Bacik
  2 siblings, 1 reply; 11+ messages in thread
From: Qu Wenruo @ 2021-10-19 11:29 UTC (permalink / raw)
  To: linux-btrfs

It's a common practice to avoid use sizeof(struct btrfs_super_block)
(3531), but to use BTRFS_SUPER_INFO_SIZE (4096).

The problem is that, sizeof(struct btrfs_super_block) doesn't match
BTRFS_SUPER_INFO_SIZE from the very beginning.

Furthermore, for all call sites except selftest, we always allocate
BTRFS_SUPER_INFO_SIZE space for btrfs super block, there isn't any real
reason to use the smaller value, and it doesn't really save any space.

So let's get rid of such confusing behavior, and unify those two values.

This patch will also add a BUILD_BUG_ON() to verify the value at build
time.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h | 3 +++
 fs/btrfs/super.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 140126898577..e05098ac0337 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -270,6 +270,9 @@ struct btrfs_super_block {
 	__le64 reserved[28];
 	u8 sys_chunk_array[BTRFS_SYSTEM_CHUNK_ARRAY_SIZE];
 	struct btrfs_root_backup super_roots[BTRFS_NUM_BACKUP_ROOTS];
+
+	/* Padded to 4096 bytes */
+	u8 padding[565];
 } __attribute__ ((__packed__));
 
 /*
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index a1c54a2c787c..6fb5cdfceaf5 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2552,6 +2552,8 @@ static int __init init_btrfs_fs(void)
 {
 	int err;
 
+	BUILD_BUG_ON(sizeof(struct btrfs_super_block) != BTRFS_SUPER_INFO_SIZE);
+
 	btrfs_props_init();
 
 	err = btrfs_init_sysfs();
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h
  2021-10-19 11:29 [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition Qu Wenruo
  2021-10-19 11:29 ` [PATCH 1/2] btrfs: make sizeof(struct btrfs_super_block) to match BTRFS_SUPER_INFO_SIZE Qu Wenruo
@ 2021-10-19 11:29 ` Qu Wenruo
  2021-10-19 16:10   ` David Sterba
  2021-11-07 23:24     ` kernel test robot
  2021-10-19 14:08 ` [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition Josef Bacik
  2 siblings, 2 replies; 11+ messages in thread
From: Qu Wenruo @ 2021-10-19 11:29 UTC (permalink / raw)
  To: linux-btrfs

Due to the fact that btrfs_tree.h contains all the info for
BTRFS_IOC_TREE_SEARCH, it's almost the perfect location of btrfs on-disk
schema.

So let's move struct btrfs_super_block to uapi/linux/btrfs_tree.h,
further reducing the size of ctree.h.

Also move btrfs_dev_item and btrfs_root_backup to btrfs_tree.h, as they
are included in btrfs_super_block.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h                | 139 --------------------------------
 include/uapi/linux/btrfs_tree.h | 135 +++++++++++++++++++++++++++++++
 2 files changed, 135 insertions(+), 139 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index e05098ac0337..843b3fdf132e 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -171,145 +171,6 @@ struct btrfs_header {
 	u8 level;
 } __attribute__ ((__packed__));
 
-/*
- * this is a very generous portion of the super block, giving us
- * room to translate 14 chunks with 3 stripes each.
- */
-#define BTRFS_SYSTEM_CHUNK_ARRAY_SIZE 2048
-
-/*
- * just in case we somehow lose the roots and are not able to mount,
- * we store an array of the roots from previous transactions
- * in the super.
- */
-#define BTRFS_NUM_BACKUP_ROOTS 4
-struct btrfs_root_backup {
-	__le64 tree_root;
-	__le64 tree_root_gen;
-
-	__le64 chunk_root;
-	__le64 chunk_root_gen;
-
-	__le64 extent_root;
-	__le64 extent_root_gen;
-
-	__le64 fs_root;
-	__le64 fs_root_gen;
-
-	__le64 dev_root;
-	__le64 dev_root_gen;
-
-	__le64 csum_root;
-	__le64 csum_root_gen;
-
-	__le64 total_bytes;
-	__le64 bytes_used;
-	__le64 num_devices;
-	/* future */
-	__le64 unused_64[4];
-
-	u8 tree_root_level;
-	u8 chunk_root_level;
-	u8 extent_root_level;
-	u8 fs_root_level;
-	u8 dev_root_level;
-	u8 csum_root_level;
-	/* future and to align */
-	u8 unused_8[10];
-} __attribute__ ((__packed__));
-
-/*
- * the super block basically lists the main trees of the FS
- * it currently lacks any block count etc etc
- */
-struct btrfs_super_block {
-	/* the first 4 fields must match struct btrfs_header */
-	u8 csum[BTRFS_CSUM_SIZE];
-	/* FS specific UUID, visible to user */
-	u8 fsid[BTRFS_FSID_SIZE];
-	__le64 bytenr; /* this block number */
-	__le64 flags;
-
-	/* allowed to be different from the btrfs_header from here own down */
-	__le64 magic;
-	__le64 generation;
-	__le64 root;
-	__le64 chunk_root;
-	__le64 log_root;
-
-	/* this will help find the new super based on the log root */
-	__le64 log_root_transid;
-	__le64 total_bytes;
-	__le64 bytes_used;
-	__le64 root_dir_objectid;
-	__le64 num_devices;
-	__le32 sectorsize;
-	__le32 nodesize;
-	__le32 __unused_leafsize;
-	__le32 stripesize;
-	__le32 sys_chunk_array_size;
-	__le64 chunk_root_generation;
-	__le64 compat_flags;
-	__le64 compat_ro_flags;
-	__le64 incompat_flags;
-	__le16 csum_type;
-	u8 root_level;
-	u8 chunk_root_level;
-	u8 log_root_level;
-	struct btrfs_dev_item dev_item;
-
-	char label[BTRFS_LABEL_SIZE];
-
-	__le64 cache_generation;
-	__le64 uuid_tree_generation;
-
-	/* the UUID written into btree blocks */
-	u8 metadata_uuid[BTRFS_FSID_SIZE];
-
-	/* future expansion */
-	__le64 reserved[28];
-	u8 sys_chunk_array[BTRFS_SYSTEM_CHUNK_ARRAY_SIZE];
-	struct btrfs_root_backup super_roots[BTRFS_NUM_BACKUP_ROOTS];
-
-	/* Padded to 4096 bytes */
-	u8 padding[565];
-} __attribute__ ((__packed__));
-
-/*
- * Compat flags that we support.  If any incompat flags are set other than the
- * ones specified below then we will fail to mount
- */
-#define BTRFS_FEATURE_COMPAT_SUPP		0ULL
-#define BTRFS_FEATURE_COMPAT_SAFE_SET		0ULL
-#define BTRFS_FEATURE_COMPAT_SAFE_CLEAR		0ULL
-
-#define BTRFS_FEATURE_COMPAT_RO_SUPP			\
-	(BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE |	\
-	 BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE_VALID | \
-	 BTRFS_FEATURE_COMPAT_RO_VERITY)
-
-#define BTRFS_FEATURE_COMPAT_RO_SAFE_SET	0ULL
-#define BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR	0ULL
-
-#define BTRFS_FEATURE_INCOMPAT_SUPP			\
-	(BTRFS_FEATURE_INCOMPAT_MIXED_BACKREF |		\
-	 BTRFS_FEATURE_INCOMPAT_DEFAULT_SUBVOL |	\
-	 BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS |		\
-	 BTRFS_FEATURE_INCOMPAT_BIG_METADATA |		\
-	 BTRFS_FEATURE_INCOMPAT_COMPRESS_LZO |		\
-	 BTRFS_FEATURE_INCOMPAT_COMPRESS_ZSTD |		\
-	 BTRFS_FEATURE_INCOMPAT_RAID56 |		\
-	 BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF |		\
-	 BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA |	\
-	 BTRFS_FEATURE_INCOMPAT_NO_HOLES	|	\
-	 BTRFS_FEATURE_INCOMPAT_METADATA_UUID	|	\
-	 BTRFS_FEATURE_INCOMPAT_RAID1C34	|	\
-	 BTRFS_FEATURE_INCOMPAT_ZONED)
-
-#define BTRFS_FEATURE_INCOMPAT_SAFE_SET			\
-	(BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF)
-#define BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR		0ULL
-
 /*
  * A leaf is full of items. offset and size tell us where to find
  * the item in the leaf (relative to the start of the data area)
diff --git a/include/uapi/linux/btrfs_tree.h b/include/uapi/linux/btrfs_tree.h
index e1c4c732aaba..4770d12d4888 100644
--- a/include/uapi/linux/btrfs_tree.h
+++ b/include/uapi/linux/btrfs_tree.h
@@ -483,6 +483,53 @@ struct btrfs_free_space_header {
 	__le64 num_bitmaps;
 } __attribute__ ((__packed__));
 
+/*
+ * This is a very generous portion of the super block, giving us
+ * room to translate 14 chunks with 3 stripes each.
+ */
+#define BTRFS_SYSTEM_CHUNK_ARRAY_SIZE 2048
+
+/*
+ * Just in case we somehow lose the roots and are not able to mount,
+ * we store an array of the roots from previous transactions
+ * in the super.
+ */
+#define BTRFS_NUM_BACKUP_ROOTS 4
+struct btrfs_root_backup {
+	__le64 tree_root;
+	__le64 tree_root_gen;
+
+	__le64 chunk_root;
+	__le64 chunk_root_gen;
+
+	__le64 extent_root;
+	__le64 extent_root_gen;
+
+	__le64 fs_root;
+	__le64 fs_root_gen;
+
+	__le64 dev_root;
+	__le64 dev_root_gen;
+
+	__le64 csum_root;
+	__le64 csum_root_gen;
+
+	__le64 total_bytes;
+	__le64 bytes_used;
+	__le64 num_devices;
+	/* future */
+	__le64 unused_64[4];
+
+	u8 tree_root_level;
+	u8 chunk_root_level;
+	u8 extent_root_level;
+	u8 fs_root_level;
+	u8 dev_root_level;
+	u8 csum_root_level;
+	/* future and to align */
+	u8 unused_8[10];
+} __attribute__ ((__packed__));
+
 #define BTRFS_HEADER_FLAG_WRITTEN	(1ULL << 0)
 #define BTRFS_HEADER_FLAG_RELOC		(1ULL << 1)
 
@@ -496,6 +543,94 @@ struct btrfs_free_space_header {
 #define BTRFS_SUPER_FLAG_CHANGING_FSID	(1ULL << 35)
 #define BTRFS_SUPER_FLAG_CHANGING_FSID_V2 (1ULL << 36)
 
+/*
+ * Compat flags that we support.  If any incompat flags are set other than the
+ * ones specified below then we will fail to mount
+ */
+#define BTRFS_FEATURE_COMPAT_SUPP		0ULL
+#define BTRFS_FEATURE_COMPAT_SAFE_SET		0ULL
+#define BTRFS_FEATURE_COMPAT_SAFE_CLEAR		0ULL
+
+#define BTRFS_FEATURE_COMPAT_RO_SUPP			\
+	(BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE |	\
+	 BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE_VALID | \
+	 BTRFS_FEATURE_COMPAT_RO_VERITY)
+
+#define BTRFS_FEATURE_COMPAT_RO_SAFE_SET	0ULL
+#define BTRFS_FEATURE_COMPAT_RO_SAFE_CLEAR	0ULL
+
+#define BTRFS_FEATURE_INCOMPAT_SUPP			\
+	(BTRFS_FEATURE_INCOMPAT_MIXED_BACKREF |		\
+	 BTRFS_FEATURE_INCOMPAT_DEFAULT_SUBVOL |	\
+	 BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS |		\
+	 BTRFS_FEATURE_INCOMPAT_BIG_METADATA |		\
+	 BTRFS_FEATURE_INCOMPAT_COMPRESS_LZO |		\
+	 BTRFS_FEATURE_INCOMPAT_COMPRESS_ZSTD |		\
+	 BTRFS_FEATURE_INCOMPAT_RAID56 |		\
+	 BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF |		\
+	 BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA |	\
+	 BTRFS_FEATURE_INCOMPAT_NO_HOLES	|	\
+	 BTRFS_FEATURE_INCOMPAT_METADATA_UUID	|	\
+	 BTRFS_FEATURE_INCOMPAT_RAID1C34	|	\
+	 BTRFS_FEATURE_INCOMPAT_ZONED)
+
+#define BTRFS_FEATURE_INCOMPAT_SAFE_SET			\
+	(BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF)
+#define BTRFS_FEATURE_INCOMPAT_SAFE_CLEAR		0ULL
+
+/* The super block basically lists the main trees of the FS */
+struct btrfs_super_block {
+	u8 csum[BTRFS_CSUM_SIZE];
+	/* FS specific UUID, visible to user */
+	u8 fsid[BTRFS_FSID_SIZE];
+
+	/* Physical bytenr of this super block */
+	__le64 bytenr;
+
+	/* Valid bits are (BTRFS_SUPER_FLAG_* | BTRFS_HEADER_FLAG_WRITTEN) */
+	__le64 flags;
+	__le64 magic;
+	__le64 generation;
+	/* All bytenr used in root and *_root are logical bytenr */
+	__le64 root;
+	__le64 chunk_root;
+	__le64 log_root;
+	__le64 log_root_transid;
+	__le64 total_bytes;
+	__le64 bytes_used;
+	__le64 root_dir_objectid;
+	__le64 num_devices;
+	__le32 sectorsize;
+	__le32 nodesize;
+	__le32 __unused_leafsize;
+	__le32 stripesize;
+	__le32 sys_chunk_array_size;
+	__le64 chunk_root_generation;
+	__le64 compat_flags;
+	__le64 compat_ro_flags;
+	__le64 incompat_flags;
+	__le16 csum_type;
+	u8 root_level;
+	u8 chunk_root_level;
+	u8 log_root_level;
+	struct btrfs_dev_item dev_item;
+
+	char label[BTRFS_LABEL_SIZE];
+
+	__le64 cache_generation;
+	__le64 uuid_tree_generation;
+
+	/* The UUID written into btree blocks */
+	u8 metadata_uuid[BTRFS_FSID_SIZE];
+
+	/* Future expansion */
+	__le64 reserved[28];
+	u8 sys_chunk_array[BTRFS_SYSTEM_CHUNK_ARRAY_SIZE];
+	struct btrfs_root_backup super_roots[BTRFS_NUM_BACKUP_ROOTS];
+
+	/* Padded to 4096 bytes */
+	u8 padding[565];
+} __attribute__ ((__packed__));
 
 /*
  * items in the extent btree are used to record the objectid of the
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition
  2021-10-19 11:29 [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition Qu Wenruo
  2021-10-19 11:29 ` [PATCH 1/2] btrfs: make sizeof(struct btrfs_super_block) to match BTRFS_SUPER_INFO_SIZE Qu Wenruo
  2021-10-19 11:29 ` [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h Qu Wenruo
@ 2021-10-19 14:08 ` Josef Bacik
  2 siblings, 0 replies; 11+ messages in thread
From: Josef Bacik @ 2021-10-19 14:08 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Oct 19, 2021 at 07:29:23PM +0800, Qu Wenruo wrote:
> This patch is to enhance the definition of btrfs_super_block by:
> 
> - Unify sizeof(struct btrfs_super_block) and BTRFS_SUPER_INFO_SIZE
>   In kernel, it's just 3 location allocating BTRFS_SUPER_INFO_SIZE
>   (one of them is for selftest), so such change is not doing much
>   difference.
>   But for btrfs-progs, it would remove call sites like:
> 
>         char tmp[BTRFS_SUPER_INFO_SIZE];
>         struct btrfs_super_block *buf = (struct btrfs_super_block *)tmp;
>  
> - Move btrfs_super_block definition to uapi/linux/btrfs_tree.h.
>   Due to BTRFS_IOC_TREE_SEARCH ioctl, we're almost exposing all on-disk
>   formats to the user space.
>   Thus it's almost a perfect location to contain all on-disk schema.
> 
> Qu Wenruo (2):
>   btrfs: make sizeof(struct btrfs_super_block) to match
>     BTRFS_SUPER_INFO_SIZE
>   btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h
> 

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] btrfs: make sizeof(struct btrfs_super_block) to match BTRFS_SUPER_INFO_SIZE
  2021-10-19 11:29 ` [PATCH 1/2] btrfs: make sizeof(struct btrfs_super_block) to match BTRFS_SUPER_INFO_SIZE Qu Wenruo
@ 2021-10-19 15:46   ` David Sterba
  0 siblings, 0 replies; 11+ messages in thread
From: David Sterba @ 2021-10-19 15:46 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Oct 19, 2021 at 07:29:24PM +0800, Qu Wenruo wrote:
> It's a common practice to avoid use sizeof(struct btrfs_super_block)
> (3531), but to use BTRFS_SUPER_INFO_SIZE (4096).
> 
> The problem is that, sizeof(struct btrfs_super_block) doesn't match
> BTRFS_SUPER_INFO_SIZE from the very beginning.
> 
> Furthermore, for all call sites except selftest, we always allocate
> BTRFS_SUPER_INFO_SIZE space for btrfs super block, there isn't any real
> reason to use the smaller value, and it doesn't really save any space.

Right, due to the size it would end up in the 4K slab anyway.

> So let's get rid of such confusing behavior, and unify those two values.
> 
> This patch will also add a BUILD_BUG_ON() to verify the value at build
> time.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ctree.h | 3 +++
>  fs/btrfs/super.c | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 140126898577..e05098ac0337 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -270,6 +270,9 @@ struct btrfs_super_block {
>  	__le64 reserved[28];
>  	u8 sys_chunk_array[BTRFS_SYSTEM_CHUNK_ARRAY_SIZE];
>  	struct btrfs_root_backup super_roots[BTRFS_NUM_BACKUP_ROOTS];
> +
> +	/* Padded to 4096 bytes */
> +	u8 padding[565];
>  } __attribute__ ((__packed__));

Please use static_assert, it can be used in a header and also is self
documenting and right in the place where we care about it.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h
  2021-10-19 11:29 ` [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h Qu Wenruo
@ 2021-10-19 16:10   ` David Sterba
  2021-10-20  0:19     ` Qu Wenruo
  2021-11-07 23:24     ` kernel test robot
  1 sibling, 1 reply; 11+ messages in thread
From: David Sterba @ 2021-10-19 16:10 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Oct 19, 2021 at 07:29:25PM +0800, Qu Wenruo wrote:
> Due to the fact that btrfs_tree.h contains all the info for
> BTRFS_IOC_TREE_SEARCH, it's almost the perfect location of btrfs on-disk
> schema.
> 
> So let's move struct btrfs_super_block to uapi/linux/btrfs_tree.h,
> further reducing the size of ctree.h.

The definitions of tree items are in the public header due to the search
tree ioctl, but why do you want to make superblock public? Ie. what user
space tool is going to use it?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h
  2021-10-19 16:10   ` David Sterba
@ 2021-10-20  0:19     ` Qu Wenruo
  2021-10-20 17:13       ` David Sterba
  0 siblings, 1 reply; 11+ messages in thread
From: Qu Wenruo @ 2021-10-20  0:19 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs



On 2021/10/20 00:10, David Sterba wrote:
> On Tue, Oct 19, 2021 at 07:29:25PM +0800, Qu Wenruo wrote:
>> Due to the fact that btrfs_tree.h contains all the info for
>> BTRFS_IOC_TREE_SEARCH, it's almost the perfect location of btrfs on-disk
>> schema.
>>
>> So let's move struct btrfs_super_block to uapi/linux/btrfs_tree.h,
>> further reducing the size of ctree.h.
>
> The definitions of tree items are in the public header due to the search
> tree ioctl, but why do you want to make superblock public? Ie. what user
> space tool is going to use it?
>

Well, for super block I'd say any user space tool can directly see it.

My main objective here is to move all on-disk format to uapi.

And I don't have better idea than reusing the existing btrfs_tree.h.

Thanks,
Qu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h
  2021-10-20  0:19     ` Qu Wenruo
@ 2021-10-20 17:13       ` David Sterba
  2021-10-20 23:18         ` Qu Wenruo
  0 siblings, 1 reply; 11+ messages in thread
From: David Sterba @ 2021-10-20 17:13 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: dsterba, Qu Wenruo, linux-btrfs

On Wed, Oct 20, 2021 at 08:19:30AM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/10/20 00:10, David Sterba wrote:
> > On Tue, Oct 19, 2021 at 07:29:25PM +0800, Qu Wenruo wrote:
> >> Due to the fact that btrfs_tree.h contains all the info for
> >> BTRFS_IOC_TREE_SEARCH, it's almost the perfect location of btrfs on-disk
> >> schema.
> >>
> >> So let's move struct btrfs_super_block to uapi/linux/btrfs_tree.h,
> >> further reducing the size of ctree.h.
> >
> > The definitions of tree items are in the public header due to the search
> > tree ioctl, but why do you want to make superblock public? Ie. what user
> > space tool is going to use it?
> >
> 
> Well, for super block I'd say any user space tool can directly see it.
> 
> My main objective here is to move all on-disk format to uapi.

Why? You sent such patches in the past, I need to read again what we
discussed but I don't think we should put everything to the UAPI
headers.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h
  2021-10-20 17:13       ` David Sterba
@ 2021-10-20 23:18         ` Qu Wenruo
  0 siblings, 0 replies; 11+ messages in thread
From: Qu Wenruo @ 2021-10-20 23:18 UTC (permalink / raw)
  To: dsterba, Qu Wenruo, linux-btrfs



On 2021/10/21 01:13, David Sterba wrote:
> On Wed, Oct 20, 2021 at 08:19:30AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/10/20 00:10, David Sterba wrote:
>>> On Tue, Oct 19, 2021 at 07:29:25PM +0800, Qu Wenruo wrote:
>>>> Due to the fact that btrfs_tree.h contains all the info for
>>>> BTRFS_IOC_TREE_SEARCH, it's almost the perfect location of btrfs on-disk
>>>> schema.
>>>>
>>>> So let's move struct btrfs_super_block to uapi/linux/btrfs_tree.h,
>>>> further reducing the size of ctree.h.
>>>
>>> The definitions of tree items are in the public header due to the search
>>> tree ioctl, but why do you want to make superblock public? Ie. what user
>>> space tool is going to use it?
>>>
>>
>> Well, for super block I'd say any user space tool can directly see it.
>>
>> My main objective here is to move all on-disk format to uapi.
>
> Why? You sent such patches in the past, I need to read again what we
> discussed but I don't think we should put everything to the UAPI
> headers.
>
Then we need a new header for on-disk format that is not exposed through
tree search API.

It's definitely not ideal to put everything into ctree.h.

This is especially useful when someone is crafting a new btrfs
implementation, no matter if it's something like winBtrfs or BSD (if
they really want) or something else.

In fact when I am trying to create the btrfs-fuse, one of the things I'm
doing is separating things from ctree.h.

Thanks,
Qu

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h
  2021-10-19 11:29 ` [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h Qu Wenruo
@ 2021-11-07 23:24     ` kernel test robot
  2021-11-07 23:24     ` kernel test robot
  1 sibling, 0 replies; 11+ messages in thread
From: kernel test robot @ 2021-11-07 23:24 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs; +Cc: llvm, kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3553 bytes --]

Hi Qu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.15]
[cannot apply to next-20211106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-cleanup-on-btrfs_super_block-definition/20211019-193103
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-c001-20211019 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9660563950aaed54020bfdf0be07e7096a9553e4)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5207fb0ee3a623a1069a84661bf6b41104c468e5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Qu-Wenruo/btrfs-cleanup-on-btrfs_super_block-definition/20211019-193103
        git checkout 5207fb0ee3a623a1069a84661bf6b41104c468e5
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <built-in>:1:
>> ./usr/include/linux/btrfs_tree.h:519:2: error: unknown type name 'u8'
           u8 tree_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:520:2: error: unknown type name 'u8'
           u8 chunk_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:521:2: error: unknown type name 'u8'
           u8 extent_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:522:2: error: unknown type name 'u8'
           u8 fs_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:523:2: error: unknown type name 'u8'
           u8 dev_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:524:2: error: unknown type name 'u8'
           u8 csum_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:526:2: error: unknown type name 'u8'
           u8 unused_8[10];
           ^
   ./usr/include/linux/btrfs_tree.h:579:2: error: unknown type name 'u8'
           u8 csum[BTRFS_CSUM_SIZE];
           ^
   ./usr/include/linux/btrfs_tree.h:581:2: error: unknown type name 'u8'
           u8 fsid[BTRFS_FSID_SIZE];
           ^
   ./usr/include/linux/btrfs_tree.h:609:2: error: unknown type name 'u8'
           u8 root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:610:2: error: unknown type name 'u8'
           u8 chunk_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:611:2: error: unknown type name 'u8'
           u8 log_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:620:2: error: unknown type name 'u8'
           u8 metadata_uuid[BTRFS_FSID_SIZE];
           ^
   ./usr/include/linux/btrfs_tree.h:624:2: error: unknown type name 'u8'
           u8 sys_chunk_array[BTRFS_SYSTEM_CHUNK_ARRAY_SIZE];
           ^
   ./usr/include/linux/btrfs_tree.h:628:2: error: unknown type name 'u8'
           u8 padding[565];
           ^
   15 errors generated.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28932 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h
@ 2021-11-07 23:24     ` kernel test robot
  0 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2021-11-07 23:24 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3637 bytes --]

Hi Qu,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on v5.15]
[cannot apply to next-20211106]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-cleanup-on-btrfs_super_block-definition/20211019-193103
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-c001-20211019 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 9660563950aaed54020bfdf0be07e7096a9553e4)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/5207fb0ee3a623a1069a84661bf6b41104c468e5
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Qu-Wenruo/btrfs-cleanup-on-btrfs_super_block-definition/20211019-193103
        git checkout 5207fb0ee3a623a1069a84661bf6b41104c468e5
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <built-in>:1:
>> ./usr/include/linux/btrfs_tree.h:519:2: error: unknown type name 'u8'
           u8 tree_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:520:2: error: unknown type name 'u8'
           u8 chunk_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:521:2: error: unknown type name 'u8'
           u8 extent_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:522:2: error: unknown type name 'u8'
           u8 fs_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:523:2: error: unknown type name 'u8'
           u8 dev_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:524:2: error: unknown type name 'u8'
           u8 csum_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:526:2: error: unknown type name 'u8'
           u8 unused_8[10];
           ^
   ./usr/include/linux/btrfs_tree.h:579:2: error: unknown type name 'u8'
           u8 csum[BTRFS_CSUM_SIZE];
           ^
   ./usr/include/linux/btrfs_tree.h:581:2: error: unknown type name 'u8'
           u8 fsid[BTRFS_FSID_SIZE];
           ^
   ./usr/include/linux/btrfs_tree.h:609:2: error: unknown type name 'u8'
           u8 root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:610:2: error: unknown type name 'u8'
           u8 chunk_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:611:2: error: unknown type name 'u8'
           u8 log_root_level;
           ^
   ./usr/include/linux/btrfs_tree.h:620:2: error: unknown type name 'u8'
           u8 metadata_uuid[BTRFS_FSID_SIZE];
           ^
   ./usr/include/linux/btrfs_tree.h:624:2: error: unknown type name 'u8'
           u8 sys_chunk_array[BTRFS_SYSTEM_CHUNK_ARRAY_SIZE];
           ^
   ./usr/include/linux/btrfs_tree.h:628:2: error: unknown type name 'u8'
           u8 padding[565];
           ^
   15 errors generated.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28932 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-11-07 23:25 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 11:29 [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition Qu Wenruo
2021-10-19 11:29 ` [PATCH 1/2] btrfs: make sizeof(struct btrfs_super_block) to match BTRFS_SUPER_INFO_SIZE Qu Wenruo
2021-10-19 15:46   ` David Sterba
2021-10-19 11:29 ` [PATCH 2/2] btrfs: move btrfs_super_block to uapi/linux/btrfs_tree.h Qu Wenruo
2021-10-19 16:10   ` David Sterba
2021-10-20  0:19     ` Qu Wenruo
2021-10-20 17:13       ` David Sterba
2021-10-20 23:18         ` Qu Wenruo
2021-11-07 23:24   ` kernel test robot
2021-11-07 23:24     ` kernel test robot
2021-10-19 14:08 ` [PATCH 0/2] btrfs: cleanup on btrfs_super_block definition Josef Bacik

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.