Skip to contentSkip to conversation

btrfs: correct empty compression property behavior

PATCH In review Satoru Takeuchi 3 participants 11 messages Sep 19, 2014 – Oct 16, 2014 Btrfs file system

lore
Share & tools
lore

Satoru Takeuchi

author
#0 lore raw

PATCH 1/4 btrfs: correct empty compression property behavior

From: Naohiro Aota <[email protected]>

In the current implementation, compression property == "" has the two different meanings: one is with BTRFS_INODE_NOCOMPRESS, and the other is without this flag.

So, even if the two files a and b have the same compression property, "", and the same contents, one file seems to be compressed and the other is not. It's difficult to understand for users and also confuses them.

Here is the real example. Let assume the following two cases.

a) A file created freshly (under a directory without both COMPRESS and NOCOMPRESS flag.)

b) A existing file which is explicitly set ""
to compression property.

In addition, here is the command log (I attached the source of "getflags" program in this patch.)

=======

$ rm -f a b; touch a b
$ btrfs prop set b compression ""
 # both a and b have the same compression property: ""
$ btrfs prop get a compression
$ btrfs prop get b compression
 # but ... let's take a look at inode flags
$ ./getflags a
0x0
$ ./getflags b
0x400             # 0x400 (FS_NOCOMP_FL) corresponds to BTRFS_INODE_NOCOMPRESS
=======

So both these two files have their compression property == "", but have different NOCOMPRESS flag state leading to different behavior.

case | BTRFS_INODE_NOCOMPRESS | behavior
=====+========================+=========================

   a | unset                  | might be compressed
   b | set                    | never be compressed

I consider that we should not expect users to remember whether their files are case a or b and should introduce another value for compress property anyway.

getflags.c:
===================
#include <sys/ioctl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
#include <linux/fs.h>

int main(int argc, char const* argv[])
{
const char *name = argv[1];
int fd = open(name, O_RDONLY);
long x;
ioctl(fd, FS_IOC_GETFLAGS, &x);
printf("0x%lx\n", x);
return 0;
}
===================

Signed-off-by Naohiro Aota <[email protected]>
Signed-off-by Satoru Takeuchi <[email protected]>
---
 fs/btrfs/props.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/btrfs/props.c +2 -2
index 129b1dd..bf005f4 100644
@@ -393,8 +393,8 @@ static int prop_compression_apply(struct inode *inode,
 	int type;
 
 	if (len == 0) {
-		BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
-		BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
+		BTRFS_I(inode)->flags &=
+			~(BTRFS_INODE_COMPRESS | BTRFS_INODE_NOCOMPRESS);
 		BTRFS_I(inode)->force_compress = BTRFS_COMPRESS_NONE;
 
 		return 0;

-- 1.8.3.1

author
#1 lore raw

PATCH 2/4 btrfs: introduce new compression property to disable compression at all

From: Naohiro Aota <[email protected]>

This new compression property, "off", to disable compression of the file at all.

Signed-off-by Naohiro Aota <[email protected]>
Signed-off-by Satoru Takeuchi <[email protected]>
---
 fs/btrfs/props.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
fs/btrfs/props.c +13 -0
index bf005f4..38efbe1 100644
@@ -382,6 +382,8 @@ static int prop_compression_validate(const char *value, size_t len)
 		return 0;
 	else if (!strncmp("zlib", value, len))
 		return 0;
+	else if (!strncmp("off", value, len))
+		return 0;
 
 	return -EINVAL;
 }

@@ -400,6 +402,14 @@ static int prop_compression_apply(struct inode *inode,
 		return 0;
 	}
 
+	if (!strncmp("off", value, len)) {
+		BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
+		BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
+		BTRFS_I(inode)->force_compress = BTRFS_COMPRESS_NONE;
+
+		return 0;
+	}
+
 	if (!strncmp("lzo", value, len))
 		type = BTRFS_COMPRESS_LZO;
 	else if (!strncmp("zlib", value, len))

@@ -423,5 +433,8 @@ static const char *prop_compression_extract(struct inode *inode)
 		return "lzo";
 	}
 
+	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
+		return "off";
+
 	return NULL;
 }

-- 1.8.3.1

author
#2 lore raw

PATCH 3/4 btrfs: export __btrfs_set_prop

From: Naohiro Aota <[email protected]>

Export __btrfs_set_prop() to be able to call it
with running transaction.

Signed-off-by Naohiro Aota <[email protected]>
Signed-off-by Satoru Takeuchi <[email protected]>
---
 fs/btrfs/props.c | 2 +-
 fs/btrfs/props.h | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)
fs/btrfs/props.c +1 -1
index 38efbe1..6f56f5b 100644
@@ -99,7 +99,7 @@ find_prop_handler(const char *name,
 	return NULL;
 }
 
-static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
+int __btrfs_set_prop(struct btrfs_trans_handle *trans,
 			    struct inode *inode,
 			    const char *name,
 			    const char *value,
fs/btrfs/props.h +6 -0
index 100f188..cff91e0 100644
@@ -28,6 +28,12 @@ int btrfs_set_prop(struct inode *inode,
 		   const char *value,
 		   size_t value_len,
 		   int flags);
+int __btrfs_set_prop(struct btrfs_trans_handle *trans,
+			    struct inode *inode,
+			    const char *name,
+			    const char *value,
+			    size_t value_len,
+			    int flags);
 
 int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);

-- 1.8.3.1

author
#3 lore raw

PATCH 4/4 btrfs: Fix compression related ioctl to run atomic operations in one transaction

From: Naohiro Aota <[email protected]>

Fix the following two problems in compression related ioctl code.

a) Updating compression flags and updating inode attribute in two separated transaction. So, if something bad happens after the former, and before the latter, file system would become inconsistent state.

This patch move them into one transaction.

b) It updates compression flags here and calls btrfs_set_prop() after that. However flags are also updated in this function.

This patch removes the duplicated code for updating flags from ioctl code and aggregates this work to __btrfs_set_prop() at all.

Signed-off-by Naohiro Aota <[email protected]>
Signed-off-by Satoru Takeuchi <[email protected]>
---
 fs/btrfs/ioctl.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)
fs/btrfs/ioctl.c +11 -21
index 0ff2127..47ac6da 100644
@@ -221,6 +221,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	u64 ip_oldflags;
 	unsigned int i_oldflags;
 	umode_t mode;
+	const char *comp;
 
 	if (!inode_owner_or_capable(inode))
 		return -EPERM;

@@ -310,40 +311,29 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	 * things smaller.
 	 */
 	if (flags & FS_NOCOMP_FL) {
-		ip->flags &= ~BTRFS_INODE_COMPRESS;
-		ip->flags |= BTRFS_INODE_NOCOMPRESS;
-
-		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
-		if (ret && ret != -ENODATA)
-			goto out_drop;
+		comp = "off";
 	} else if (flags & FS_COMPR_FL) {
-		const char *comp;
-
-		ip->flags |= BTRFS_INODE_COMPRESS;
-		ip->flags &= ~BTRFS_INODE_NOCOMPRESS;
-
 		if (root->fs_info->compress_type == BTRFS_COMPRESS_LZO)
 			comp = "lzo";
 		else
 			comp = "zlib";
-		ret = btrfs_set_prop(inode, "btrfs.compression",
-				     comp, strlen(comp), 0);
-		if (ret)
-			goto out_drop;
-
 	} else {
-		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);
+		comp = "";
 	}
 
-	trans = btrfs_start_transaction(root, 1);
+	trans = btrfs_start_transaction(root, 2);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		goto out_drop;
 	}
 
+	ret = __btrfs_set_prop(trans, inode, "btrfs.compression", comp,
+			       strlen(comp), 0);
+	if (ret && ret != -ENODATA) {
+		btrfs_end_transaction(trans, root);
+		goto out_drop;
+	}
+
 	btrfs_update_iflags(inode);
 	inode_inc_iversion(inode);
 	inode->i_ctime = CURRENT_TIME;

-- 1.8.3.1

author
#4 lore raw

PATCH v2 4/4 btrfs: Fix compression related ioctl to run atomic operations in one transaction

From: Naohiro Aota <[email protected]>

Fix the following two problems in compression related ioctl() code.

a) Updating compression flags and updating inode attribute in two separated transaction. So, if something bad happens after the former, and before the latter, file system would become inconsistent state.

This patch move them into one transaction.

b) It updates compression flags here and calls btrfs_set_prop() after that. However flags are also updated in this function.

This patch removes the duplicated code for updating flags from ioctl code and aggregates this work to btrfs_set_prop_trans() at all.

Signed-off-by Naohiro Aota <[email protected]>
Signed-off-by Satoru Takeuchi <[email protected]>

---
changelog

	v1->v2: Reflect the following comment from David
        https://www.mail-archive.com/[email protected]/msg37513.html
---
 fs/btrfs/ioctl.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)
fs/btrfs/ioctl.c +11 -21
index 0ff2127..40a0fdd 100644
@@ -221,6 +221,7 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	u64 ip_oldflags;
 	unsigned int i_oldflags;
 	umode_t mode;
+	const char *comp;
 
 	if (!inode_owner_or_capable(inode))
 		return -EPERM;

@@ -310,40 +311,29 @@ static int btrfs_ioctl_setflags(struct file *file, void __user *arg)
 	 * things smaller.
 	 */
 	if (flags & FS_NOCOMP_FL) {
-		ip->flags &= ~BTRFS_INODE_COMPRESS;
-		ip->flags |= BTRFS_INODE_NOCOMPRESS;
-
-		ret = btrfs_set_prop(inode, "btrfs.compression", NULL, 0, 0);
-		if (ret && ret != -ENODATA)
-			goto out_drop;
+		comp = "off";
 	} else if (flags & FS_COMPR_FL) {
-		const char *comp;
-
-		ip->flags |= BTRFS_INODE_COMPRESS;
-		ip->flags &= ~BTRFS_INODE_NOCOMPRESS;
-
 		if (root->fs_info->compress_type == BTRFS_COMPRESS_LZO)
 			comp = "lzo";
 		else
 			comp = "zlib";
-		ret = btrfs_set_prop(inode, "btrfs.compression",
-				     comp, strlen(comp), 0);
-		if (ret)
-			goto out_drop;
-
 	} else {
-		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);
+		comp = "";
 	}
 
-	trans = btrfs_start_transaction(root, 1);
+	trans = btrfs_start_transaction(root, 2);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		goto out_drop;
 	}
 
+	ret = btrfs_set_prop_trans(trans, inode, "btrfs.compression", comp,
+			       strlen(comp), 0);
+	if (ret && ret != -ENODATA) {
+		btrfs_end_transaction(trans, root);
+		goto out_drop;
+	}
+
 	btrfs_update_iflags(inode);
 	inode_inc_iversion(inode);
 	inode->i_ctime = CURRENT_TIME;

-- 1.8.3.1

#5 lore raw

Re: [PATCH 3/4] btrfs: export __btrfs_set_prop

In reply to Satoru Takeuchi

fs/btrfs/props.c find_prop_handler(const char *name, · line 102

Quoted 6 lines · Satoru Takeuchi
fs/btrfs/props.c +1 -1
@@ -99,7 +99,7 @@ find_prop_handler(const char *name,
 	return NULL;
 }
 
-static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
+int __btrfs_set_prop(struct btrfs_trans_handle *trans,

It's common for static helpers to use the __ prefix, but please drop it for an exported function(s).

author
#6 lore raw

PATCH v2 3/4 btrfs: Rename and export __btrfs_set_prop to be called from running transaction

Hi David,

Quoted 10 lines · David Sterba

Review find_prop_handler(const char *name, · line 102

Quoted 6 lines · Satoru Takeuchi
(quoted diff) +1 -1
@@ -99,7 +99,7 @@ find_prop_handler(const char *name,
  	return NULL;
  }
 
-static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
+int __btrfs_set_prop(struct btrfs_trans_handle *trans,

It's common for static helpers to use the __ prefix, but please drop it for an exported function(s).

Sorry for the late reply. Here is the v2 patch.
Please take alook at it.

---
From: Naohiro Aota <[email protected]>

Since "__" prefix means static helper, rename __btrfs_set_prop() to btrfs_set_prop_trans.

Signed-off-by Naohiro Aota <[email protected]>
Signed-off-by Satoru Takeuchi <[email protected]>

---
changelog

	v1->v2: Reflect the following comment from David.
        https://www.mail-archive.com/[email protected]/msg37513.html
---
 fs/btrfs/props.c | 6 +++---
 fs/btrfs/props.h | 7 +++++++
 2 files changed, 10 insertions(+), 3 deletions(-)
fs/btrfs/props.c +3 -3
index 38efbe1..bba081a 100644
@@ -99,7 +99,7 @@ find_prop_handler(const char *name,
 	return NULL;
 }
 
-static int __btrfs_set_prop(struct btrfs_trans_handle *trans,
+int btrfs_set_prop_trans(struct btrfs_trans_handle *trans,
 			    struct inode *inode,
 			    const char *name,
 			    const char *value,

@@ -153,7 +153,7 @@ int btrfs_set_prop(struct inode *inode,
 		   size_t value_len,
 		   int flags)
 {
-	return __btrfs_set_prop(NULL, inode, name, value, value_len, flags);
+	return btrfs_set_prop_trans(NULL, inode, name, value, value_len, flags);
 }
 
 static int iterate_object_props(struct btrfs_root *root,

@@ -325,7 +325,7 @@ static int inherit_props(struct btrfs_trans_handle *trans,
 					  num_bytes, BTRFS_RESERVE_NO_FLUSH);
 		if (ret)
 			goto out;
-		ret = __btrfs_set_prop(trans, inode, h->xattr_name,
+		ret = btrfs_set_prop_trans(trans, inode, h->xattr_name,
 				       value, strlen(value), 0);
 		btrfs_block_rsv_release(root, trans->block_rsv, num_bytes);
 		if (ret)
fs/btrfs/props.h +7 -0
index 100f188..dcbccf9 100644
@@ -29,6 +29,13 @@ int btrfs_set_prop(struct inode *inode,
 		   size_t value_len,
 		   int flags);
 
+int btrfs_set_prop_trans(struct btrfs_trans_handle *trans,
+			    struct inode *inode,
+			    const char *name,
+			    const char *value,
+			    size_t value_len,
+			    int flags);
+
 int btrfs_load_inode_props(struct inode *inode, struct btrfs_path *path);
 
 int btrfs_inode_inherit_props(struct btrfs_trans_handle *trans,

-- 1.8.3.1

#8 lore raw
In reply to Satoru Takeuchi
Quoted 8 lines · Satoru Takeuchi

In the current implementation, compression property == "" has the two different meanings: one is with BTRFS_INODE_NOCOMPRESS, and the other is without this flag.

So, even if the two files a and b have the same compression property, "", and the same contents, one file seems to be compressed and the other is not. It's difficult to understand for users and also confuses them.

Fixing this inconsistency is good, let me think more about the interface.

author
#9 lore raw

Hi David and Chris,

Quoted 5 lines · David Sterba
Quoted 4 lines · Satoru Takeuchi

In the current implementation, compression property == "" has the two different meanings: one is with BTRFS_INODE_NOCOMPRESS, and the other is without this flag.

Quoted 7 lines
Quoted 4 lines

So, even if the two files a and b have the same compression property, "", and the same contents, one file seems to be compressed and the other is not. It's difficult to understand for users and also confuses them.

Fixing this inconsistency is good, let me think more about the interface.

How about these patches? These patches seems not to be merged yet. Especially patch 4/4 resolve the bug that atomic two operations are separated to the different two transactions. I consider this patch should be applied as soon as possible.

Thanks,
Satoru

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#10 lore raw
Quoted 16 lines · Satoru Takeuchi

Hi David and Chris,

Quoted 13 lines · David Sterba
Quoted 9 lines · Satoru Takeuchi

In the current implementation, compression property == "" has the two different meanings: one is with BTRFS_INODE_NOCOMPRESS, and the other is without this flag.

So, even if the two files a and b have the same compression property, "", and the same contents, one file seems to be compressed and the other is not. It's difficult to understand for users and also confuses them.

Quoted 8 lines
Quoted 2 lines

Fixing this inconsistency is good, let me think more about the interface.

How about these patches? These patches seems not to be merged yet. Especially patch 4/4 resolve the bug that atomic two operations are separated to the different two transactions. I consider this patch should be applied as soon as possible.

Why do you think the possibility of 2 transactions being used are such a terrible thing that needs immediate attention?

If the first transaction ends up getting committed after calling btrfs_end_transaction(), and an error (or a crash/reboot) happens before the second transaction, userspace gets an error and gets to know the changes weren't fully applied (and the sensible action is to retry the ioctl later).
The worst it can happen is leaving some compression xattr or flag on disk after the first transaction commit. But that doesn't prevent userspace from doing file/directory operations nor much less causes any data loss or corruption, nor does it cause any metadata inconsistency reported by btrfsck or scrub for e.g.. So leaving compression on/off is transparent thing that doesn't affect user operations.

You can my Reviewed-by: Filipe Manana <[email protected]> to the series if you want.

thanks

Quoted 15 lines

Thanks,
Satoru

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 5 lines
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."