[PATCH 0/4] Btrfs-convert: Add support to copy common inode flags

[PATCH 0/4] Btrfs-convert: Add support to copy common inode flags

[Qu Wenruo]

Branch can be fetched from:
https://github.com/adam900710/btrfs-progs/tree/convert_inode_flags

Thanks for the report from Lakshmipathi.G, we exposed a bug that
btrfs-convert never copies common inode flags like SYNC/IMMUTABLE/APPEND.

The root cause is quite awkward, we didn't even have these flags defined
in ctree.h.

This patchset will copy related flags to btrfs-progs header and make
btrfs-convert to copy these flags.(Only some ext2 flags are support)
And enhance btrfs-debug-tree to handle these flags.

Finally, adds test case to prevent such problem happens again.

Qu Wenruo (4):
  btrfs-progs: Copy btrfs inode flags from kernel header
  btrfs-progs: Make btrfs-debug-tree print all readable strings for
    inode flags
  btrfs-progs: convert: Convert ext inode flags to btrfs inode flags
  btrfs-progs: convert-test: Add test case for common inode flags

 btrfs-convert.c                                    | 25 ++++++++++++
 ctree.h                                            |  9 +++++
 print-tree.c                                       | 46 ++++++++++++----------
 tests/convert-tests/009-common-inode-flags/test.sh | 34 ++++++++++++++++
 4 files changed, 94 insertions(+), 20 deletions(-)
 create mode 100755 tests/convert-tests/009-common-inode-flags/test.sh

-- 
2.10.0

[PATCH 1/4] btrfs-progs: Copy btrfs inode flags from kernel header

[Qu Wenruo]

Btrfs-progs header lacks quite a lot inode flags.
Copy them from kernel for later use.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 ctree.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ctree.h b/ctree.h
index e0dd2e4..c76b1f1 100644
--- a/ctree.h
+++ b/ctree.h
@@ -1246,6 +1246,15 @@ struct btrfs_root {
 #define BTRFS_INODE_NODATASUM		(1 << 0)
 #define BTRFS_INODE_NODATACOW		(1 << 1)
 #define BTRFS_INODE_READONLY		(1 << 2)
+#define BTRFS_INODE_NOCOMPRESS		(1 << 3)
+#define BTRFS_INODE_PREALLOC		(1 << 4)
+#define BTRFS_INODE_SYNC		(1 << 5)
+#define BTRFS_INODE_IMMUTABLE		(1 << 6)
+#define BTRFS_INODE_APPEND		(1 << 7)
+#define BTRFS_INODE_NODUMP		(1 << 8)
+#define BTRFS_INODE_NOATIME		(1 << 9)
+#define BTRFS_INODE_DIRSYNC		(1 << 10)
+#define BTRFS_INODE_COMPRESS		(1 << 11)
 
 #define read_eb_member(eb, ptr, type, member, result) (			\
 	read_extent_buffer(eb, (char *)(result),			\
-- 
2.10.0

[PATCH 2/4] btrfs-progs: Make btrfs-debug-tree print all readable strings for inode flags

[Qu Wenruo]

Before this patch, only 3 inode flags have readable string: NODATACOW,
NODATASUM, READONLY.

This patch will output all readable strings for remaining inode flags,
making debug-tree tool more handy.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 print-tree.c | 46 ++++++++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/print-tree.c b/print-tree.c
index 4444a14..f015646 100644
--- a/print-tree.c
+++ b/print-tree.c
@@ -851,29 +851,35 @@ static void print_uuid_item(struct extent_buffer *l, unsigned long offset,
 	}
 }
 
-/* Caller should ensure sizeof(*ret) >= 29 "NODATASUM|NODATACOW|READONLY" */
+#define copy_one_inode_flag(flags, name, empty, dst) ({			\
+	if (flags & BTRFS_INODE_##name) {				\
+		if (!empty)						\
+			strcat(dst, "|");				\
+		strcat(dst, #name);					\
+		empty = 0;						\
+	}								\
+})
+
+/*
+ * Caller should ensure sizeof(*ret) >= 102: all charactors plus '|' of
+ * BTRFS_INODE_* flags
+ */
 static void inode_flags_to_str(u64 flags, char *ret)
 {
 	int empty = 1;
 
-	if (flags & BTRFS_INODE_NODATASUM) {
-		empty = 0;
-		strcpy(ret, "NODATASUM");
-	}
-	if (flags & BTRFS_INODE_NODATACOW) {
-		if (!empty) {
-			empty = 0;
-			strcat(ret, "|");
-		}
-		strcat(ret, "NODATACOW");
-	}
-	if (flags & BTRFS_INODE_READONLY) {
-		if (!empty) {
-			empty = 0;
-			strcat(ret, "|");
-		}
-		strcat(ret, "READONLY");
-	}
+	copy_one_inode_flag(flags, NODATASUM, empty, ret);
+	copy_one_inode_flag(flags, NODATACOW, empty, ret);
+	copy_one_inode_flag(flags, READONLY, empty, ret);
+	copy_one_inode_flag(flags, NOCOMPRESS, empty, ret);
+	copy_one_inode_flag(flags, PREALLOC, empty, ret);
+	copy_one_inode_flag(flags, SYNC, empty, ret);
+	copy_one_inode_flag(flags, IMMUTABLE, empty, ret);
+	copy_one_inode_flag(flags, APPEND, empty, ret);
+	copy_one_inode_flag(flags, NODUMP, empty, ret);
+	copy_one_inode_flag(flags, NOATIME, empty, ret);
+	copy_one_inode_flag(flags, DIRSYNC, empty, ret);
+	copy_one_inode_flag(flags, COMPRESS, empty, ret);
 	if (empty)
 		strcat(ret, "none");
 }
@@ -902,7 +908,7 @@ void btrfs_print_leaf(struct btrfs_root *root, struct extent_buffer *l)
 	u32 nr = btrfs_header_nritems(l);
 	u64 objectid;
 	u32 type;
-	char flags_str[32];
+	char flags_str[128];
 
 	printf("leaf %llu items %d free space %d generation %llu owner %llu\n",
 		(unsigned long long)btrfs_header_bytenr(l), nr,
-- 
2.10.0

[PATCH 3/4] btrfs-progs: convert: Convert ext inode flags to btrfs inode flags

[Qu Wenruo]

Before this patch, btrfs-convert never copy ext* inode flags to
corresponding btrfs inode flags.

This makes common flags like APPEND/SYNC/SYNCDIR/IMMUTABLE not copied to
btrfs inode.

This patch introduces ext2_convert_inode_flags() function to handle the
convert, so btrfs-convert can copy as many inode flags as possible.

Reported-by: Lakshmipathi.G <lakshmipathi.g@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-convert.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 542179e..160a635 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2201,6 +2201,30 @@ static int ext2_check_state(struct btrfs_convert_context *cctx)
 		return 0;
 }
 
+#define copy_one_ext2_flag(flags, ext2_inode, name) ({			\
+	if (ext2_inode->i_flags & EXT2_##name##_FL)			\
+		flags |= BTRFS_INODE_##name;				\
+})
+
+/*
+ * Convert EXT2_*_FL to corresponding BTRFS_INODE_* flags
+ *
+ * Only a subset of EXT_*_FL is supported in btrfs.
+ */
+static void ext2_convert_inode_flags(struct btrfs_inode_item *dst,
+				     struct ext2_inode *src)
+{
+	u64 flags = 0;
+
+	copy_one_ext2_flag(flags, src, APPEND);
+	copy_one_ext2_flag(flags, src, SYNC);
+	copy_one_ext2_flag(flags, src, IMMUTABLE);
+	copy_one_ext2_flag(flags, src, NODUMP);
+	copy_one_ext2_flag(flags, src, NOATIME);
+	copy_one_ext2_flag(flags, src, DIRSYNC);
+	btrfs_set_stack_inode_flags(dst, flags);
+}
+
 /*
  * copy a single inode. do all the required works, such as cloning
  * inode item, creating file extents and creating directory entries.
@@ -2223,6 +2247,7 @@ static int ext2_copy_single_inode(struct btrfs_trans_handle *trans,
 			    BTRFS_INODE_NODATASUM;
 		btrfs_set_stack_inode_flags(&btrfs_inode, flags);
 	}
+	ext2_convert_inode_flags(&btrfs_inode, ext2_inode);
 
 	switch (ext2_inode->i_mode & S_IFMT) {
 	case S_IFREG:
-- 
2.10.0

[PATCH 4/4] btrfs-progs: convert-test: Add test case for common inode flags

[Qu Wenruo]

Add a new test case to check if btrfs-convert copies common inode flags
like append(a), immutable(i).

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 tests/convert-tests/009-common-inode-flags/test.sh | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100755 tests/convert-tests/009-common-inode-flags/test.sh

diff --git a/tests/convert-tests/009-common-inode-flags/test.sh b/tests/convert-tests/009-common-inode-flags/test.sh
new file mode 100755
index 0000000..8bbac22
--- /dev/null
+++ b/tests/convert-tests/009-common-inode-flags/test.sh
@@ -0,0 +1,34 @@
+#!/bin/bash
+# Check if btrfs-convert can copy common inode flags like SYNC/IMMUTABLE
+
+source $TOP/tests/common
+source $TOP/tests/common.convert
+
+setup_root_helper
+prepare_test_dev 512M
+check_prereq btrfs-convert
+
+fail=0
+default_mke2fs="mke2fs -t ext4 -b 4096"
+convert_test_preamble '' 'common inode flags test' 16k "$default_mke2fs"
+convert_test_prep_fs $default_mke2fs
+
+# create file with specific flags
+$SUDO_HELPER run_check touch $TEST_MNT/flag_test
+$SUDO_HELPER run_check chattr +aSidA $TEST_MNT/flag_test
+
+run_check_umount_test_dev
+convert_test_do_convert
+run_check_mount_test_dev
+
+# Above flags should be copied to btrfs flags, and lsattr should get them
+run_check_stdout lsattr $TEST_MNT/flag_test | cut -f1 -d\  > tmp_output
+grep -e "S" -e "i" -e "a" -e "d" -e "A" -q tmp_output
+if [ $? -ne 0 ]; then
+	rm tmp_output
+	_fail "no common inode flags are copied after convert"
+fi
+rm tmp_output
+
+run_check_umount_test_dev
+convert_test_post_rollback
-- 
2.10.0

Re: [PATCH 2/4] btrfs-progs: Make btrfs-debug-tree print all readable strings for inode flags

[David Sterba]

On Mon, Oct 10, 2016 at 11:11:19AM +0800, Qu Wenruo wrote:
> Before this patch, only 3 inode flags have readable string: NODATACOW,
> NODATASUM, READONLY.
> 
> This patch will output all readable strings for remaining inode flags,
> making debug-tree tool more handy.
> 
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>  print-tree.c | 46 ++++++++++++++++++++++++++--------------------
>  1 file changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/print-tree.c b/print-tree.c
> index 4444a14..f015646 100644
> --- a/print-tree.c
> +++ b/print-tree.c
> @@ -851,29 +851,35 @@ static void print_uuid_item(struct extent_buffer *l, unsigned long offset,
>  	}
>  }
>  
> -/* Caller should ensure sizeof(*ret) >= 29 "NODATASUM|NODATACOW|READONLY" */
> +#define copy_one_inode_flag(flags, name, empty, dst) ({			\
> +	if (flags & BTRFS_INODE_##name) {				\
> +		if (!empty)						\
> +			strcat(dst, "|");				\
> +		strcat(dst, #name);					\
> +		empty = 0;						\
> +	}								\
> +})

Can you please avoid using the macro? Or at least make it uppercase so
it's visible. Similar in the next patch.

Re: [PATCH 2/4] btrfs-progs: Make btrfs-debug-tree print all readable strings for inode flags

[Qu Wenruo]

At 10/10/2016 11:50 PM, David Sterba wrote:
> On Mon, Oct 10, 2016 at 11:11:19AM +0800, Qu Wenruo wrote:
>> Before this patch, only 3 inode flags have readable string: NODATACOW,
>> NODATASUM, READONLY.
>>
>> This patch will output all readable strings for remaining inode flags,
>> making debug-tree tool more handy.
>>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>  print-tree.c | 46 ++++++++++++++++++++++++++--------------------
>>  1 file changed, 26 insertions(+), 20 deletions(-)
>>
>> diff --git a/print-tree.c b/print-tree.c
>> index 4444a14..f015646 100644
>> --- a/print-tree.c
>> +++ b/print-tree.c
>> @@ -851,29 +851,35 @@ static void print_uuid_item(struct extent_buffer *l, unsigned long offset,
>>  	}
>>  }
>>
>> -/* Caller should ensure sizeof(*ret) >= 29 "NODATASUM|NODATACOW|READONLY" */
>> +#define copy_one_inode_flag(flags, name, empty, dst) ({			\
>> +	if (flags & BTRFS_INODE_##name) {				\
>> +		if (!empty)						\
>> +			strcat(dst, "|");				\
>> +		strcat(dst, #name);					\
>> +		empty = 0;						\
>> +	}								\
>> +})
>
> Can you please avoid using the macro? Or at least make it uppercase so
> it's visible. Similar in the next patch.
>
>
OK, I'll change it to upper case.

The only reason I'm using macro is, inline function can't do 
stringification, or I missed something?

Thanks,
Qu

Re: [PATCH 2/4] btrfs-progs: Make btrfs-debug-tree print all readable strings for inode flags

[David Sterba]

On Tue, Oct 11, 2016 at 10:18:51AM +0800, Qu Wenruo wrote:
> >> -/* Caller should ensure sizeof(*ret) >= 29 "NODATASUM|NODATACOW|READONLY" */
> >> +#define copy_one_inode_flag(flags, name, empty, dst) ({			\
> >> +	if (flags & BTRFS_INODE_##name) {				\
> >> +		if (!empty)						\
> >> +			strcat(dst, "|");				\
> >> +		strcat(dst, #name);					\
> >> +		empty = 0;						\
> >> +	}								\
> >> +})
> >
> > Can you please avoid using the macro? Or at least make it uppercase so
> > it's visible. Similar in the next patch.
> >
> >
> OK, I'll change it to upper case.

Ok.

> The only reason I'm using macro is, inline function can't do 
> stringification, or I missed something?

No, that's where macros help. My concern was about the hidden use of a
local variable, so at least an all-caps macro name would make it more
visible. As this is not going to be used elsewhere, we can live with
that.