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

Message ID 9dc75da8c4a2cfc0cddec3c9ccf95f5d28a89084.1500317040.git.dsterba@suse.com
State New
Headers show

Commit Message

David Sterba July 17, 2017, 6:46 p.m. UTC
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(-)

Comments

Anand Jain July 20, 2017, 10:49 a.m. UTC | #1
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;



--
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
David Sterba July 21, 2017, 5:35 p.m. UTC | #2
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.
--
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

Patch
diff mbox

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)