[PATCH 0/4] Compression

[PATCH 0/4] Compression

[David Sterba]

An improved version of patch https://patchwork.kernel.org/patch/9838439/ after
Anand's comment that the change also affects properties, this time it addresses
the core problem first and then changes the defrag priority over nocompress.

David Sterba (4):
  btrfs: rename variable holding per-inode compression type
  btrfs: separate defrag and property compression
  btrfs: defrag: cleanup checking for compression status
  btrfs: allow defrag compress to override NOCOMPRESS attribute

 fs/btrfs/btrfs_inode.h |  9 +++++++--
 fs/btrfs/inode.c       | 16 +++++++++++-----
 fs/btrfs/ioctl.c       | 16 ++++++++--------
 fs/btrfs/props.c       |  6 +++---
 4 files changed, 29 insertions(+), 18 deletions(-)

-- 
2.13.0

[PATCH 1/4] btrfs: rename variable holding per-inode compression type

[David Sterba]

This is preparatory for separating inode compression requested by defrag
and set via properties. This will fix a usability bug when defrag will
reset compression type to NONE. If the file has compression set via
property, it will not apply anymore (until next mount or reset through
command line).

We're going to fix that by adding another variable just for the defrag
call and won't touch the property. The defrag will have higher priority
when deciding whether to compress the data.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/btrfs_inode.h |  4 ++--
 fs/btrfs/inode.c       | 10 +++++-----
 fs/btrfs/ioctl.c       |  4 ++--
 fs/btrfs/props.c       |  6 +++---
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index b8622e4d1744..0c9ba8b96782 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -179,9 +179,9 @@ struct btrfs_inode {
 	unsigned reserved_extents;
 
 	/*
-	 * always compress this one file
+	 * Cached values if inode properties
 	 */
-	unsigned force_compress;
+	unsigned prop_compress;		/* per-file compression algorithm */
 
 	struct btrfs_delayed_node *delayed_node;
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index eb495e956d53..ece5362e7aff 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -404,7 +404,7 @@ static inline int inode_need_compress(struct inode *inode)
 		return 0;
 	if (btrfs_test_opt(fs_info, COMPRESS) ||
 	    BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS ||
-	    BTRFS_I(inode)->force_compress)
+	    BTRFS_I(inode)->prop_compress)
 		return 1;
 	return 0;
 }
@@ -511,8 +511,8 @@ static noinline void compress_file_range(struct inode *inode,
 			goto cont;
 		}
 
-		if (BTRFS_I(inode)->force_compress)
-			compress_type = BTRFS_I(inode)->force_compress;
+		if (BTRFS_I(inode)->prop_compress)
+			compress_type = BTRFS_I(inode)->prop_compress;
 
 		/*
 		 * we need to call clear_page_dirty_for_io on each
@@ -645,7 +645,7 @@ static noinline void compress_file_range(struct inode *inode,
 
 		/* flag the file so we don't compress in the future */
 		if (!btrfs_test_opt(fs_info, FORCE_COMPRESS) &&
-		    !(BTRFS_I(inode)->force_compress)) {
+		    !(BTRFS_I(inode)->prop_compress)) {
 			BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
 		}
 	}
@@ -9402,7 +9402,7 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
 	ei->reserved_extents = 0;
 
 	ei->runtime_flags = 0;
-	ei->force_compress = BTRFS_COMPRESS_NONE;
+	ei->prop_compress = BTRFS_COMPRESS_NONE;
 
 	ei->delayed_node = NULL;
 
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index fa1b78cf25f6..571392cbd64c 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1402,7 +1402,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 
 		inode_lock(inode);
 		if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)
-			BTRFS_I(inode)->force_compress = compress_type;
+			BTRFS_I(inode)->prop_compress = compress_type;
 		ret = cluster_pages_for_defrag(inode, pages, i, cluster);
 		if (ret < 0) {
 			inode_unlock(inode);
@@ -1473,7 +1473,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 out_ra:
 	if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS) {
 		inode_lock(inode);
-		BTRFS_I(inode)->force_compress = BTRFS_COMPRESS_NONE;
+		BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
 		inode_unlock(inode);
 	}
 	if (!file)
diff --git a/fs/btrfs/props.c b/fs/btrfs/props.c
index 4b23ae5d0e5c..916f5cf9b292 100644
--- a/fs/btrfs/props.c
+++ b/fs/btrfs/props.c
@@ -403,7 +403,7 @@ static int prop_compression_apply(struct inode *inode,
 	if (len == 0) {
 		BTRFS_I(inode)->flags |= BTRFS_INODE_NOCOMPRESS;
 		BTRFS_I(inode)->flags &= ~BTRFS_INODE_COMPRESS;
-		BTRFS_I(inode)->force_compress = BTRFS_COMPRESS_NONE;
+		BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
 
 		return 0;
 	}
@@ -417,14 +417,14 @@ static int prop_compression_apply(struct inode *inode,
 
 	BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;
 	BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
-	BTRFS_I(inode)->force_compress = type;
+	BTRFS_I(inode)->prop_compress = type;
 
 	return 0;
 }
 
 static const char *prop_compression_extract(struct inode *inode)
 {
-	switch (BTRFS_I(inode)->force_compress) {
+	switch (BTRFS_I(inode)->prop_compress) {
 	case BTRFS_COMPRESS_ZLIB:
 		return "zlib";
 	case BTRFS_COMPRESS_LZO:
-- 
2.13.0

[PATCH 2/4] btrfs: separate defrag and property compression

[David Sterba]

Add new value for compression to distinguish between defrag and
property. Previously, a single variable was used and this caused clashes
when the per-file 'compression' was set and a defrag -c was called.

The property-compression is loaded when the file is open, defrag will
overwrite the same variable and reset to 0 (ie. NONE) at when the file
defragmentaion is finished. That's considered a usability bug.

Now we won't touch the property value, use the defrag-compression. The
precedence of defrag is higher than for property (and whole-filesystem).

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

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 0c9ba8b96782..d71ad059ca7c 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -182,6 +182,11 @@ struct btrfs_inode {
 	 * Cached values if inode properties
 	 */
 	unsigned prop_compress;		/* per-file compression algorithm */
+	/*
+	 * Force compression on the file using the defrag ioctl, could be
+	 * different from prop_compress and takes precedence if set
+	 */
+	unsigned defrag_compress;
 
 	struct btrfs_delayed_node *delayed_node;
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index ece5362e7aff..14677535610b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -402,6 +402,9 @@ static inline int inode_need_compress(struct inode *inode)
 	/* bad compression ratios */
 	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
 		return 0;
+	/* defrag ioctl */
+	if (BTRFS_I(inode)->defrag_compress)
+		return 1;
 	if (btrfs_test_opt(fs_info, COMPRESS) ||
 	    BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS ||
 	    BTRFS_I(inode)->prop_compress)
@@ -511,7 +514,9 @@ static noinline void compress_file_range(struct inode *inode,
 			goto cont;
 		}
 
-		if (BTRFS_I(inode)->prop_compress)
+		if (BTRFS_I(inode)->defrag_compress)
+			compress_type = BTRFS_I(inode)->defrag_compress;
+		else if (BTRFS_I(inode)->prop_compress)
 			compress_type = BTRFS_I(inode)->prop_compress;
 
 		/*
@@ -9403,6 +9408,7 @@ struct inode *btrfs_alloc_inode(struct super_block *sb)
 
 	ei->runtime_flags = 0;
 	ei->prop_compress = BTRFS_COMPRESS_NONE;
+	ei->defrag_compress = BTRFS_COMPRESS_NONE;
 
 	ei->delayed_node = NULL;
 
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 571392cbd64c..5d28d53e1b81 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1402,7 +1402,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 
 		inode_lock(inode);
 		if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)
-			BTRFS_I(inode)->prop_compress = compress_type;
+			BTRFS_I(inode)->defrag_compress = compress_type;
 		ret = cluster_pages_for_defrag(inode, pages, i, cluster);
 		if (ret < 0) {
 			inode_unlock(inode);
@@ -1473,7 +1473,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 out_ra:
 	if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS) {
 		inode_lock(inode);
-		BTRFS_I(inode)->prop_compress = BTRFS_COMPRESS_NONE;
+		BTRFS_I(inode)->defrag_compress = BTRFS_COMPRESS_NONE;
 		inode_unlock(inode);
 	}
 	if (!file)
-- 
2.13.0

Re: [PATCH 2/4] btrfs: separate defrag and property compression

[Anand Jain]

On 07/18/2017 02:46 AM, David Sterba wrote:
> Add new value for compression to distinguish between defrag and
> property. Previously, a single variable was used and this caused clashes
> when the per-file 'compression' was set and a defrag -c was called.

  How about..
    deprecate property compression
    introduce property compress (inline with -o compress) [1]
    introduce property compress-force (inline with -o compress-force) [2]

inode_need_compress will look something like this..
-----
static inline int inode_need_compress(struct inode *inode)
{
         struct btrfs_root *root = BTRFS_I(inode)->root;

         /* force compress */
         if (btrfs_test_opt(root->fs_info, FORCE_COMPRESS) ||
		            BTRFS_I(inode)->force_compress) [2]
                 return 1;

         /* bad compression ratios */
         if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
                 return 0;

         if (btrfs_test_opt(root->fs_info, COMPRESS) ||
             BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS ||
             BTRFS_I(inode)->compress) [1]
                 return 1;

         return 0;
}
-----

  defrag -c will in turn set the compress property.

  introduce defrag --compress-force|-C to in turn set the compress-force
  property.

  Now user has a way to check the compression property using
    btrfs prop get ...

  And we have a consistent nomenclature ;-)

Thanks, Anand


> The property-compression is loaded when the file is open, defrag will
> overwrite the same variable and reset to 0 (ie. NONE) at when the file
> defragmentaion is finished. That's considered a usability bug.
> 
> Now we won't touch the property value, use the defrag-compression. The
> precedence of defrag is higher than for property (and whole-filesystem).

> @@ -511,7 +514,9 @@ static noinline void compress_file_range(struct inode *inode,
>   			goto cont;
>   		}
>   
> -		if (BTRFS_I(inode)->prop_compress)
> +		if (BTRFS_I(inode)->defrag_compress)
> +			compress_type = BTRFS_I(inode)->defrag_compress;
> +		else if (BTRFS_I(inode)->prop_compress)
>   			compress_type = BTRFS_I(inode)->prop_compress;

Re: [PATCH 2/4] btrfs: separate defrag and property compression

[David Sterba]

On Thu, Jul 20, 2017 at 06:49:19PM +0800, Anand Jain wrote:
> 
> 
> On 07/18/2017 02:46 AM, David Sterba wrote:
> > Add new value for compression to distinguish between defrag and
> > property. Previously, a single variable was used and this caused clashes
> > when the per-file 'compression' was set and a defrag -c was called.
> 
>   How about..
>     deprecate property compression
>     introduce property compress (inline with -o compress) [1]
>     introduce property compress-force (inline with -o compress-force) [2]

IIRC compress-force has been added as a bandaid when 'compress' bails
out of compression too soon (due to the trivial check and that just one
incompressible chunk can set tne NOCOMPRESS bit). So compress-force now
compresses everthing even if it's not worth.

What we need is better behaviour of 'compress', and that's where the
heuristics should help. It will try to guess if the data are
compressible in advance, so we hopefully don't reach the point where the
incompressible chunk will flip the NOCOMPRESS bit.

I don't want to spread the compress-force logic further, rather improve
the heuristic and possibly track a long-term stats about the file and
the data compressibility. So everybody can use 'compress' and expect
sane behaviour, and this would apply to the per-file property and
defrag.

> inode_need_compress will look something like this..
> -----
> static inline int inode_need_compress(struct inode *inode)
> {
>          struct btrfs_root *root = BTRFS_I(inode)->root;
> 
>          /* force compress */
>          if (btrfs_test_opt(root->fs_info, FORCE_COMPRESS) ||
> 		            BTRFS_I(inode)->force_compress) [2]
>                  return 1;
> 
>          /* bad compression ratios */
>          if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
>                  return 0;
> 
>          if (btrfs_test_opt(root->fs_info, COMPRESS) ||
>              BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS ||
>              BTRFS_I(inode)->compress) [1]
>                  return 1;
> 
>          return 0;
> }
> -----
> 
>   defrag -c will in turn set the compress property.
> 
>   introduce defrag --compress-force|-C to in turn set the compress-force
>   property.

This would be a good enhancement to the defrag interface to
check/manipulate the properties, but this does not affect the kernel
behaviour.

[PATCH 3/4] btrfs: defrag: cleanup checking for compression status

[David Sterba]

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 5d28d53e1b81..37226b13e289 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1286,6 +1286,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 	unsigned long cluster = max_cluster;
 	u64 new_align = ~((u64)SZ_128K - 1);
 	struct page **pages = NULL;
+	bool do_compress = range->flags & BTRFS_DEFRAG_RANGE_COMPRESS;
 
 	if (isize == 0)
 		return 0;
@@ -1293,7 +1294,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 	if (range->start >= isize)
 		return -EINVAL;
 
-	if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS) {
+	if (do_compress) {
 		if (range->compress_type > BTRFS_COMPRESS_TYPES)
 			return -EINVAL;
 		if (range->compress_type)
@@ -1373,8 +1374,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 
 		if (!should_defrag_range(inode, (u64)i << PAGE_SHIFT,
 					 extent_thresh, &last_len, &skip,
-					 &defrag_end, range->flags &
-					 BTRFS_DEFRAG_RANGE_COMPRESS)) {
+					 &defrag_end, do_compress)){
 			unsigned long next;
 			/*
 			 * the should_defrag function tells us how much to skip
@@ -1401,7 +1401,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 		}
 
 		inode_lock(inode);
-		if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)
+		if (do_compress)
 			BTRFS_I(inode)->defrag_compress = compress_type;
 		ret = cluster_pages_for_defrag(inode, pages, i, cluster);
 		if (ret < 0) {
@@ -1449,7 +1449,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 			filemap_flush(inode->i_mapping);
 	}
 
-	if ((range->flags & BTRFS_DEFRAG_RANGE_COMPRESS)) {
+	if (do_compress) {
 		/* the filemap_flush will queue IO into the worker threads, but
 		 * we have to make sure the IO is actually started and that
 		 * ordered extents get created before we return
@@ -1471,7 +1471,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 	ret = defrag_count;
 
 out_ra:
-	if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS) {
+	if (do_compress) {
 		inode_lock(inode);
 		BTRFS_I(inode)->defrag_compress = BTRFS_COMPRESS_NONE;
 		inode_unlock(inode);
-- 
2.13.0

[PATCH 4/4] btrfs: allow defrag compress to override NOCOMPRESS attribute

[David Sterba]

Currently, the BTRFS_INODE_NOCOMPRESS will prevent any compression on a
given file, except when the mount is force-compress. As users have
reported on IRC, this will also prevent compression when requested by
defrag (btrfs fi defrag -c file).

The nocompress flag is set automatically by filesystem when the ratios
are bad and the user would have to manually drop the bit in order to
make defrag -c work. This is not good from the usability perspective.

This patch will raise priority for the defrag -c over nocompress, ie.
any file with NOCOMPRESS bit set will get defragmented. The bit will
remain untouched.

Alternate option was to also drop the nocompress bit and keep the
decision logic as is, but I think this is not the right solution.

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

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 14677535610b..d28441831ac6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -399,12 +399,12 @@ static inline int inode_need_compress(struct inode *inode)
 	/* force compress */
 	if (btrfs_test_opt(fs_info, FORCE_COMPRESS))
 		return 1;
-	/* bad compression ratios */
-	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
-		return 0;
 	/* defrag ioctl */
 	if (BTRFS_I(inode)->defrag_compress)
 		return 1;
+	/* bad compression ratios */
+	if (BTRFS_I(inode)->flags & BTRFS_INODE_NOCOMPRESS)
+		return 0;
 	if (btrfs_test_opt(fs_info, COMPRESS) ||
 	    BTRFS_I(inode)->flags & BTRFS_INODE_COMPRESS ||
 	    BTRFS_I(inode)->prop_compress)
-- 
2.13.0

Re: [PATCH 4/4] btrfs: allow defrag compress to override NOCOMPRESS attribute

[Anand Jain]

On 07/18/2017 02:46 AM, David Sterba wrote:
> Currently, the BTRFS_INODE_NOCOMPRESS will prevent any compression on a
> given file, except when the mount is force-compress. As users have
> reported on IRC, this will also prevent compression when requested by
> defrag (btrfs fi defrag -c file).

  There is a hidden workaround... even with the existing
  inode_need_compression().

  BTRFS_INODE_NOCOMPRESS gets reset [1] so ..

    (btrfs prop set /btrfs/sv1 compression "")
    btrfs prop set /btrfs/sv1 compression lzo
    A normal defrag or with -c will try to compress again.

----------------
static int prop_compression_apply(struct inode *inode,
                                   const char *value,
                                   size_t len)
{
         int type;

         if (len == 0) {
                 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))
                 type = BTRFS_COMPRESS_ZLIB;
         else
                 return -EINVAL;

         BTRFS_I(inode)->flags &= ~BTRFS_INODE_NOCOMPRESS;   <---- [1]
         BTRFS_I(inode)->flags |= BTRFS_INODE_COMPRESS;
         BTRFS_I(inode)->force_compress = type;

         return 0;
}
---------------

  So what's missing is btrfs prop set /btrfs/sv1 compress-force
  and rest cleanup as discussed in the other email.

Thanks, Anand