[PATCH v2 0/8] Add ioctl to support extended inode flags

[PATCH v2 0/8] Add ioctl to support extended inode flags

[David Sterba]

Changes for v2:
 - dropped renaming of variable from patch "btrfs: rename
   btrfs_flags_to_ioctl to reflect which flags it touches"
 - fixed the error handling in "btrfs: add FS_IOC_FSSETXATTR ioctl"
 - new patch to unify naming of some local variables


I'm about to add this patchset to the main queue for 4.18, unless there
are objections.


The patchset implements the existing VFS ioctls for reading extended
ioctl flags by btrfs.

There are many flags/attributes/extended/combined, the naming is
confusing, so let's recap what we have:

* generic VFS inode flags (i_flags)
  - S_* namespace
  https://elixir.bootlin.com/linux/v4.17-rc1/source/include/linux/fs.h#L1850
  - FS_IOC_GETFLAGS, FS_IOC_SETFLAGS
  - tools: lsatrr, chattr

* btrfs inode flags, on-disk format, independent of the above, with
  to/from conversions
  https://elixir.bootlin.com/linux/v4.17-rc1/source/fs/btrfs/ctree.h#L1416

* extended attributes, also called XATTR, but they're different entity,
  stored by an inode as key:value pairs
  - tools: getfattr, setfattr

* XFLAGs, another interface to the generic S_* flags, new ioctl added
  because [GS]ETFLAGS is frozen, new bits added, eg. for project quotas
  or DAX, and more that originate in XFS features
  https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fs.h#L168
  - tools: xfs_io -c lsattr, xfs_io -c chattr

In the future, btrfs will probably get:

- nodefrag -- eg. to disable autodefrag or defrag ioctl
- nosymlinks -- for directories, prevent creating new symlinks
- dax

git://github.com/kdave/btrfs-devel dev/xflags

David Sterba (8):
  btrfs: rename btrfs_update_iflags to reflect which flags it touches
  btrfs: rename btrfs_mask_flags to reflect which flags it touches
  btrfs: rename check_flags to reflect which flags it touches
  btrfs: rename btrfs_flags_to_ioctl to reflect which flags it touches
  btrfs: add helpers for FS_XFLAG_* conversion
  btrfs: add FS_IOC_FSGETXATTR ioctl
  btrfs: add FS_IOC_FSSETXATTR ioctl
  btrfs: unify naming of flags variables for SETFLAGS and XFLAGS

 fs/btrfs/ctree.h |   2 +-
 fs/btrfs/inode.c |   4 +-
 fs/btrfs/ioctl.c | 271 ++++++++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 213 insertions(+), 64 deletions(-)

-- 
2.16.2

[PATCH v2 1/8] btrfs: rename btrfs_update_iflags to reflect which flags it touches

[David Sterba]

The btrfs inode flag flavour is now simply called 'inode flags' and the
vfs inode are i_flags. Also rename the internal variable to something
less confusing than 'ip'.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ctree.h | 2 +-
 fs/btrfs/inode.c | 4 ++--
 fs/btrfs/ioctl.c | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2771cc56a622..d738eaa1d6e1 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3260,7 +3260,7 @@ void btrfs_test_inode_set_ops(struct inode *inode);
 long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 long btrfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 int btrfs_ioctl_get_supported_features(void __user *arg);
-void btrfs_update_iflags(struct inode *inode);
+void btrfs_sync_inode_flags_to_i_flags(struct inode *inode);
 int btrfs_is_empty_uuid(u8 *uuid);
 int btrfs_defrag_file(struct inode *inode, struct file *file,
 		      struct btrfs_ioctl_defrag_range_args *range,
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d241285a0d2a..c30afabcb6b0 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3924,7 +3924,7 @@ static int btrfs_read_locked_inode(struct inode *inode)
 		break;
 	}
 
-	btrfs_update_iflags(inode);
+	btrfs_sync_inode_flags_to_i_flags(inode);
 	return 0;
 
 make_bad:
@@ -6263,7 +6263,7 @@ static void btrfs_inherit_iflags(struct inode *inode, struct inode *dir)
 			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
 	}
 
-	btrfs_update_iflags(inode);
+	btrfs_sync_inode_flags_to_i_flags(inode);
 }
 
 static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 632e26d6f7ce..11edbdf3df7d 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -136,7 +136,7 @@ static unsigned int btrfs_flags_to_ioctl(unsigned int flags)
 /*
  * Update inode->i_flags based on the btrfs internal flags.
  */
-void btrfs_update_iflags(struct inode *inode)
+void btrfs_sync_inode_flags_to_i_flags(struct inode *inode)
 {
 	struct btrfs_inode *ip = BTRFS_I(inode);
 	unsigned int new_fl = 0;
@@ -317,7 +317,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		goto out_drop;
 	}
 
-	btrfs_update_iflags(inode);
+	btrfs_sync_inode_flags_to_i_flags(inode);
 	inode_inc_iversion(inode);
 	inode->i_ctime = current_time(inode);
 	ret = btrfs_update_inode(trans, root, inode);
-- 
2.16.2

[PATCH v2 2/8] btrfs: rename btrfs_mask_flags to reflect which flags it touches

[David Sterba]

The FS_*_FL flags cannot be easily identified by a variable name prefix
but we still need to recognize them so the 'fsflags' should be closer to
the naming scheme but again the 'fs' part sounds like it's a filesystem
flag. I don't have a better idea for now.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 11edbdf3df7d..e93dc3a6f554 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -93,11 +93,12 @@ static int btrfs_clone(struct inode *src, struct inode *inode,
 		       int no_time_update);
 
 /* Mask out flags that are inappropriate for the given type of inode. */
-static unsigned int btrfs_mask_flags(umode_t mode, unsigned int flags)
+static unsigned int btrfs_mask_fsflags_for_type(struct inode *inode,
+		unsigned int flags)
 {
-	if (S_ISDIR(mode))
+	if (S_ISDIR(inode->i_mode))
 		return flags;
-	else if (S_ISREG(mode))
+	else if (S_ISREG(inode->i_mode))
 		return flags & ~FS_DIRSYNC_FL;
 	else
 		return flags & (FS_NODUMP_FL | FS_NOATIME_FL);
@@ -218,7 +219,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	i_oldflags = inode->i_flags;
 	mode = inode->i_mode;
 
-	flags = btrfs_mask_flags(inode->i_mode, flags);
+	flags = btrfs_mask_fsflags_for_type(inode, flags);
 	oldflags = btrfs_flags_to_ioctl(ip->flags);
 	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
 		if (!capable(CAP_LINUX_IMMUTABLE)) {
-- 
2.16.2

[PATCH v2 3/8] btrfs: rename check_flags to reflect which flags it touches

[David Sterba]

The FS_*_FL flags cannot be easily identified by a prefix but we still
need to recognize them so the 'fsflags' should be closer to the naming
scheme but again the 'fs' part sounds like it's a filesystem flag. I
don't have a better idea for now.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index e93dc3a6f554..0b84f9e68f86 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -168,7 +168,8 @@ static int btrfs_ioctl_getflags(struct file *file, void __user *arg)
 	return 0;
 }
 
-static int check_flags(unsigned int flags)
+/* Check if @flags are a supported and valid set of FS_*_FL flags */
+static int check_fsflags(unsigned int flags)
 {
 	if (flags & ~(FS_IMMUTABLE_FL | FS_APPEND_FL | \
 		      FS_NOATIME_FL | FS_NODUMP_FL | \
@@ -205,7 +206,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	if (copy_from_user(&flags, arg, sizeof(flags)))
 		return -EFAULT;
 
-	ret = check_flags(flags);
+	ret = check_fsflags(flags);
 	if (ret)
 		return ret;
 
-- 
2.16.2

[PATCH v2 4/8] btrfs: rename btrfs_flags_to_ioctl to reflect which flags it touches

[David Sterba]

Converts btrfs_inode::flags to the FS_*_FL flags.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0b84f9e68f86..40eeb99310d5 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -105,9 +105,10 @@ static unsigned int btrfs_mask_fsflags_for_type(struct inode *inode,
 }
 
 /*
- * Export inode flags to the format expected by the FS_IOC_GETFLAGS ioctl.
+ * Export internal inode flags to the format expected by the FS_IOC_GETFLAGS
+ * ioctl.
  */
-static unsigned int btrfs_flags_to_ioctl(unsigned int flags)
+static unsigned int btrfs_inode_flags_to_fsflags(unsigned int flags)
 {
 	unsigned int iflags = 0;
 
@@ -161,7 +162,7 @@ void btrfs_sync_inode_flags_to_i_flags(struct inode *inode)
 static int btrfs_ioctl_getflags(struct file *file, void __user *arg)
 {
 	struct btrfs_inode *ip = BTRFS_I(file_inode(file));
-	unsigned int flags = btrfs_flags_to_ioctl(ip->flags);
+	unsigned int flags = btrfs_inode_flags_to_fsflags(ip->flags);
 
 	if (copy_to_user(arg, &flags, sizeof(flags)))
 		return -EFAULT;
@@ -221,7 +222,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	mode = inode->i_mode;
 
 	flags = btrfs_mask_fsflags_for_type(inode, flags);
-	oldflags = btrfs_flags_to_ioctl(ip->flags);
+	oldflags = btrfs_inode_flags_to_fsflags(ip->flags);
 	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
 		if (!capable(CAP_LINUX_IMMUTABLE)) {
 			ret = -EPERM;
-- 
2.16.2

[PATCH v2 5/8] btrfs: add helpers for FS_XFLAG_* conversion

[David Sterba]

Preparatory work for the FS_IOC_FSGETXATTR ioctl, basic conversions and
checking helpers.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 40eeb99310d5..54dfcb851ec2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -338,6 +338,38 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	return ret;
 }
 
+/*
+ * Translate btrfs internal inode flags to xflags as expected by the
+ * FS_IOC_FSGETXATT ioctl. Filter only the supported ones, unknown flags are
+ * silently dropped.
+ */
+static unsigned int btrfs_inode_flags_to_xflags(unsigned int flags)
+{
+	unsigned int xflags = 0;
+
+	if (flags & BTRFS_INODE_APPEND)
+		xflags |= FS_XFLAG_APPEND;
+	if (flags & BTRFS_INODE_IMMUTABLE)
+		xflags |= FS_XFLAG_IMMUTABLE;
+	if (flags & BTRFS_INODE_NOATIME)
+		xflags |= FS_XFLAG_NOATIME;
+	if (flags & BTRFS_INODE_NODUMP)
+		xflags |= FS_XFLAG_NODUMP;
+	if (flags & BTRFS_INODE_SYNC)
+		xflags |= FS_XFLAG_SYNC;
+
+	return xflags;
+}
+
+/* Check if @flags are a supported and valid set of FS_XFLAGS_* flags */
+static int check_xflags(unsigned int flags)
+{
+	if (flags & ~(FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE | FS_XFLAG_NOATIME |
+		      FS_XFLAG_NODUMP | FS_XFLAG_SYNC))
+		return -EOPNOTSUPP;
+	return 0;
+}
+
 static int btrfs_ioctl_getversion(struct file *file, int __user *arg)
 {
 	struct inode *inode = file_inode(file);
-- 
2.16.2

[PATCH v2 6/8] btrfs: add FS_IOC_FSGETXATTR ioctl

[David Sterba]

The new ioctl is an extension to the FS_IOC_GETFLAGS and adds new
flags and is extensible. This patch allows to return the xflags portion
of the fsxattr structure, other items have no meaning for btrfs or can
be added later.

The original patch was written by Chandan Jay Sharma but was incomplete
and no further revision has been sent. Several cleanups were necessary
to avoid confusion with other ioctls, as we have another flavor of
flags.

Based-on-patches-by: Chandan Jay Sharma <chandansbg@gmail.com>
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 54dfcb851ec2..a3bb59ac5666 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -370,6 +370,24 @@ static int check_xflags(unsigned int flags)
 	return 0;
 }
 
+/*
+ * Set the xflags from the internal inode flags. The remaining items of fsxattr
+ * are zeroed.
+ */
+static int btrfs_ioctl_fsgetxattr(struct file *file, void __user *arg)
+{
+	struct btrfs_inode *binode = BTRFS_I(file_inode(file));
+	struct fsxattr fa;
+
+	memset(&fa, 0, sizeof(fa));
+	fa.fsx_xflags = btrfs_inode_flags_to_xflags(binode->flags);
+
+	if (copy_to_user(arg, &fa, sizeof(fa)))
+		return -EFAULT;
+
+	return 0;
+}
+
 static int btrfs_ioctl_getversion(struct file *file, int __user *arg)
 {
 	struct inode *inode = file_inode(file);
@@ -5600,6 +5618,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_get_features(file, argp);
 	case BTRFS_IOC_SET_FEATURES:
 		return btrfs_ioctl_set_features(file, argp);
+	case FS_IOC_FSGETXATTR:
+		return btrfs_ioctl_fsgetxattr(file, argp);
 	}
 
 	return -ENOTTY;
-- 
2.16.2

[PATCH v2 7/8] btrfs: add FS_IOC_FSSETXATTR ioctl

[David Sterba]

The new ioctl is an extension to the FS_IOC_SETFLAGS and adds new
flags and is extensible. Don't get fooled by the XATTR in the name, it
does not have anything in common with the extended attributes,
incidentally also abbreviated as XATTRs.

This patch allows to set the xflags portion of the fsxattr structure,
other items have no meaning and non-zero values will result in
EOPNOTSUPP.

Currently supported xflags:

- APPEND
- IMMUTABLE
- NOATIME
- NODUMP
- SYNC

The structure of btrfs_ioctl_fssetxattr copies btrfs_ioctl_setflags but
is simpler on the flag setting side.

The original patch was written by Chandan Jay Sharma but was incomplete
and no further revision has been sent.

Based-on-patches-by: Chandan Jay Sharma <chandansbg@gmail.com>
Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a3bb59ac5666..838a7a80e6b0 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -388,6 +388,98 @@ static int btrfs_ioctl_fsgetxattr(struct file *file, void __user *arg)
 	return 0;
 }
 
+static int btrfs_ioctl_fssetxattr(struct file *file, void __user *arg)
+{
+	struct inode *inode = file_inode(file);
+	struct btrfs_inode *binode = BTRFS_I(inode);
+	struct btrfs_root *root = binode->root;
+	struct btrfs_trans_handle *trans;
+	struct fsxattr fa;
+	unsigned old_flags;
+	unsigned old_i_flags;
+	int ret = 0;
+
+	if (!inode_owner_or_capable(inode))
+		return -EPERM;
+
+	if (btrfs_root_readonly(root))
+		return -EROFS;
+
+	memset(&fa, 0, sizeof(fa));
+	if (copy_from_user(&fa, arg, sizeof(fa)))
+		return -EFAULT;
+
+	ret = check_xflags(fa.fsx_xflags);
+	if (ret)
+		return ret;
+
+	if (fa.fsx_extsize != 0 || fa.fsx_projid != 0 || fa.fsx_cowextsize != 0)
+		return -EOPNOTSUPP;
+
+	ret = mnt_want_write_file(file);
+	if (ret)
+		return ret;
+
+	inode_lock(inode);
+
+	old_flags = binode->flags;
+	old_i_flags = inode->i_flags;
+
+	/* We need the capabilities to change append-only or immutable inode */
+	if (((old_flags & (BTRFS_INODE_APPEND | BTRFS_INODE_IMMUTABLE)) ||
+	     (fa.fsx_xflags & (FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE))) &&
+	    !capable(CAP_LINUX_IMMUTABLE)) {
+		ret = -EPERM;
+		goto out_unlock;
+	}
+
+	if (fa.fsx_xflags & FS_XFLAG_SYNC)
+		binode->flags |= BTRFS_INODE_SYNC;
+	else
+		binode->flags &= ~BTRFS_INODE_SYNC;
+	if (fa.fsx_xflags & FS_XFLAG_IMMUTABLE)
+		binode->flags |= BTRFS_INODE_IMMUTABLE;
+	else
+		binode->flags &= ~BTRFS_INODE_IMMUTABLE;
+	if (fa.fsx_xflags & FS_XFLAG_APPEND)
+		binode->flags |= BTRFS_INODE_APPEND;
+	else
+		binode->flags &= ~BTRFS_INODE_APPEND;
+	if (fa.fsx_xflags & FS_XFLAG_NODUMP)
+		binode->flags |= BTRFS_INODE_NODUMP;
+	else
+		binode->flags &= ~BTRFS_INODE_NODUMP;
+	if (fa.fsx_xflags & FS_XFLAG_NOATIME)
+		binode->flags |= BTRFS_INODE_NOATIME;
+	else
+		binode->flags &= ~BTRFS_INODE_NOATIME;
+
+	/* 1 item for the inode */
+	trans = btrfs_start_transaction(root, 1);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto out_unlock;
+	}
+
+	btrfs_sync_inode_flags_to_i_flags(inode);
+	inode_inc_iversion(inode);
+	inode->i_ctime = current_time(inode);
+	ret = btrfs_update_inode(trans, root, inode);
+
+	btrfs_end_transaction(trans);
+
+out_unlock:
+	if (ret) {
+		binode->flags = old_flags;
+		inode->i_flags = old_i_flags;
+	}
+
+	inode_unlock(inode);
+	mnt_drop_write_file(file);
+
+	return ret;
+}
+
 static int btrfs_ioctl_getversion(struct file *file, int __user *arg)
 {
 	struct inode *inode = file_inode(file);
@@ -5620,6 +5712,8 @@ long btrfs_ioctl(struct file *file, unsigned int
 		return btrfs_ioctl_set_features(file, argp);
 	case FS_IOC_FSGETXATTR:
 		return btrfs_ioctl_fsgetxattr(file, argp);
+	case FS_IOC_FSSETXATTR:
+		return btrfs_ioctl_fssetxattr(file, argp);
 	}
 
 	return -ENOTTY;
-- 
2.16.2

[PATCH v2 8/8] btrfs: unify naming of flags variables for SETFLAGS and XFLAGS

[David Sterba]

* The simple 'flags' refer to the btrfs inode
* ... that's in 'binode
* the FS_*_FL variables are 'fsflags'
* the old copies of the variable are prefixed by 'old_'
* Struct inode flags contain 'i_flags'.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 106 +++++++++++++++++++++++++++----------------------------
 1 file changed, 53 insertions(+), 53 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 838a7a80e6b0..c1904d9d99e2 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -140,18 +140,18 @@ static unsigned int btrfs_inode_flags_to_fsflags(unsigned int flags)
  */
 void btrfs_sync_inode_flags_to_i_flags(struct inode *inode)
 {
-	struct btrfs_inode *ip = BTRFS_I(inode);
+	struct btrfs_inode *binode = BTRFS_I(inode);
 	unsigned int new_fl = 0;
 
-	if (ip->flags & BTRFS_INODE_SYNC)
+	if (binode->flags & BTRFS_INODE_SYNC)
 		new_fl |= S_SYNC;
-	if (ip->flags & BTRFS_INODE_IMMUTABLE)
+	if (binode->flags & BTRFS_INODE_IMMUTABLE)
 		new_fl |= S_IMMUTABLE;
-	if (ip->flags & BTRFS_INODE_APPEND)
+	if (binode->flags & BTRFS_INODE_APPEND)
 		new_fl |= S_APPEND;
-	if (ip->flags & BTRFS_INODE_NOATIME)
+	if (binode->flags & BTRFS_INODE_NOATIME)
 		new_fl |= S_NOATIME;
-	if (ip->flags & BTRFS_INODE_DIRSYNC)
+	if (binode->flags & BTRFS_INODE_DIRSYNC)
 		new_fl |= S_DIRSYNC;
 
 	set_mask_bits(&inode->i_flags,
@@ -161,8 +161,8 @@ void btrfs_sync_inode_flags_to_i_flags(struct inode *inode)
 
 static int btrfs_ioctl_getflags(struct file *file, void __user *arg)
 {
-	struct btrfs_inode *ip = BTRFS_I(file_inode(file));
-	unsigned int flags = btrfs_inode_flags_to_fsflags(ip->flags);
+	struct btrfs_inode *binode = BTRFS_I(file_inode(file));
+	unsigned int flags = btrfs_inode_flags_to_fsflags(binode->flags);
 
 	if (copy_to_user(arg, &flags, sizeof(flags)))
 		return -EFAULT;
@@ -189,13 +189,13 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 {
 	struct inode *inode = file_inode(file);
 	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
-	struct btrfs_inode *ip = BTRFS_I(inode);
-	struct btrfs_root *root = ip->root;
+	struct btrfs_inode *binode = BTRFS_I(inode);
+	struct btrfs_root *root = binode->root;
 	struct btrfs_trans_handle *trans;
-	unsigned int flags, oldflags;
+	unsigned int fsflags, old_fsflags;
 	int ret;
-	u64 ip_oldflags;
-	unsigned int i_oldflags;
+	u64 old_flags;
+	unsigned int old_i_flags;
 	umode_t mode;
 
 	if (!inode_owner_or_capable(inode))
@@ -204,10 +204,10 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	if (btrfs_root_readonly(root))
 		return -EROFS;
 
-	if (copy_from_user(&flags, arg, sizeof(flags)))
+	if (copy_from_user(&fsflags, arg, sizeof(fsflags)))
 		return -EFAULT;
 
-	ret = check_fsflags(flags);
+	ret = check_fsflags(fsflags);
 	if (ret)
 		return ret;
 
@@ -217,44 +217,44 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 
 	inode_lock(inode);
 
-	ip_oldflags = ip->flags;
-	i_oldflags = inode->i_flags;
+	old_flags = binode->flags;
+	old_i_flags = inode->i_flags;
 	mode = inode->i_mode;
 
-	flags = btrfs_mask_fsflags_for_type(inode, flags);
-	oldflags = btrfs_inode_flags_to_fsflags(ip->flags);
-	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
+	fsflags = btrfs_mask_fsflags_for_type(inode, fsflags);
+	old_fsflags = btrfs_inode_flags_to_fsflags(binode->flags);
+	if ((fsflags ^ old_fsflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
 		if (!capable(CAP_LINUX_IMMUTABLE)) {
 			ret = -EPERM;
 			goto out_unlock;
 		}
 	}
 
-	if (flags & FS_SYNC_FL)
-		ip->flags |= BTRFS_INODE_SYNC;
+	if (fsflags & FS_SYNC_FL)
+		binode->flags |= BTRFS_INODE_SYNC;
 	else
-		ip->flags &= ~BTRFS_INODE_SYNC;
-	if (flags & FS_IMMUTABLE_FL)
-		ip->flags |= BTRFS_INODE_IMMUTABLE;
+		binode->flags &= ~BTRFS_INODE_SYNC;
+	if (fsflags & FS_IMMUTABLE_FL)
+		binode->flags |= BTRFS_INODE_IMMUTABLE;
 	else
-		ip->flags &= ~BTRFS_INODE_IMMUTABLE;
-	if (flags & FS_APPEND_FL)
-		ip->flags |= BTRFS_INODE_APPEND;
+		binode->flags &= ~BTRFS_INODE_IMMUTABLE;
+	if (fsflags & FS_APPEND_FL)
+		binode->flags |= BTRFS_INODE_APPEND;
 	else
-		ip->flags &= ~BTRFS_INODE_APPEND;
-	if (flags & FS_NODUMP_FL)
-		ip->flags |= BTRFS_INODE_NODUMP;
+		binode->flags &= ~BTRFS_INODE_APPEND;
+	if (fsflags & FS_NODUMP_FL)
+		binode->flags |= BTRFS_INODE_NODUMP;
 	else
-		ip->flags &= ~BTRFS_INODE_NODUMP;
-	if (flags & FS_NOATIME_FL)
-		ip->flags |= BTRFS_INODE_NOATIME;
+		binode->flags &= ~BTRFS_INODE_NODUMP;
+	if (fsflags & FS_NOATIME_FL)
+		binode->flags |= BTRFS_INODE_NOATIME;
 	else
-		ip->flags &= ~BTRFS_INODE_NOATIME;
-	if (flags & FS_DIRSYNC_FL)
-		ip->flags |= BTRFS_INODE_DIRSYNC;
+		binode->flags &= ~BTRFS_INODE_NOATIME;
+	if (fsflags & FS_DIRSYNC_FL)
+		binode->flags |= BTRFS_INODE_DIRSYNC;
 	else
-		ip->flags &= ~BTRFS_INODE_DIRSYNC;
-	if (flags & FS_NOCOW_FL) {
+		binode->flags &= ~BTRFS_INODE_DIRSYNC;
+	if (fsflags & FS_NOCOW_FL) {
 		if (S_ISREG(mode)) {
 			/*
 			 * It's safe to turn csums off here, no extents exist.
@@ -262,10 +262,10 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 			 * status of the file and will not set it.
 			 */
 			if (inode->i_size == 0)
-				ip->flags |= BTRFS_INODE_NODATACOW
-					   | BTRFS_INODE_NODATASUM;
+				binode->flags |= BTRFS_INODE_NODATACOW
+					      | BTRFS_INODE_NODATASUM;
 		} else {
-			ip->flags |= BTRFS_INODE_NODATACOW;
+			binode->flags |= BTRFS_INODE_NODATACOW;
 		}
 	} else {
 		/*
@@ -273,10 +273,10 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		 */
 		if (S_ISREG(mode)) {
 			if (inode->i_size == 0)
-				ip->flags &= ~(BTRFS_INODE_NODATACOW
+				binode->flags &= ~(BTRFS_INODE_NODATACOW
 				             | BTRFS_INODE_NODATASUM);
 		} else {
-			ip->flags &= ~BTRFS_INODE_NODATACOW;
+			binode->flags &= ~BTRFS_INODE_NODATACOW;
 		}
 	}
 
@@ -285,18 +285,18 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	 * flag may be changed automatically if compression code won't make
 	 * things smaller.
 	 */
-	if (flags & FS_NOCOMP_FL) {
-		ip->flags &= ~BTRFS_INODE_COMPRESS;
-		ip->flags |= BTRFS_INODE_NOCOMPRESS;
+	if (fsflags & FS_NOCOMP_FL) {
+		binode->flags &= ~BTRFS_INODE_COMPRESS;
+		binode->flags |= BTRFS_INODE_NOCOMPRESS;
 
 		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
 		if (ret && ret != -ENODATA)
 			goto out_drop;
-	} else if (flags & FS_COMPR_FL) {
+	} else if (fsflags & FS_COMPR_FL) {
 		const char *comp;
 
-		ip->flags |= BTRFS_INODE_COMPRESS;
-		ip->flags &= ~BTRFS_INODE_NOCOMPRESS;
+		binode->flags |= BTRFS_INODE_COMPRESS;
+		binode->flags &= ~BTRFS_INODE_NOCOMPRESS;
 
 		comp = btrfs_compress_type2str(fs_info->compress_type);
 		if (!comp || comp[0] == 0)
@@ -311,7 +311,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
 		if (ret && ret != -ENODATA)
 			goto out_drop;
-		ip->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
+		binode->flags &= ~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
 	}
 
 	trans = btrfs_start_transaction(root, 1);
@@ -328,8 +328,8 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	btrfs_end_transaction(trans);
  out_drop:
 	if (ret) {
-		ip->flags = ip_oldflags;
-		inode->i_flags = i_oldflags;
+		binode->flags = old_flags;
+		inode->i_flags = old_i_flags;
 	}
 
  out_unlock:
-- 
2.16.2

Re: [PATCH v2 1/8] btrfs: rename btrfs_update_iflags to reflect which flags it touches

[Nikolay Borisov]

On  9.05.2018 16:54, David Sterba wrote:
> The btrfs inode flag flavour is now simply called 'inode flags' and the
> vfs inode are i_flags. Also rename the internal variable to something
> less confusing than 'ip'.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ctree.h | 2 +-
>  fs/btrfs/inode.c | 4 ++--
>  fs/btrfs/ioctl.c | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 2771cc56a622..d738eaa1d6e1 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3260,7 +3260,7 @@ void btrfs_test_inode_set_ops(struct inode *inode);
>  long btrfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
>  long btrfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
>  int btrfs_ioctl_get_supported_features(void __user *arg);
> -void btrfs_update_iflags(struct inode *inode);
> +void btrfs_sync_inode_flags_to_i_flags(struct inode *inode);
>  int btrfs_is_empty_uuid(u8 *uuid);
>  int btrfs_defrag_file(struct inode *inode, struct file *file,
>  		      struct btrfs_ioctl_defrag_range_args *range,
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index d241285a0d2a..c30afabcb6b0 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3924,7 +3924,7 @@ static int btrfs_read_locked_inode(struct inode *inode)
>  		break;
>  	}
>  
> -	btrfs_update_iflags(inode);
> +	btrfs_sync_inode_flags_to_i_flags(inode);
>  	return 0;
>  
>  make_bad:
> @@ -6263,7 +6263,7 @@ static void btrfs_inherit_iflags(struct inode *inode, struct inode *dir)
>  			BTRFS_I(inode)->flags |= BTRFS_INODE_NODATASUM;
>  	}
>  
> -	btrfs_update_iflags(inode);
> +	btrfs_sync_inode_flags_to_i_flags(inode);
>  }
>  
>  static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 632e26d6f7ce..11edbdf3df7d 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -136,7 +136,7 @@ static unsigned int btrfs_flags_to_ioctl(unsigned int flags)
>  /*
>   * Update inode->i_flags based on the btrfs internal flags.
>   */
> -void btrfs_update_iflags(struct inode *inode)
> +void btrfs_sync_inode_flags_to_i_flags(struct inode *inode)
>  {

nit: imho that name is too verbose, I think btrfs_sync_inode_flags
should suffice alongside the one line comment at the top of the
function. As a matter of fact what necessitates duplicating those flags
in both the btrfs-specific data structure and the generic vfs inode?

So looking at how btrfs_update_iflags is used, we have:

btrfs_ioctl_setflags - we gain nothing by having those flags duplicated
since in case of failure the revert procedure is the same - we have to
set i_oldflags to both btrfs_inode and vfs_inode

btrfs_read_locked_inode - so here we read the flags from the on-disk
inode and then use btrfs_update_iflags to set the generic vfs flags to
the vfs_inode

btrfs_inherit_iflags - the call to btrfs_update_iflags is not even
necessary since none of the flags which is being inherited:
BTRFS_INODE_NOCOMPRESS/BTRFS_INODE_COMPRESS/BTRFS_INODE_NODATACOW
is supported by btrfs_update_iflags.

Given this I'd be more inclined to rename the function to something
like: btrfs_init_i_flags/btrfs_set_iflags or some such.

And to go a bit further imho those flags which are generic should always
be set in the vfs_inode and when we are about to persist an inode item
on-disk then we should get the necessary flags from the vfs_inode and
put in the inode item.

>  	struct btrfs_inode *ip = BTRFS_I(inode);
>  	unsigned int new_fl = 0;
> @@ -317,7 +317,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  		goto out_drop;
>  	}
>  
> -	btrfs_update_iflags(inode);
> +	btrfs_sync_inode_flags_to_i_flags(inode);
>  	inode_inc_iversion(inode);
>  	inode->i_ctime = current_time(inode);
>  	ret = btrfs_update_inode(trans, root, inode);
> 

Re: [PATCH v2 2/8] btrfs: rename btrfs_mask_flags to reflect which flags it touches

[Nikolay Borisov]

On  9.05.2018 16:54, David Sterba wrote:
> The FS_*_FL flags cannot be easily identified by a variable name prefix
> but we still need to recognize them so the 'fsflags' should be closer to
> the naming scheme but again the 'fs' part sounds like it's a filesystem
> flag. I don't have a better idea for now.

Why not using iflags, my reasoning is that the official documentation of
those flags: http://man7.org/linux/man-pages/man2/ioctl_iflags.2.html
refers to them as "inode flags" hence iflags or the slightly longer
inode_flags?

> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ioctl.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 11edbdf3df7d..e93dc3a6f554 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -93,11 +93,12 @@ static int btrfs_clone(struct inode *src, struct inode *inode,
>  		       int no_time_update);
>  
>  /* Mask out flags that are inappropriate for the given type of inode. */
> -static unsigned int btrfs_mask_flags(umode_t mode, unsigned int flags)
> +static unsigned int btrfs_mask_fsflags_for_type(struct inode *inode,
> +		unsigned int flags)
>  {
> -	if (S_ISDIR(mode))
> +	if (S_ISDIR(inode->i_mode))
>  		return flags;
> -	else if (S_ISREG(mode))
> +	else if (S_ISREG(inode->i_mode))
>  		return flags & ~FS_DIRSYNC_FL;
>  	else
>  		return flags & (FS_NODUMP_FL | FS_NOATIME_FL);
> @@ -218,7 +219,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	i_oldflags = inode->i_flags;
>  	mode = inode->i_mode;
>  
> -	flags = btrfs_mask_flags(inode->i_mode, flags);
> +	flags = btrfs_mask_fsflags_for_type(inode, flags);
>  	oldflags = btrfs_flags_to_ioctl(ip->flags);
>  	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
>  		if (!capable(CAP_LINUX_IMMUTABLE)) {
> 

Re: [PATCH v2 4/8] btrfs: rename btrfs_flags_to_ioctl to reflect which flags it touches

[Nikolay Borisov]

On  9.05.2018 16:54, David Sterba wrote:
> Converts btrfs_inode::flags to the FS_*_FL flags.

What about :

btrfs_to_iflags(struct btrfs_inode *inode) or

BTRFS_IFLAGS(ip) (macro) or

btrfs_iflags(ip) (function).

I'm more inclined towards one of the shorter function/macro variants.

> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ioctl.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0b84f9e68f86..40eeb99310d5 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -105,9 +105,10 @@ static unsigned int btrfs_mask_fsflags_for_type(struct inode *inode,
>  }
>  
>  /*
> - * Export inode flags to the format expected by the FS_IOC_GETFLAGS ioctl.
> + * Export internal inode flags to the format expected by the FS_IOC_GETFLAGS
> + * ioctl.
>   */
> -static unsigned int btrfs_flags_to_ioctl(unsigned int flags)
> +static unsigned int btrfs_inode_flags_to_fsflags(unsigned int flags)
>  {
>  	unsigned int iflags = 0;
>  
> @@ -161,7 +162,7 @@ void btrfs_sync_inode_flags_to_i_flags(struct inode *inode)
>  static int btrfs_ioctl_getflags(struct file *file, void __user *arg)
>  {
>  	struct btrfs_inode *ip = BTRFS_I(file_inode(file));
> -	unsigned int flags = btrfs_flags_to_ioctl(ip->flags);
> +	unsigned int flags = btrfs_inode_flags_to_fsflags(ip->flags);
>  
>  	if (copy_to_user(arg, &flags, sizeof(flags)))
>  		return -EFAULT;
> @@ -221,7 +222,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	mode = inode->i_mode;
>  
>  	flags = btrfs_mask_fsflags_for_type(inode, flags);
> -	oldflags = btrfs_flags_to_ioctl(ip->flags);
> +	oldflags = btrfs_inode_flags_to_fsflags(ip->flags);
>  	if ((flags ^ oldflags) & (FS_APPEND_FL | FS_IMMUTABLE_FL)) {
>  		if (!capable(CAP_LINUX_IMMUTABLE)) {
>  			ret = -EPERM;
> 

Re: [PATCH v2 5/8] btrfs: add helpers for FS_XFLAG_* conversion

[Nikolay Borisov]

On  9.05.2018 16:54, David Sterba wrote:
> Preparatory work for the FS_IOC_FSGETXATTR ioctl, basic conversions and
> checking helpers.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 40eeb99310d5..54dfcb851ec2 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -338,6 +338,38 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
>  	return ret;
>  }
>  
> +/*
> + * Translate btrfs internal inode flags to xflags as expected by the
> + * FS_IOC_FSGETXATT ioctl. Filter only the supported ones, unknown flags are
> + * silently dropped.
> + */
> +static unsigned int btrfs_inode_flags_to_xflags(unsigned int flags)
> +{
> +	unsigned int xflags = 0;
> +
> +	if (flags & BTRFS_INODE_APPEND)
> +		xflags |= FS_XFLAG_APPEND;
> +	if (flags & BTRFS_INODE_IMMUTABLE)
> +		xflags |= FS_XFLAG_IMMUTABLE;
> +	if (flags & BTRFS_INODE_NOATIME)
> +		xflags |= FS_XFLAG_NOATIME;
> +	if (flags & BTRFS_INODE_NODUMP)
> +		xflags |= FS_XFLAG_NODUMP;
> +	if (flags & BTRFS_INODE_SYNC)
> +		xflags |= FS_XFLAG_SYNC;
> +
> +	return xflags;
> +}

same suggestion as previous patch:

BTRFS_XFLAGS(ip)/btrfs_xflags(ip) (macro/function variants). Imho it
makes sense for such function to actually take an inode. Otherwise we
are exposing an implementation detail how we derive the iflags/xflags
from an inode - i.e query the ->flags field. Let's keep this
encapsulated behind whatever interface we decide.

> +
> +/* Check if @flags are a supported and valid set of FS_XFLAGS_* flags */
> +static int check_xflags(unsigned int flags)
> +{
> +	if (flags & ~(FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE | FS_XFLAG_NOATIME |
> +		      FS_XFLAG_NODUMP | FS_XFLAG_SYNC))
> +		return -EOPNOTSUPP;
> +	return 0;
> +}
> +
>  static int btrfs_ioctl_getversion(struct file *file, int __user *arg)
>  {
>  	struct inode *inode = file_inode(file);
> 

Re: [PATCH v2 1/8] btrfs: rename btrfs_update_iflags to reflect which flags it touches

[David Sterba]

On Thu, May 10, 2018 at 09:34:21AM +0300, Nikolay Borisov wrote:
> > @@ -136,7 +136,7 @@ static unsigned int btrfs_flags_to_ioctl(unsigned int flags)
> >  /*
> >   * Update inode->i_flags based on the btrfs internal flags.
> >   */
> > -void btrfs_update_iflags(struct inode *inode)
> > +void btrfs_sync_inode_flags_to_i_flags(struct inode *inode)
> >  {
> 
> nit: imho that name is too verbose,

That's intentional to be descriptive enough so reading the comment at
the functions is not necessary.

> I think btrfs_sync_inode_flags
> should suffice alongside the one line comment at the top of the
> function.

There are too many flavors of inodes, which direction would
'btrfs_sync_inode_flags' sync?

> As a matter of fact what necessitates duplicating those flags
> in both the btrfs-specific data structure and the generic vfs inode?

Because btrfs inode flags are part of the on-disk format while the VFS
inode flags are in-memory and can change any time.

> So looking at how btrfs_update_iflags is used, we have:
> 
> btrfs_ioctl_setflags - we gain nothing by having those flags duplicated
> since in case of failure the revert procedure is the same - we have to
> set i_oldflags to both btrfs_inode and vfs_inode
> 
> btrfs_read_locked_inode - so here we read the flags from the on-disk
> inode and then use btrfs_update_iflags to set the generic vfs flags to
> the vfs_inode
> 
> btrfs_inherit_iflags - the call to btrfs_update_iflags is not even
> necessary since none of the flags which is being inherited:
> BTRFS_INODE_NOCOMPRESS/BTRFS_INODE_COMPRESS/BTRFS_INODE_NODATACOW
> is supported by btrfs_update_iflags.
> 
> Given this I'd be more inclined to rename the function to something
> like: btrfs_init_i_flags/btrfs_set_iflags or some such.
> 
> And to go a bit further imho those flags which are generic should always
> be set in the vfs_inode and when we are about to persist an inode item
> on-disk then we should get the necessary flags from the vfs_inode and
> put in the inode item.

Not all btrfs flags have the counterpart in vfs inode flags and vice
versa, they're different namespace and for that we need the conversion
helpers.

Re: [PATCH v2 2/8] btrfs: rename btrfs_mask_flags to reflect which flags it touches

[David Sterba]

On Thu, May 10, 2018 at 09:38:12AM +0300, Nikolay Borisov wrote:
> On  9.05.2018 16:54, David Sterba wrote:
> > The FS_*_FL flags cannot be easily identified by a variable name prefix
> > but we still need to recognize them so the 'fsflags' should be closer to
> > the naming scheme but again the 'fs' part sounds like it's a filesystem
> > flag. I don't have a better idea for now.
> 
> Why not using iflags, my reasoning is that the official documentation of
> those flags: http://man7.org/linux/man-pages/man2/ioctl_iflags.2.html
> refers to them as "inode flags" hence iflags or the slightly longer
> inode_flags?

The first patch tries to capture the inode flags name for the btrfs
inode ... "The btrfs inode flag flavour is now simply called 'inode
flags'"

The manual page talks only about one ioctl and namespace, but in the
code we have to deal with several. I'm not happy about the naming, but
at least it's obvious which namespace is used.

Re: [PATCH v2 5/8] btrfs: add helpers for FS_XFLAG_* conversion

[David Sterba]

On Thu, May 10, 2018 at 09:44:23AM +0300, Nikolay Borisov wrote:
> 
> 
> On  9.05.2018 16:54, David Sterba wrote:
> > Preparatory work for the FS_IOC_FSGETXATTR ioctl, basic conversions and
> > checking helpers.
> > 
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >  fs/btrfs/ioctl.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 40eeb99310d5..54dfcb851ec2 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -338,6 +338,38 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
> >  	return ret;
> >  }
> >  
> > +/*
> > + * Translate btrfs internal inode flags to xflags as expected by the
> > + * FS_IOC_FSGETXATT ioctl. Filter only the supported ones, unknown flags are
> > + * silently dropped.
> > + */
> > +static unsigned int btrfs_inode_flags_to_xflags(unsigned int flags)
> > +{
> > +	unsigned int xflags = 0;
> > +
> > +	if (flags & BTRFS_INODE_APPEND)
> > +		xflags |= FS_XFLAG_APPEND;
> > +	if (flags & BTRFS_INODE_IMMUTABLE)
> > +		xflags |= FS_XFLAG_IMMUTABLE;
> > +	if (flags & BTRFS_INODE_NOATIME)
> > +		xflags |= FS_XFLAG_NOATIME;
> > +	if (flags & BTRFS_INODE_NODUMP)
> > +		xflags |= FS_XFLAG_NODUMP;
> > +	if (flags & BTRFS_INODE_SYNC)
> > +		xflags |= FS_XFLAG_SYNC;
> > +
> > +	return xflags;
> > +}
> 
> same suggestion as previous patch:
> 
> BTRFS_XFLAGS(ip)/btrfs_xflags(ip) (macro/function variants). Imho it
> makes sense for such function to actually take an inode. Otherwise we
> are exposing an implementation detail how we derive the iflags/xflags
> from an inode - i.e query the ->flags field. Let's keep this
> encapsulated behind whatever interface we decide.

So the inode as argument does not sound bad, I'd rather do it as a
separate patch as I don't see how to squash the prototype change into
any of the patches. The helper btrfs_inode_flags_to_xflags now follows
the style of the other helpers.