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

[PATCH v2 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.

Changelog:
v2:
  Use upper case for stringification macro for patch 2 and 3.

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                                    | 26 ++++++++++++
 ctree.h                                            |  9 +++++
 print-tree.c                                       | 47 +++++++++++++---------
 tests/convert-tests/009-common-inode-flags/test.sh | 34 ++++++++++++++++
 4 files changed, 96 insertions(+), 20 deletions(-)
 create mode 100755 tests/convert-tests/009-common-inode-flags/test.sh

-- 
2.10.0

[PATCH v2 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 v2 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 | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/print-tree.c b/print-tree.c
index 4444a14..990cdba 100644
--- a/print-tree.c
+++ b/print-tree.c
@@ -851,29 +851,36 @@ static void print_uuid_item(struct extent_buffer *l, unsigned long offset,
 	}
 }
 
-/* Caller should ensure sizeof(*ret) >= 29 "NODATASUM|NODATACOW|READONLY" */
+/* Btrfs inode flag stringification helper */
+#define CAT_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");
-	}
+	CAT_ONE_INODE_FLAG(flags, NODATASUM, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, NODATACOW, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, READONLY, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, NOCOMPRESS, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, PREALLOC, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, SYNC, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, IMMUTABLE, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, APPEND, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, NODUMP, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, NOATIME, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, DIRSYNC, empty, ret);
+	CAT_ONE_INODE_FLAG(flags, COMPRESS, empty, ret);
 	if (empty)
 		strcat(ret, "none");
 }
@@ -902,7 +909,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 v2 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 | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/btrfs-convert.c b/btrfs-convert.c
index 542179e..2bcc16c 100644
--- a/btrfs-convert.c
+++ b/btrfs-convert.c
@@ -2201,6 +2201,31 @@ static int ext2_check_state(struct btrfs_convert_context *cctx)
 		return 0;
 }
 
+/* EXT2_*_FL to BTRFS_INODE_FLAG_* stringification helper */
+#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 +2248,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 v2 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 v2 0/4] Btrfs-convert: Add support to copy common inode flags

[Qu Wenruo]

At 10/13/2016 10:15 PM, David Sterba wrote:
> Hi,
>
> patches merged to devel, thanks. I made a few minor adjustments, not
> bothering you with them.

Thanks for all your work.
Now I can just rebase my scrub work to devel branch.

>
> On Tue, Oct 11, 2016 at 10:44:42AM +0800, Qu Wenruo wrote:
>> Branch can be fetched from:
>> https://github.com/adam900710/btrfs-progs/tree/convert_inode_flags
>
> I can pull the branches from you, either to devel or integration. Please
> base them on the last release point (ie. master) if there are no other
> dependencies. Otherwise, I'll publish a branch that contains the desired
> patches and will promise not to change it with rebases.
>

Sorry I'm not very familiar with when to send a pull request and when to 
send normal patches, and to which branch it should be rebased to.

 From my understanding, for large patchset pull request is preferred(and 
patchse should still be sent out for review) and for small changes, 
normal mail should be good enough.

And normally I rebase all my branches/patches to your devel branch.
Or should I rebase them to the master branch?

Thanks,
Qu

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

[David Sterba]

On Fri, Oct 14, 2016 at 08:50:31AM +0800, Qu Wenruo wrote:
> > I can pull the branches from you, either to devel or integration. Please
> > base them on the last release point (ie. master) if there are no other
> > dependencies. Otherwise, I'll publish a branch that contains the desired
> > patches and will promise not to change it with rebases.
> 
> Sorry I'm not very familiar with when to send a pull request and when to 
> send normal patches, and to which branch it should be rebased to.

It's no required, just would make a few things easier. The pull is
additional to mailinglist patches, so send patches as usual.  If you put
the patches to a branch that I can pull, I'll do that eventually. This
happens if the patches are in a state that I don't need to touch them at
all or do just minor modifications.

>  From my understanding, for large patchset pull request is preferred(and 
> patchse should still be sent out for review) and for small changes, 
> normal mail should be good enough.

Basically yes. Single patches or very short series do not need to go
through git (just mails).

> And normally I rebase all my branches/patches to your devel branch.
> Or should I rebase them to the master branch?

As the devel branch could be rebased, it's not suitable as a starting
point. Even if you rebase it, I might not be able to pull it if
something changes in the meantime. As I don't process progs patches
every day, this is likely to happen.

If you don't want to do the git way now, no big deal. It's a long term
goal, but this requires to do the things right, so both parties,
develoeprs and maintainers, know what to expect and get it. I'll also
write that to wiki for future reference.