Compression

PATCH In review David Sterba 3 participants 8 messages Jul 17, 2017 – Jul 21, 2017 Btrfs file system

Share & tools

David Sterba

author maintainer

2017-07-17 18:47 UTC #0 lore raw

cover Compression

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(-)

Signature · 1 line

2.13.0

David Sterba

author maintainer

2017-07-17 18:47 UTC #1 lore raw

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

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 <[email protected]>

---
 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(-)

fs/btrfs/btrfs_inode.h +2 -2

index b8622e4d1744..0c9ba8b96782 100644
@@ -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;

fs/btrfs/inode.c +5 -5

index eb495e956d53..ece5362e7aff 100644
@@ -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;

fs/btrfs/ioctl.c +2 -2

index fa1b78cf25f6..571392cbd64c 100644
@@ -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)

fs/btrfs/props.c +3 -3

index 4b23ae5d0e5c..916f5cf9b292 100644
@@ -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:

Signature · 1 line

2.13.0

David Sterba

author maintainer

2017-07-17 18:47 UTC #2 lore raw

PATCH 2/4 btrfs: separate defrag and property compression

In reply to 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 <[email protected]>

---
 fs/btrfs/btrfs_inode.h | 5 +++++
 fs/btrfs/inode.c       | 8 +++++++-
 fs/btrfs/ioctl.c       | 4 ++--
 3 files changed, 14 insertions(+), 3 deletions(-)

fs/btrfs/btrfs_inode.h +5 -0

index 0c9ba8b96782..d71ad059ca7c 100644
@@ -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;

fs/btrfs/inode.c +7 -1

index ece5362e7aff..14677535610b 100644
@@ -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;

fs/btrfs/ioctl.c +2 -2

index 571392cbd64c..5d28d53e1b81 100644
@@ -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)

Signature · 1 line

2.13.0

David Sterba

author maintainer

2017-07-17 18:47 UTC #5 lore raw

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

In reply to David Sterba

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ioctl.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

fs/btrfs/ioctl.c +6 -6

index 5d28d53e1b81..37226b13e289 100644
@@ -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);

Signature · 1 line

2.13.0

David Sterba

author maintainer

2017-07-17 18:47 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 fs/btrfs/inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

fs/btrfs/inode.c +3 -3

index 14677535610b..d28441831ac6 100644
@@ -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)

Signature · 1 line

2.13.0

Anand Jain

2017-07-20 10:42 UTC #3 lore raw

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

In reply to David Sterba

Quoted 3 lines · 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.

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

Quoted 16 lines

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).
fs/btrfs/inode.c +3 -1
@@ -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;

Anand Jain

2017-07-20 10:56 UTC #7 lore raw

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

In reply to David Sterba

Quoted 4 lines · 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).

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

David Sterba

2017-07-21 17:36 UTC #4 lore raw

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

In reply to Anand Jain

Quoted 11 lines · Anand Jain

Quoted 3 lines · 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.
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.

Quoted 22 lines

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;
}

Quoted 6 lines

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