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