| Message ID | 20250304171403.571335-1-neelx@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs/defrag: implement compression levels | expand |
The feature itself looks good to me. Although not sure if a blank commit message is fine for this case. 在 2025/3/5 03:44, Daniel Vacek 写道: > Signed-off-by: Daniel Vacek <neelx@suse.com> > --- > fs/btrfs/btrfs_inode.h | 2 ++ > fs/btrfs/defrag.c | 22 +++++++++++++++++----- > fs/btrfs/fs.h | 2 +- > fs/btrfs/inode.c | 10 +++++++--- > include/uapi/linux/btrfs.h | 10 +++++++++- > 5 files changed, 36 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > index aa1f55cd81b79..5ee9da0054a74 100644 > --- a/fs/btrfs/btrfs_inode.h > +++ b/fs/btrfs/btrfs_inode.h > @@ -145,6 +145,7 @@ struct btrfs_inode { > * different from prop_compress and takes precedence if set. > */ > u8 defrag_compress; > + s8 defrag_compress_level; > > /* > * Lock for counters and all fields used to determine if the inode is in > diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c > index 968dae9539482..03a0287a78ea0 100644 > --- a/fs/btrfs/defrag.c > +++ b/fs/btrfs/defrag.c > @@ -1363,6 +1363,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > u64 last_byte; > bool do_compress = (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS); > int compress_type = BTRFS_COMPRESS_ZLIB; > + int compress_level = 0; > int ret = 0; > u32 extent_thresh = range->extent_thresh; > pgoff_t start_index; > @@ -1376,10 +1377,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > return -EINVAL; > > if (do_compress) { > - if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > - return -EINVAL; > - if (range->compress_type) > - compress_type = range->compress_type; > + if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL) { > + if (range->compress.type >= BTRFS_NR_COMPRESS_TYPES) > + return -EINVAL; > + if (range->compress.type) { > + compress_type = range->compress.type; > + compress_level= range->compress.level; > + } I am not familiar with the compress level, but btrfs_compress_set_level() does extra clamping, maybe we also want to do that too? > + } else { > + if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > + return -EINVAL; > + if (range->compress_type) > + compress_type = range->compress_type; > + } > } > > if (extent_thresh == 0) > @@ -1430,8 +1440,10 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > btrfs_inode_unlock(BTRFS_I(inode), 0); > break; > } > - if (do_compress) > + if (do_compress) { > BTRFS_I(inode)->defrag_compress = compress_type; > + BTRFS_I(inode)->defrag_compress_level = compress_level; > + } > ret = defrag_one_cluster(BTRFS_I(inode), ra, cur, > cluster_end + 1 - cur, extent_thresh, > newer_than, do_compress, §ors_defragged, > diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h > index be6d5a24bd4e6..2dae7ffd37133 100644 > --- a/fs/btrfs/fs.h > +++ b/fs/btrfs/fs.h > @@ -485,7 +485,7 @@ struct btrfs_fs_info { > u64 last_trans_log_full_commit; > unsigned long long mount_opt; > > - unsigned long compress_type:4; > + int compress_type; > int compress_level; > u32 commit_interval; > /* > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index fa04b027d53ac..156a9d4603391 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -925,6 +925,7 @@ static void compress_file_range(struct btrfs_work *work) > unsigned int poff; > int i; > int compress_type = fs_info->compress_type; > + int compress_level= fs_info->compress_level; > > inode_should_defrag(inode, start, end, end - start + 1, SZ_16K); > > @@ -1007,13 +1008,15 @@ static void compress_file_range(struct btrfs_work *work) > goto cleanup_and_bail_uncompressed; > } > > - if (inode->defrag_compress) > + if (inode->defrag_compress) { > compress_type = inode->defrag_compress; > - else if (inode->prop_compress) > + compress_level= inode->defrag_compress_level; > + } else if (inode->prop_compress) { > compress_type = inode->prop_compress; > + } > > /* Compression level is applied here. */ > - ret = btrfs_compress_folios(compress_type, fs_info->compress_level, > + ret = btrfs_compress_folios(compress_type, compress_level, > mapping, start, folios, &nr_folios, &total_in, > &total_compressed); > if (ret) > diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h > index d3b222d7af240..3540d33d6f50c 100644 > --- a/include/uapi/linux/btrfs.h > +++ b/include/uapi/linux/btrfs.h > @@ -615,7 +615,9 @@ struct btrfs_ioctl_clone_range_args { > */ > #define BTRFS_DEFRAG_RANGE_COMPRESS 1 > #define BTRFS_DEFRAG_RANGE_START_IO 2 > +#define BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL 4 > #define BTRFS_DEFRAG_RANGE_FLAGS_SUPP (BTRFS_DEFRAG_RANGE_COMPRESS | \ > + BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL | \ > BTRFS_DEFRAG_RANGE_START_IO) > > struct btrfs_ioctl_defrag_range_args { > @@ -643,7 +645,13 @@ struct btrfs_ioctl_defrag_range_args { > * for this defrag operation. If unspecified, zlib will > * be used > */ > - __u32 compress_type; > + union { > + __u32 compress_type; > + struct { > + __u8 type; > + __s8 level; > + } compress; > + }; > > /* spare for later */ > __u32 unused[4]; We have enough space left here, although u32 is overkilled for compress_type, using the unused space for a new s8/s16/s32 member should be fine. Thanks, Qu
On Wed, Mar 05, 2025 at 08:01:24AM +1030, Qu Wenruo wrote: > The feature itself looks good to me. > > Although not sure if a blank commit message is fine for this case. > > 在 2025/3/5 03:44, Daniel Vacek 写道: > > Signed-off-by: Daniel Vacek <neelx@suse.com> > > --- > > fs/btrfs/btrfs_inode.h | 2 ++ > > fs/btrfs/defrag.c | 22 +++++++++++++++++----- > > fs/btrfs/fs.h | 2 +- > > fs/btrfs/inode.c | 10 +++++++--- > > include/uapi/linux/btrfs.h | 10 +++++++++- > > 5 files changed, 36 insertions(+), 10 deletions(-) > > > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > > index aa1f55cd81b79..5ee9da0054a74 100644 > > --- a/fs/btrfs/btrfs_inode.h > > +++ b/fs/btrfs/btrfs_inode.h > > @@ -145,6 +145,7 @@ struct btrfs_inode { > > * different from prop_compress and takes precedence if set. > > */ > > u8 defrag_compress; > > + s8 defrag_compress_level; > > > > /* > > * Lock for counters and all fields used to determine if the inode is in > > diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c > > index 968dae9539482..03a0287a78ea0 100644 > > --- a/fs/btrfs/defrag.c > > +++ b/fs/btrfs/defrag.c > > @@ -1363,6 +1363,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > u64 last_byte; > > bool do_compress = (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS); > > int compress_type = BTRFS_COMPRESS_ZLIB; > > + int compress_level = 0; > > int ret = 0; > > u32 extent_thresh = range->extent_thresh; > > pgoff_t start_index; > > @@ -1376,10 +1377,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > return -EINVAL; > > > > if (do_compress) { > > - if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > > - return -EINVAL; > > - if (range->compress_type) > > - compress_type = range->compress_type; > > + if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL) { > > + if (range->compress.type >= BTRFS_NR_COMPRESS_TYPES) > > + return -EINVAL; > > + if (range->compress.type) { > > + compress_type = range->compress.type; > > + compress_level= range->compress.level; > > + } > > I am not familiar with the compress level, but > btrfs_compress_set_level() does extra clamping, maybe we also want to do > that too? Yes the level needs to be validated here as well. > > @@ -643,7 +645,13 @@ struct btrfs_ioctl_defrag_range_args { > > * for this defrag operation. If unspecified, zlib will > > * be used > > */ > > - __u32 compress_type; > > + union { > > + __u32 compress_type; > > + struct { > > + __u8 type; > > + __s8 level; > > + } compress; > > + }; > > > > /* spare for later */ > > __u32 unused[4]; > > We have enough space left here, although u32 is overkilled for > compress_type, using the unused space for a new s8/s16/s32 member should > be fine. I suggested to do it like that, u32 is wasting space and the union trick reusing existing space was already done e.g. in the balance filters.
On Tue, 4 Mar 2025 at 22:31, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > The feature itself looks good to me. > > Although not sure if a blank commit message is fine for this case. Sigh, that's a mistake. I'll send a v2 with a few sentences. > 在 2025/3/5 03:44, Daniel Vacek 写道: > > Signed-off-by: Daniel Vacek <neelx@suse.com> > > --- > > fs/btrfs/btrfs_inode.h | 2 ++ > > fs/btrfs/defrag.c | 22 +++++++++++++++++----- > > fs/btrfs/fs.h | 2 +- > > fs/btrfs/inode.c | 10 +++++++--- > > include/uapi/linux/btrfs.h | 10 +++++++++- > > 5 files changed, 36 insertions(+), 10 deletions(-) > > > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > > index aa1f55cd81b79..5ee9da0054a74 100644 > > --- a/fs/btrfs/btrfs_inode.h > > +++ b/fs/btrfs/btrfs_inode.h > > @@ -145,6 +145,7 @@ struct btrfs_inode { > > * different from prop_compress and takes precedence if set. > > */ > > u8 defrag_compress; > > + s8 defrag_compress_level; > > > > /* > > * Lock for counters and all fields used to determine if the inode is in > > diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c > > index 968dae9539482..03a0287a78ea0 100644 > > --- a/fs/btrfs/defrag.c > > +++ b/fs/btrfs/defrag.c > > @@ -1363,6 +1363,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > u64 last_byte; > > bool do_compress = (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS); > > int compress_type = BTRFS_COMPRESS_ZLIB; > > + int compress_level = 0; > > int ret = 0; > > u32 extent_thresh = range->extent_thresh; > > pgoff_t start_index; > > @@ -1376,10 +1377,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > return -EINVAL; > > > > if (do_compress) { > > - if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > > - return -EINVAL; > > - if (range->compress_type) > > - compress_type = range->compress_type; > > + if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL) { > > + if (range->compress.type >= BTRFS_NR_COMPRESS_TYPES) > > + return -EINVAL; > > + if (range->compress.type) { > > + compress_type = range->compress.type; > > + compress_level= range->compress.level; > > + } > > I am not familiar with the compress level, but > btrfs_compress_set_level() does extra clamping, maybe we also want to do > that too? This is intentionally left to be limited later. There's no need to do it at this point and the code is simpler. It's also compression type/method agnostic. > > + } else { > > + if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > > + return -EINVAL; > > + if (range->compress_type) > > + compress_type = range->compress_type; > > + } > > } > > > > if (extent_thresh == 0) > > @@ -1430,8 +1440,10 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > btrfs_inode_unlock(BTRFS_I(inode), 0); > > break; > > } > > - if (do_compress) > > + if (do_compress) { > > BTRFS_I(inode)->defrag_compress = compress_type; > > + BTRFS_I(inode)->defrag_compress_level = compress_level; > > + } > > ret = defrag_one_cluster(BTRFS_I(inode), ra, cur, > > cluster_end + 1 - cur, extent_thresh, > > newer_than, do_compress, §ors_defragged, > > diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h > > index be6d5a24bd4e6..2dae7ffd37133 100644 > > --- a/fs/btrfs/fs.h > > +++ b/fs/btrfs/fs.h > > @@ -485,7 +485,7 @@ struct btrfs_fs_info { > > u64 last_trans_log_full_commit; > > unsigned long long mount_opt; > > > > - unsigned long compress_type:4; > > + int compress_type; > > int compress_level; > > u32 commit_interval; > > /* > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index fa04b027d53ac..156a9d4603391 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -925,6 +925,7 @@ static void compress_file_range(struct btrfs_work *work) > > unsigned int poff; > > int i; > > int compress_type = fs_info->compress_type; > > + int compress_level= fs_info->compress_level; > > > > inode_should_defrag(inode, start, end, end - start + 1, SZ_16K); > > > > @@ -1007,13 +1008,15 @@ static void compress_file_range(struct btrfs_work *work) > > goto cleanup_and_bail_uncompressed; > > } > > > > - if (inode->defrag_compress) > > + if (inode->defrag_compress) { > > compress_type = inode->defrag_compress; > > - else if (inode->prop_compress) > > + compress_level= inode->defrag_compress_level; > > + } else if (inode->prop_compress) { > > compress_type = inode->prop_compress; > > + } > > > > /* Compression level is applied here. */ > > - ret = btrfs_compress_folios(compress_type, fs_info->compress_level, > > + ret = btrfs_compress_folios(compress_type, compress_level, > > mapping, start, folios, &nr_folios, &total_in, > > &total_compressed); > > if (ret) > > diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h > > index d3b222d7af240..3540d33d6f50c 100644 > > --- a/include/uapi/linux/btrfs.h > > +++ b/include/uapi/linux/btrfs.h > > @@ -615,7 +615,9 @@ struct btrfs_ioctl_clone_range_args { > > */ > > #define BTRFS_DEFRAG_RANGE_COMPRESS 1 > > #define BTRFS_DEFRAG_RANGE_START_IO 2 > > +#define BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL 4 > > #define BTRFS_DEFRAG_RANGE_FLAGS_SUPP (BTRFS_DEFRAG_RANGE_COMPRESS | \ > > + BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL | \ > > BTRFS_DEFRAG_RANGE_START_IO) > > > > struct btrfs_ioctl_defrag_range_args { > > @@ -643,7 +645,13 @@ struct btrfs_ioctl_defrag_range_args { > > * for this defrag operation. If unspecified, zlib will > > * be used > > */ > > - __u32 compress_type; > > + union { > > + __u32 compress_type; > > + struct { > > + __u8 type; > > + __s8 level; > > + } compress; > > + }; > > > > /* spare for later */ > > __u32 unused[4]; > > We have enough space left here, although u32 is overkilled for > compress_type, using the unused space for a new s8/s16/s32 member should > be fine. That is what I did originally, but discussing with Dave he suggested this solution. > > Thanks, > Qu
On Wed, Mar 05, 2025 at 08:02:28AM +0100, Daniel Vacek wrote: > > > @@ -1376,10 +1377,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > > return -EINVAL; > > > > > > if (do_compress) { > > > - if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > > > - return -EINVAL; > > > - if (range->compress_type) > > > - compress_type = range->compress_type; > > > + if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL) { > > > + if (range->compress.type >= BTRFS_NR_COMPRESS_TYPES) > > > + return -EINVAL; > > > + if (range->compress.type) { > > > + compress_type = range->compress.type; > > > + compress_level= range->compress.level; > > > + } > > > > I am not familiar with the compress level, but > > btrfs_compress_set_level() does extra clamping, maybe we also want to do > > that too? > > This is intentionally left to be limited later. There's no need to do > it at this point and the code is simpler. It's also compression > type/method agnostic. This is input parameter validation so we should not postpone it until the whole process starts. The complexity can be wrapped in helpers, we already have that for various purposes like compression_decompress_bio().
On Wed, 5 Mar 2025 at 08:02, David Sterba <dsterba@suse.cz> wrote: > > On Wed, Mar 05, 2025 at 08:01:24AM +1030, Qu Wenruo wrote: > > The feature itself looks good to me. > > > > Although not sure if a blank commit message is fine for this case. > > > > 在 2025/3/5 03:44, Daniel Vacek 写道: > > > Signed-off-by: Daniel Vacek <neelx@suse.com> > > > --- > > > fs/btrfs/btrfs_inode.h | 2 ++ > > > fs/btrfs/defrag.c | 22 +++++++++++++++++----- > > > fs/btrfs/fs.h | 2 +- > > > fs/btrfs/inode.c | 10 +++++++--- > > > include/uapi/linux/btrfs.h | 10 +++++++++- > > > 5 files changed, 36 insertions(+), 10 deletions(-) > > > > > > diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h > > > index aa1f55cd81b79..5ee9da0054a74 100644 > > > --- a/fs/btrfs/btrfs_inode.h > > > +++ b/fs/btrfs/btrfs_inode.h > > > @@ -145,6 +145,7 @@ struct btrfs_inode { > > > * different from prop_compress and takes precedence if set. > > > */ > > > u8 defrag_compress; > > > + s8 defrag_compress_level; > > > > > > /* > > > * Lock for counters and all fields used to determine if the inode is in > > > diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c > > > index 968dae9539482..03a0287a78ea0 100644 > > > --- a/fs/btrfs/defrag.c > > > +++ b/fs/btrfs/defrag.c > > > @@ -1363,6 +1363,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > > u64 last_byte; > > > bool do_compress = (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS); > > > int compress_type = BTRFS_COMPRESS_ZLIB; > > > + int compress_level = 0; > > > int ret = 0; > > > u32 extent_thresh = range->extent_thresh; > > > pgoff_t start_index; > > > @@ -1376,10 +1377,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > > return -EINVAL; > > > > > > if (do_compress) { > > > - if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > > > - return -EINVAL; > > > - if (range->compress_type) > > > - compress_type = range->compress_type; > > > + if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL) { > > > + if (range->compress.type >= BTRFS_NR_COMPRESS_TYPES) > > > + return -EINVAL; > > > + if (range->compress.type) { > > > + compress_type = range->compress.type; > > > + compress_level= range->compress.level; > > > + } > > > > I am not familiar with the compress level, but > > btrfs_compress_set_level() does extra clamping, maybe we also want to do > > that too? > > Yes the level needs to be validated here as well. The level is passed to btrfs_compress_folios() in compress_file_range() and the first thing it does is precisely btrfs_compress_set_level(). So I thought it's not needed to be done twice. > > > @@ -643,7 +645,13 @@ struct btrfs_ioctl_defrag_range_args { > > > * for this defrag operation. If unspecified, zlib will > > > * be used > > > */ > > > - __u32 compress_type; > > > + union { > > > + __u32 compress_type; > > > + struct { > > > + __u8 type; > > > + __s8 level; > > > + } compress; > > > + }; > > > > > > /* spare for later */ > > > __u32 unused[4]; > > > > We have enough space left here, although u32 is overkilled for > > compress_type, using the unused space for a new s8/s16/s32 member should > > be fine. > > I suggested to do it like that, u32 is wasting space and the union trick > reusing existing space was already done e.g. in the balance filters.
On Wed, 5 Mar 2025 at 08:05, David Sterba <dsterba@suse.cz> wrote: > > On Wed, Mar 05, 2025 at 08:02:28AM +0100, Daniel Vacek wrote: > > > > @@ -1376,10 +1377,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, > > > > return -EINVAL; > > > > > > > > if (do_compress) { > > > > - if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) > > > > - return -EINVAL; > > > > - if (range->compress_type) > > > > - compress_type = range->compress_type; > > > > + if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL) { > > > > + if (range->compress.type >= BTRFS_NR_COMPRESS_TYPES) > > > > + return -EINVAL; > > > > + if (range->compress.type) { > > > > + compress_type = range->compress.type; > > > > + compress_level= range->compress.level; > > > > + } > > > > > > I am not familiar with the compress level, but > > > btrfs_compress_set_level() does extra clamping, maybe we also want to do > > > that too? > > > > This is intentionally left to be limited later. There's no need to do > > it at this point and the code is simpler. It's also compression > > type/method agnostic. > > This is input parameter validation so we should not postpone it until > the whole process starts. The complexity can be wrapped in helpers, we > already have that for various purposes like > compression_decompress_bio(). OK, that makes sense. I'll add the check in v2. Thaks guys.
在 2025/3/5 17:32, Daniel Vacek 写道: > On Tue, 4 Mar 2025 at 22:31, Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: [...] >> >> I am not familiar with the compress level, but >> btrfs_compress_set_level() does extra clamping, maybe we also want to do >> that too? > > This is intentionally left to be limited later. There's no need to do > it at this point and the code is simpler. It's also compression > type/method agnostic. You're right, the level checks are done in the compression path already. [...] >>> /* spare for later */ >>> __u32 unused[4]; >> >> We have enough space left here, although u32 is overkilled for >> compress_type, using the unused space for a new s8/s16/s32 member should >> be fine. > > That is what I did originally, but discussing with Dave he suggested > this solution. Normally I would be fine with the union, to save some memory. Maybe I'm a little paranoid, but the defrag ioctl flag check is only introduced last year by commit 173431b274a9 ("btrfs: defrag: reject unknown flags of btrfs_ioctl_defrag_range_args"). So it's possible that some older kernels don't have that commit, and may incorrectly continue by ignoring the flag. Thankfully that should fail with -EINVAL (type always in the higher bits, thus always tricking the NR_COMPRESS_TYPES check. If that layout (type in higher bits, level in lower bits) is intentionally, I'd say it's very clever. Anyway either solution looks fine to me now. With that commit message fixed: Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > >> >> Thanks, >> Qu >
On Wed, Mar 05, 2025 at 06:14:16PM +1030, Qu Wenruo wrote: > [...] > >>> /* spare for later */ > >>> __u32 unused[4]; > >> > >> We have enough space left here, although u32 is overkilled for > >> compress_type, using the unused space for a new s8/s16/s32 member should > >> be fine. > > > > That is what I did originally, but discussing with Dave he suggested > > this solution. > > Normally I would be fine with the union, to save some memory. > > Maybe I'm a little paranoid, but the defrag ioctl flag check is only > introduced last year by commit 173431b274a9 ("btrfs: defrag: reject > unknown flags of btrfs_ioctl_defrag_range_args"). The commit has been backported to stable trees 4.19.307 5.10.210 5.15.149 5.4.269 6.1.76 6.6.15 6.7.3 , so we could assume the flags are validated. > So it's possible that some older kernels don't have that commit, and may > incorrectly continue by ignoring the flag. > Thankfully that should fail with -EINVAL (type always in the higher > bits, thus always tricking the NR_COMPRESS_TYPES check. > > If that layout (type in higher bits, level in lower bits) is > intentionally, I'd say it's very clever. > > Anyway either solution looks fine to me now. The layout also depends on endianness, but should not matter as long as the flgags are validated. If not, either the level is ignored or it fails due to the >= NR_COMPRESS_TYPES check. Both should be acceptable as fallback.
在 2025/3/5 18:31, David Sterba 写道: > On Wed, Mar 05, 2025 at 06:14:16PM +1030, Qu Wenruo wrote: >> [...] >>>>> /* spare for later */ >>>>> __u32 unused[4]; >>>> >>>> We have enough space left here, although u32 is overkilled for >>>> compress_type, using the unused space for a new s8/s16/s32 member should >>>> be fine. >>> >>> That is what I did originally, but discussing with Dave he suggested >>> this solution. >> >> Normally I would be fine with the union, to save some memory. >> >> Maybe I'm a little paranoid, but the defrag ioctl flag check is only >> introduced last year by commit 173431b274a9 ("btrfs: defrag: reject >> unknown flags of btrfs_ioctl_defrag_range_args"). > > The commit has been backported to stable trees 4.19.307 5.10.210 5.15.149 > 5.4.269 6.1.76 6.6.15 6.7.3 , so we could assume the flags are > validated. I know it's backported to all stable kernels, but I still won't consider a patch that's only one year old to be applied to all the kernels in the wide. Maybe I'm really paranoid on this. > >> So it's possible that some older kernels don't have that commit, and may >> incorrectly continue by ignoring the flag. >> Thankfully that should fail with -EINVAL (type always in the higher >> bits, thus always tricking the NR_COMPRESS_TYPES check. >> >> If that layout (type in higher bits, level in lower bits) is >> intentionally, I'd say it's very clever. >> >> Anyway either solution looks fine to me now. > > The layout also depends on endianness, but should not matter as long as > the flgags are validated. If not, either the level is ignored or it > fails due to the >= NR_COMPRESS_TYPES check. Both should be acceptable > as fallback. Since the fallback behavior is sane, I'm fine with the union solution. Thanks, Qu
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h index aa1f55cd81b79..5ee9da0054a74 100644 --- a/fs/btrfs/btrfs_inode.h +++ b/fs/btrfs/btrfs_inode.h @@ -145,6 +145,7 @@ struct btrfs_inode { * different from prop_compress and takes precedence if set. */ u8 defrag_compress; + s8 defrag_compress_level; /* * Lock for counters and all fields used to determine if the inode is in diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c index 968dae9539482..03a0287a78ea0 100644 --- a/fs/btrfs/defrag.c +++ b/fs/btrfs/defrag.c @@ -1363,6 +1363,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, u64 last_byte; bool do_compress = (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS); int compress_type = BTRFS_COMPRESS_ZLIB; + int compress_level = 0; int ret = 0; u32 extent_thresh = range->extent_thresh; pgoff_t start_index; @@ -1376,10 +1377,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, return -EINVAL; if (do_compress) { - if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) - return -EINVAL; - if (range->compress_type) - compress_type = range->compress_type; + if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL) { + if (range->compress.type >= BTRFS_NR_COMPRESS_TYPES) + return -EINVAL; + if (range->compress.type) { + compress_type = range->compress.type; + compress_level= range->compress.level; + } + } else { + if (range->compress_type >= BTRFS_NR_COMPRESS_TYPES) + return -EINVAL; + if (range->compress_type) + compress_type = range->compress_type; + } } if (extent_thresh == 0) @@ -1430,8 +1440,10 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra, btrfs_inode_unlock(BTRFS_I(inode), 0); break; } - if (do_compress) + if (do_compress) { BTRFS_I(inode)->defrag_compress = compress_type; + BTRFS_I(inode)->defrag_compress_level = compress_level; + } ret = defrag_one_cluster(BTRFS_I(inode), ra, cur, cluster_end + 1 - cur, extent_thresh, newer_than, do_compress, §ors_defragged, diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h index be6d5a24bd4e6..2dae7ffd37133 100644 --- a/fs/btrfs/fs.h +++ b/fs/btrfs/fs.h @@ -485,7 +485,7 @@ struct btrfs_fs_info { u64 last_trans_log_full_commit; unsigned long long mount_opt; - unsigned long compress_type:4; + int compress_type; int compress_level; u32 commit_interval; /* diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index fa04b027d53ac..156a9d4603391 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -925,6 +925,7 @@ static void compress_file_range(struct btrfs_work *work) unsigned int poff; int i; int compress_type = fs_info->compress_type; + int compress_level= fs_info->compress_level; inode_should_defrag(inode, start, end, end - start + 1, SZ_16K); @@ -1007,13 +1008,15 @@ static void compress_file_range(struct btrfs_work *work) goto cleanup_and_bail_uncompressed; } - if (inode->defrag_compress) + if (inode->defrag_compress) { compress_type = inode->defrag_compress; - else if (inode->prop_compress) + compress_level= inode->defrag_compress_level; + } else if (inode->prop_compress) { compress_type = inode->prop_compress; + } /* Compression level is applied here. */ - ret = btrfs_compress_folios(compress_type, fs_info->compress_level, + ret = btrfs_compress_folios(compress_type, compress_level, mapping, start, folios, &nr_folios, &total_in, &total_compressed); if (ret) diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h index d3b222d7af240..3540d33d6f50c 100644 --- a/include/uapi/linux/btrfs.h +++ b/include/uapi/linux/btrfs.h @@ -615,7 +615,9 @@ struct btrfs_ioctl_clone_range_args { */ #define BTRFS_DEFRAG_RANGE_COMPRESS 1 #define BTRFS_DEFRAG_RANGE_START_IO 2 +#define BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL 4 #define BTRFS_DEFRAG_RANGE_FLAGS_SUPP (BTRFS_DEFRAG_RANGE_COMPRESS | \ + BTRFS_DEFRAG_RANGE_COMPRESS_LEVEL | \ BTRFS_DEFRAG_RANGE_START_IO) struct btrfs_ioctl_defrag_range_args { @@ -643,7 +645,13 @@ struct btrfs_ioctl_defrag_range_args { * for this defrag operation. If unspecified, zlib will * be used */ - __u32 compress_type; + union { + __u32 compress_type; + struct { + __u8 type; + __s8 level; + } compress; + }; /* spare for later */ __u32 unused[4];
Signed-off-by: Daniel Vacek <neelx@suse.com> --- fs/btrfs/btrfs_inode.h | 2 ++ fs/btrfs/defrag.c | 22 +++++++++++++++++----- fs/btrfs/fs.h | 2 +- fs/btrfs/inode.c | 10 +++++++--- include/uapi/linux/btrfs.h | 10 +++++++++- 5 files changed, 36 insertions(+), 10 deletions(-)