[PATCH 1/4] btrfs: correct empty compression property behavior

[PATCH 1/4] btrfs: correct empty compression property behavior

[Satoru Takeuchi]

From: Naohiro Aota <naota@elisp.net>

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 <naota@elisp.net>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
---
 fs/btrfs/props.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 129b1dd..bf005f4 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -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 

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

[Satoru Takeuchi]

From: Naohiro Aota <naota@elisp.net>

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

Signed-off-by: Naohiro Aota <naota@elisp.net>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
---
 fs/btrfs/props.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index bf005f4..38efbe1 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -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 

[PATCH 3/4] btrfs: export __btrfs_set_prop

[Satoru Takeuchi]

From: Naohiro Aota <naota@elisp.net>

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

Signed-off-by: Naohiro Aota <naota@elisp.net>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
---
 fs/btrfs/props.c | 2 +-
 fs/btrfs/props.h | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 38efbe1..6f56f5b 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -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,
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index 100f188..cff91e0 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -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 

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

[Satoru Takeuchi]

From: Naohiro Aota <naota@elisp.net>

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 <naota@elisp.net>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
---
 fs/btrfs/ioctl.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0ff2127..47ac6da 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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 

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

[David Sterba]

On Fri, Sep 19, 2014 at 05:52:17PM +0900, Satoru Takeuchi wrote:
> @@ -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).

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

[Satoru Takeuchi]

Hi David,

(2014/09/22 21:01), David Sterba wrote:
> On Fri, Sep 19, 2014 at 05:52:17PM +0900, Satoru Takeuchi wrote:
>> @@ -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 <naota@elisp.net>

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

Signed-off-by: Naohiro Aota <naota@elisp.net>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
---
changelog
	v1->v2: Reflect the following comment from David.
         https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg37513.html
---
  fs/btrfs/props.c | 6 +++---
  fs/btrfs/props.h | 7 +++++++
  2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 38efbe1..bba081a 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -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)
diff --git a/fs/btrfs/props.h b/fs/btrfs/props.h
index 100f188..dcbccf9 100644
--- a/fs/btrfs/props.h
+++ b/fs/btrfs/props.h
@@ -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

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

[Satoru Takeuchi]

From: Naohiro Aota <naota@elisp.net>

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 <naota@elisp.net>
Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
---
changelog
	v1->v2: Reflect the following comment from David
        https://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg37513.html
---
 fs/btrfs/ioctl.c | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0ff2127..40a0fdd 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -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 

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

[David Sterba]

On Thu, Sep 25, 2014 at 02:55:29PM +0900, Satoru Takeuchi wrote:
> Since "__" prefix means static helper, rename __btrfs_set_prop() to
> btrfs_set_prop_trans.
> 
> Signed-off-by: Naohiro Aota <naota@elisp.net>
> Signed-off-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

Reviewed-by: David Sterba <dsterba@suse.cz>

Re: [PATCH 1/4] btrfs: correct empty compression property behavior

[David Sterba]

On Fri, Sep 19, 2014 at 05:45:49PM +0900, Satoru Takeuchi wrote:
> 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.

Re: [PATCH 1/4] btrfs: correct empty compression property behavior

[Satoru Takeuchi]

Hi David and Chris,

(2014/09/29 18:36), David Sterba wrote:
> On Fri, Sep 19, 2014 at 05:45:49PM +0900, Satoru Takeuchi wrote:
>> 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.

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

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

Re: [PATCH 1/4] btrfs: correct empty compression property behavior

[Filipe David Manana]

On Thu, Oct 16, 2014 at 2:37 AM, Satoru Takeuchi
<takeuchi_satoru@jp.fujitsu.com> wrote:
> Hi David and Chris,
>
> (2014/09/29 18:36), David Sterba wrote:
>>
>> On Fri, Sep 19, 2014 at 05:45:49PM +0900, Satoru Takeuchi wrote:
>>>
>>> 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.
>
>
> 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 <fdmanana@suse.com> to the
series if you want.

thanks

>
> Thanks,
> Satoru
>
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
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."