[PATCH] btrfs: allow setting per extent compression

[PATCH] btrfs: allow setting per extent compression

[stijn rutjens]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

Hi all,
As mentioned in https://github.com/kdave/btrfs-progs/issues/184 it
would be nice to be able to set the compression level per extent (or
file) from the IOCTL interface.
I'm not sure how submitting patches to mailing lists works, but I have
attached a patch which implements this. Any and all feedback is
appreciated.
Kind regards,
Stijn Rutjens

[-- Attachment #2: allow-per-extent-compression.patch --]
[-- Type: text/x-patch, Size: 3883 bytes --]

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index bb374042d..e1603e1cf 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -67,7 +67,7 @@ int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
 	btrfs_set_file_extent_ram_bytes(leaf, item, ram_bytes);
 	btrfs_set_file_extent_generation(leaf, item, trans->transid);
 	btrfs_set_file_extent_type(leaf, item, BTRFS_FILE_EXTENT_REG);
-	btrfs_set_file_extent_compression(leaf, item, compression);
+	btrfs_set_file_extent_compression(leaf, item, compression & 0xF);
 	btrfs_set_file_extent_encryption(leaf, item, encryption);
 	btrfs_set_file_extent_other_encoding(leaf, item, other_encoding);
 
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index dbc9bcaf5..3285d4bd3 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -227,7 +227,7 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
 			compressed_size -= cur_size;
 		}
 		btrfs_set_file_extent_compression(leaf, ei,
-						  compress_type);
+						  compress_type & 0XF);
 	} else {
 		page = find_get_page(inode->i_mapping,
 				     start >> PAGE_SHIFT);
@@ -573,14 +573,31 @@ static noinline int compress_file_range(struct async_chunk *async_chunk)
 		}
 
 		/* Compression level is applied here and only here */
-		ret = btrfs_compress_pages(
-			compress_type | (fs_info->compress_level << 4),
-					   inode->i_mapping, start,
-					   pages,
-					   &nr_pages,
-					   &total_in,
-					   &total_compressed);
-
+		/*
+		 * Check if the upper bits are set, and if so,
+		 * take them as the compression level.
+		 * the inode compression level takes precendence, if set
+		 */
+		if ((compress_type & 0xF) == compress_type) {
+			ret = btrfs_compress_pages(
+				compress_type | (fs_info->compress_level << 4),
+						inode->i_mapping, start,
+						pages,
+						&nr_pages,
+						&total_in,
+						&total_compressed);
+		} else {
+			int compress_level = btrfs_compress_set_level(
+						compress_type & 0xF,
+						compress_type>>4);
+			ret = btrfs_compress_pages(
+				compress_type | (compress_level << 4),
+				inode->i_mapping, start,
+				pages,
+				&nr_pages,
+				&total_in,
+				&total_compressed);
+		}
 		if (!ret) {
 			unsigned long offset = offset_in_page(total_compressed);
 			struct page *page = pages[nr_pages - 1];
@@ -2362,7 +2379,7 @@ static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,
 	btrfs_set_file_extent_offset(leaf, fi, 0);
 	btrfs_set_file_extent_num_bytes(leaf, fi, num_bytes);
 	btrfs_set_file_extent_ram_bytes(leaf, fi, ram_bytes);
-	btrfs_set_file_extent_compression(leaf, fi, compression);
+	btrfs_set_file_extent_compression(leaf, fi, compression & 0xF);
 	btrfs_set_file_extent_encryption(leaf, fi, encryption);
 	btrfs_set_file_extent_other_encoding(leaf, fi, other_encoding);
 
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0fa1c386d..2a9c1f312 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1414,7 +1414,11 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 		return -EINVAL;
 
 	if (do_compress) {
-		if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES)
+		/*
+		* The bottom 4 bits of compress_type are for used for the
+		* compression type, the other bits for the compression level
+		*/
+		if ((range->compress_type & 0xF) >= BTRFS_NR_COMPRESS_TYPES)
 			return -EINVAL;
 		if (range->compress_type)
 			compress_type = range->compress_type;
@@ -1572,9 +1576,9 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
 			filemap_flush(inode->i_mapping);
 	}
 
-	if (range->compress_type == BTRFS_COMPRESS_LZO) {
+	if ((range->compress_type & 0xF) == BTRFS_COMPRESS_LZO) {
 		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
-	} else if (range->compress_type == BTRFS_COMPRESS_ZSTD) {
+	} else if ((range->compress_type & 0xF) == BTRFS_COMPRESS_ZSTD) {
 		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
 	}
 

Re: [PATCH] btrfs: allow setting per extent compression

[David Sterba]

Hi,

On Sun, Apr 12, 2020 at 10:57:03AM +0200, stijn rutjens wrote:
> Hi all,
> As mentioned in https://github.com/kdave/btrfs-progs/issues/184 it
> would be nice to be able to set the compression level per extent (or
> file) from the IOCTL interface.
> I'm not sure how submitting patches to mailing lists works, but I have
> attached a patch which implements this. Any and all feedback is
> appreciated.

thanks for sending it, I don't insist on the patch formatting etc as
long as we have code to discuss, adding the signed-off-by tags is easy
and not the most imporatnt thing at the moment.

So the idea is to allow passing compression level to the defrag ioctl
where it applies to the files, regardless of other compression settings
like mount options or properties.

This has several parts:

* make the code accept the type and level, ie. it has to filter out the
  level which is what your patch mostly does

* keep compatibility with older kernels and newer tools, so the
  type+level is refused by older kernels

* priority of the level when there are more settings to choose from
  (inode, mount, ioctl)

* on-disk format

> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index bb374042d..e1603e1cf 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -67,7 +67,7 @@ int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
>  	btrfs_set_file_extent_ram_bytes(leaf, item, ram_bytes);
>  	btrfs_set_file_extent_generation(leaf, item, trans->transid);
>  	btrfs_set_file_extent_type(leaf, item, BTRFS_FILE_EXTENT_REG);
> -	btrfs_set_file_extent_compression(leaf, item, compression);
> +	btrfs_set_file_extent_compression(leaf, item, compression & 0xF);

As a general comment, open coding the level as "& 0xf" should be replaced
with a helper.

But in all cases where btrfs_set_file_extent_compression is called,
there shluld be already only the type. Ie. it's up to the calling code
to pass the correct value. An assert would be good.

The number of entry points where the type and level are available is
limited to ioctls or properties and the level is not part of the on-disk
format so all code that handles the format should assume the value is
valid.

>  	btrfs_set_file_extent_encryption(leaf, item, encryption);
>  	btrfs_set_file_extent_other_encoding(leaf, item, other_encoding);
>  
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index dbc9bcaf5..3285d4bd3 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -227,7 +227,7 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
>  			compressed_size -= cur_size;
>  		}
>  		btrfs_set_file_extent_compression(leaf, ei,
> -						  compress_type);
> +						  compress_type & 0XF);
>  	} else {
>  		page = find_get_page(inode->i_mapping,
>  				     start >> PAGE_SHIFT);
> @@ -573,14 +573,31 @@ static noinline int compress_file_range(struct async_chunk *async_chunk)
>  		}
>  
>  		/* Compression level is applied here and only here */
> -		ret = btrfs_compress_pages(
> -			compress_type | (fs_info->compress_level << 4),
> -					   inode->i_mapping, start,
> -					   pages,
> -					   &nr_pages,
> -					   &total_in,
> -					   &total_compressed);
> -
> +		/*
> +		 * Check if the upper bits are set, and if so,
> +		 * take them as the compression level.
> +		 * the inode compression level takes precendence, if set
> +		 */
> +		if ((compress_type & 0xF) == compress_type) {

So this is a default behaviour, if no level is specified, take whatever
default is there. Ok.

> +			ret = btrfs_compress_pages(
> +				compress_type | (fs_info->compress_level << 4),
> +						inode->i_mapping, start,
> +						pages,
> +						&nr_pages,
> +						&total_in,
> +						&total_compressed);
> +		} else {
> +			int compress_level = btrfs_compress_set_level(
> +						compress_type & 0xF,
> +						compress_type>>4);

Otherwise use the level specified by the user, that's the intended
usecase.

> +			ret = btrfs_compress_pages(
> +				compress_type | (compress_level << 4),
> +				inode->i_mapping, start,
> +				pages,
> +				&nr_pages,
> +				&total_in,
> +				&total_compressed);
> +		}
>  		if (!ret) {
>  			unsigned long offset = offset_in_page(total_compressed);
>  			struct page *page = pages[nr_pages - 1];
> @@ -2362,7 +2379,7 @@ static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,
>  	btrfs_set_file_extent_offset(leaf, fi, 0);
>  	btrfs_set_file_extent_num_bytes(leaf, fi, num_bytes);
>  	btrfs_set_file_extent_ram_bytes(leaf, fi, ram_bytes);
> -	btrfs_set_file_extent_compression(leaf, fi, compression);
> +	btrfs_set_file_extent_compression(leaf, fi, compression & 0xF);
>  	btrfs_set_file_extent_encryption(leaf, fi, encryption);
>  	btrfs_set_file_extent_other_encoding(leaf, fi, other_encoding);
>  
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 0fa1c386d..2a9c1f312 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1414,7 +1414,11 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
>  		return -EINVAL;
>  
>  	if (do_compress) {
> -		if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES)
> +		/*
> +		* The bottom 4 bits of compress_type are for used for the
> +		* compression type, the other bits for the compression level
> +		*/
> +		if ((range->compress_type & 0xF) >= BTRFS_NR_COMPRESS_TYPES)
>  			return -EINVAL;

For the backward compatibility, the check without "& 0xf" is sufficient,
but a new flag is needed to actually tell the ioctl code to read the
level. I'm not sure that relying on automatic parsing of the high 4 bits
is a good idea, usually all new data are accompanied by new flags.

>  		if (range->compress_type)
>  			compress_type = range->compress_type;
> @@ -1572,9 +1576,9 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
>  			filemap_flush(inode->i_mapping);
>  	}
>  
> -	if (range->compress_type == BTRFS_COMPRESS_LZO) {
> +	if ((range->compress_type & 0xF) == BTRFS_COMPRESS_LZO) {
>  		btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
> -	} else if (range->compress_type == BTRFS_COMPRESS_ZSTD) {
> +	} else if ((range->compress_type & 0xF) == BTRFS_COMPRESS_ZSTD) {
>  		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
>  	}

For further iterations of the patch, I suggest to split it into more
patches by logical change, like adding helpers, extending the ioctl,
adding asserts, etc.

I haven't found any obstacles, the core of the work is already in this
patch, remaining is the interface bits and polishing the code.

[PATCH v2] btrfs: allow setting per extent compression

[stijn rutjens]

""Hi,
I've added an ioctl flag to notify the interface we want to use the
upper 4 bits for the compression level (which has significantly
decreased the size of the patch). How do you want to split the patch
up?

> > diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> > index bb374042d..e1603e1cf 100644
> > --- a/fs/btrfs/file-item.c
> > +++ b/fs/btrfs/file-item.c
> > @@ -67,7 +67,7 @@ int btrfs_insert_file_extent(struct btrfs_trans_handle *trans,
> >       btrfs_set_file_extent_ram_bytes(leaf, item, ram_bytes);
> >       btrfs_set_file_extent_generation(leaf, item, trans->transid);
> >       btrfs_set_file_extent_type(leaf, item, BTRFS_FILE_EXTENT_REG);
> > -     btrfs_set_file_extent_compression(leaf, item, compression);
> > +     btrfs_set_file_extent_compression(leaf, item, compression & 0xF);
>
> As a general comment, open coding the level as "& 0xf" should be replaced
> with a helper.

I've now changed this to use the already written "btrfs_compress_type"
and "btrfs_compress_level" helpers

> But in all cases where btrfs_set_file_extent_compression is called,
> there shluld be already only the type. Ie. it's up to the calling code
> to pass the correct value. An assert would be good.
>
> The number of entry points where the type and level are available is
> limited to ioctls or properties and the level is not part of the on-disk
> format so all code that handles the format should assume the value is
> valid.
this is also fixed

> > +                     ret = btrfs_compress_pages(
> > +                             compress_type | (compress_level << 4),
> > +                             inode->i_mapping, start,
> > +                             pages,
> > +                             &nr_pages,
> > +                             &total_in,
> > +                             &total_compressed);
> > +             }
> >               if (!ret) {
> >                       unsigned long offset = offset_in_page(total_compressed);
> >                       struct page *page = pages[nr_pages - 1];
> > @@ -2362,7 +2379,7 @@ static int insert_reserved_file_extent(struct btrfs_trans_handle *trans,
> >       btrfs_set_file_extent_offset(leaf, fi, 0);
> >       btrfs_set_file_extent_num_bytes(leaf, fi, num_bytes);
> >       btrfs_set_file_extent_ram_bytes(leaf, fi, ram_bytes);
> > -     btrfs_set_file_extent_compression(leaf, fi, compression);
> > +     btrfs_set_file_extent_compression(leaf, fi, compression & 0xF);
> >       btrfs_set_file_extent_encryption(leaf, fi, encryption);
> >       btrfs_set_file_extent_other_encoding(leaf, fi, other_encoding);
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index 0fa1c386d..2a9c1f312 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -1414,7 +1414,11 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
> >               return -EINVAL;
> >
> >       if (do_compress) {
> > -             if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES)
> > +             /*
> > +             * The bottom 4 bits of compress_type are for used for the
> > +             * compression type, the other bits for the compression level
> > +             */
> > +             if ((range->compress_type & 0xF) >= BTRFS_NR_COMPRESS_TYPES)
> >                       return -EINVAL;
>
> For the backward compatibility, the check without "& 0xf" is sufficient,
> but a new flag is needed to actually tell the ioctl code to read the
> level. I'm not sure that relying on automatic parsing of the high 4 bits
> is a good idea, usually all new data are accompanied by new flags.

fixed

> >               if (range->compress_type)
> >                       compress_type = range->compress_type;
> > @@ -1572,9 +1576,9 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
> >                       filemap_flush(inode->i_mapping);
> >       }
> >
> > -     if (range->compress_type == BTRFS_COMPRESS_LZO) {
> > +     if ((range->compress_type & 0xF) == BTRFS_COMPRESS_LZO) {
> >               btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
> > -     } else if (range->compress_type == BTRFS_COMPRESS_ZSTD) {
> > +     } else if ((range->compress_type & 0xF) == BTRFS_COMPRESS_ZSTD) {
> >               btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
> >       }
>
> For further iterations of the patch, I suggest to split it into more
> patches by logical change, like adding helpers, extending the ioctl,
> adding asserts, etc.

The ioctl interface is now extended (see patch-extend-ioctl.patch)
for the other patch, see patch-extend-inode.patch
Where should asserts be added?

Re: [PATCH v2] btrfs: allow setting per extent compression

[David Sterba]

On Tue, Apr 21, 2020 at 01:46:59PM +0200, stijn rutjens wrote:
> ""Hi,
> I've added an ioctl flag to notify the interface we want to use the
> upper 4 bits for the compression level (which has significantly
> decreased the size of the patch). How do you want to split the patch
> up?

I'd like to see the patches to answer that.

> > > @@ -1572,9 +1576,9 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
> > >                       filemap_flush(inode->i_mapping);
> > >       }
> > >
> > > -     if (range->compress_type == BTRFS_COMPRESS_LZO) {
> > > +     if ((range->compress_type & 0xF) == BTRFS_COMPRESS_LZO) {
> > >               btrfs_set_fs_incompat(fs_info, COMPRESS_LZO);
> > > -     } else if (range->compress_type == BTRFS_COMPRESS_ZSTD) {
> > > +     } else if ((range->compress_type & 0xF) == BTRFS_COMPRESS_ZSTD) {
> > >               btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
> > >       }
> >
> > For further iterations of the patch, I suggest to split it into more
> > patches by logical change, like adding helpers, extending the ioctl,
> > adding asserts, etc.
> 
> The ioctl interface is now extended (see patch-extend-ioctl.patch)
> for the other patch, see patch-extend-inode.patch
> Where should asserts be added?

Where are the patches? Your mail does not have any attachments.