btrfs-progs: mkfs: add --compression #882

maharmstone · 2024-08-30T10:24:34Z

Adds a --compression option to mkfs.btrfs when using --rootdir.

adam900710

Just some small code style nitpicks.

mkfs/rootdir.c

maharmstone · 2024-08-30T10:58:59Z

Thanks Qu

convert/source-ext2.c

adam900710 · 2024-08-30T10:58:52Z

convert/source-ext2.c

@@ -489,7 +490,8 @@ static int ext2_create_symlink(struct btrfs_trans_handle *trans,
 	pathname = (char *)&(ext2_inode->i_block[0]);
 	BUG_ON(pathname[inode_size] != 0);
 	ret = btrfs_insert_inline_extent(trans, root, objectid, 0,
-					 pathname, inode_size + 1);
+					 pathname, inode_size + 1,
+					 0, inode_size + 1);


inode_size + 1 is here because I've added a separate ram_bytes param to btrfs_insert_inline_extent, because it's no longer necessarily the same as size.

In fact this is a minor problem in the existing convert code. The resulted symlink would be one byte larger than the original path (but inode size is still correct).

E.g.
The original ext4 has a link to file name path:

lrwxrwxrwx 1 root root 4 Sep 3 10:27 path -> path

But after convert, the result looks like this:

item 10 key (267 INODE_ITEM 0) itemoff 15543 itemsize 160 generation 1 transid 1 size 4 nbytes 5 block group 0 mode 120777 links 1 uid 0 gid 0 rdev 0 sequence 0 flags 0x0(none) atime 1725325025.930020384 (2024-09-03 10:27:05) ctime 1725325023.110085974 (2024-09-03 10:27:03) mtime 1725325023.110085974 (2024-09-03 10:27:03) otime 1725325023.110085974 (2024-09-03 10:27:03) item 11 key (267 INODE_REF 256) itemoff 15529 itemsize 14 index 3 namelen 4 name: path item 12 key (267 EXTENT_DATA 0) itemoff 15503 itemsize 26 generation 4 type 0 (inline) inline extent data size 5 ram_bytes 5 compression 0 (none)

Note the link has one extra byte.

It's definitely not your fault nor it's causing problems, but I guess I'll fix it before your patchset.

And the fix for that is here: #884

Since our current plan is to merge subvolume related feature in v6.11 first then compression for v6.12, so we should have plenty time to get the fix merged then this refactor.

The proper fix for that bug is here: #885

In fact the +1 is already causing btrfs/012 random failures.
So that has to be fixed anyway.

Okay, I'll rebase once you've committed your fixes.

adam900710 · 2024-08-30T10:59:11Z

convert/source-reiserfs.c

@@ -538,7 +539,7 @@ static int reiserfs_copy_symlink(struct btrfs_trans_handle *trans,
 	len = get_ih_item_len(tp_item_head(&path));

 	ret = btrfs_insert_inline_extent(trans, root, objectid, 0,
-					 symlink, len + 1);
+					 symlink, len + 1, 0, len + 1);


As in ext2_create_symlink

The same bug as ext2_create_symlink

mkfs/main.c

mkfs/rootdir.c

Now that mkfs.btrfs is adding support for compressing the generated filesystem (kdave/btrfs-progs#882), let's add general support for specifying the compression algorithm and compression level to use. We opt to not parse the specified compression algorithm and instead pass it on as is to the mkfs tool. This has a few benefits: - We support every compression algorithm supported by every tool automatically. - Users don't need to modify systemd-repart if a mkfs tool learns a new compression algorithm in the future - We don't need to maintain a bunch of tables for filesystem to map from our generic compression algorithm enum to the filesystem specific names.

maharmstone · 2024-09-02T15:43:35Z

Pushed new version with changes discussed.

adam900710

Just a really small nitpick. Otherwise the first patch looks perfect to me.

mkfs/rootdir.c

Now that mkfs.btrfs is adding support for compressing the generated filesystem (kdave/btrfs-progs#882), let's add general support for specifying the compression algorithm and compression level to use. We opt to not parse the specified compression algorithm and instead pass it on as is to the mkfs tool. This has a few benefits: - We support every compression algorithm supported by every tool automatically. - Users don't need to modify systemd-repart if a mkfs tool learns a new compression algorithm in the future - We don't need to maintain a bunch of tables for filesystem to map from our generic compression algorithm enum to the filesystem specific names.

Now that mkfs.btrfs is adding support for compressing the generated filesystem (kdave/btrfs-progs#882), let's add general support for specifying the compression algorithm and compression level to use. We opt to not parse the specified compression algorithm and instead pass it on as is to the mkfs tool. This has a few benefits: - We support every compression algorithm supported by every tool automatically. - Users don't need to modify systemd-repart if a mkfs tool learns a new compression algorithm in the future - We don't need to maintain a bunch of tables for filesystem to map from our generic compression algorithm enum to the filesystem specific names. We don't add support for btrfs just yet until the corresponding PR in btrfs-progs is merged.

DaanDeMeyer · 2024-09-03T06:43:10Z

@adam900710 Any chance this could be considered for 6.11 as well? Then we can land support for this in systemd-repart in the upcoming systemd 257 release.

Now that mkfs.btrfs is adding support for compressing the generated filesystem (kdave/btrfs-progs#882), let's add general support for specifying the compression algorithm and compression level to use. We opt to not parse the specified compression algorithm and instead pass it on as is to the mkfs tool. This has a few benefits: - We support every compression algorithm supported by every tool automatically. - Users don't need to modify systemd-repart if a mkfs tool learns a new compression algorithm in the future - We don't need to maintain a bunch of tables for filesystem to map from our generic compression algorithm enum to the filesystem specific names. We don't add support for btrfs just yet until the corresponding PR in btrfs-progs is merged.

adam900710 · 2024-09-03T07:33:01Z

David is not hurry to push so many features in one single release.

And considering this is only for compression, it shouldn't cause too much change except the image size.

Corresponding PR in btrfs-progs: kdave/btrfs-progs#882

DaanDeMeyer · 2024-09-03T08:51:54Z

David is not hurry to push so many features in one single release.

And considering this is only for compression, it shouldn't cause too much change except the image size.

The image size is roughly halved by enabling this in my case, which can make a huge difference.

adam900710 · 2024-09-03T09:07:52Z

Compression is indeed awesome to reduce the image size, but here we really want to go a more stable methods towards new features.

In fact, thanks to the recent refactor (the first patch of the series) from @maharmstone , we have already pinned down a btrfs-convert bug that may lead to invalid soft link.
So I want to be a little more cautious here.

@kdave may give a better roadmap on this, and do the final call.

maharmstone · 2024-09-03T10:29:24Z

Pushed with latest change

Conan-Kudo · 2024-09-04T13:55:24Z

It would be really great for us image build tools if this landed in 6.11 / systemd 257. It becomes possible to create Fedora images mostly offline and unprivileged with these features.

Documentation/mkfs.btrfs.rst

maharmstone · 2024-09-06T16:25:25Z

I've pushed a new version, which includes setting the level and renaming --compression to --compress. I've changed it so that the base patch includes zlib and the subsequent patch adds zstd, on the grounds that zlib is a hard dependency but zstd isn't.

maharmstone · 2024-09-09T10:50:19Z

Rebased against devel

maharmstone · 2024-09-09T17:33:51Z

I've added LZO too, to round it off

There were two major problems with the function add_file_items: it was writing all files sector-by-sector, making compression impossible, and it was assuming that pread would never do a short read. Fix these problems, and create a new helper function read_and_write_extent. Signed-off-by: Mark Harmstone <[email protected]>

Adds an option --compress to mkfs.btrfs, to allow creating files using zlib when using --rootdir. Signed-off-by: Mark Harmstone <[email protected]>

Allows --compress to work with zstd, when compiled in. Signed-off-by: Mark Harmstone <[email protected]>

Allows --compress to work with lzo. Signed-off-by: Mark Harmstone <[email protected]>

maharmstone · 2024-10-15T10:42:25Z

Pushed rebase

adam900710

Sorry I merged the branch by incident, and I can not find a way to re-open the merge request.

It turns out that when modifying the code inside my editor, it's much easier to expose problems, than viewing it through github web.
(Not to mention github hides large chunks of code by default).

adam900710 · 2024-11-05T03:21:06Z

mkfs/rootdir.c

+
+		ret_read = pread(fd, buf + bytes_read, to_read - bytes_read,
+				 file_pos + bytes_read);
+		if (ret_read == -1) {


Please use ret_read < 0, as that's the common practice for detecting an error, not only for the common IO but also all the other btrfs specific calls.

adam900710 · 2024-11-05T03:21:48Z

mkfs/rootdir.c

+ */
+#define MAX_EXTENT_SIZE SZ_1M
+
+static int read_and_write_extent(struct btrfs_trans_handle *trans,


I found the function name read_and_write_extent() a little hard to grasp.

The caller is add_file_items(), what about just a simple add_one_file_item().

Because it's not adding a file item, it's adding an extent.

I'll rename it to add_file_item_extent.

adam900710 · 2024-11-05T03:24:41Z

mkfs/rootdir.c

+static int read_and_write_extent(struct btrfs_trans_handle *trans,
+				 struct btrfs_root *root,
+				 struct btrfs_inode_item *btrfs_inode,
+				 u64 objectid, int fd, u64 file_pos, char *buf,


Everything from fd, buf, size (from st structure), and path_name is only here to help read the file.
The only thing changing is file_pos which is worth a dedicated parameter.

During my initial failed merge, I added an extra helper structure, source_descriptor, to include fd, st, path and buf. Later expands the structure to add compressed_buf for compression.

Such structure is more expandable than adding more and more parameters to the already long list.

adam900710 · 2024-11-05T03:26:36Z

mkfs/rootdir.c

-			error("cannot read %s at offset %llu length %llu: %m",
-				path_name, bytes_read, (unsigned long long)st->st_size);
+			error("cannot read %s at offset %u length %llu: %m",
+				path_name, 0, (unsigned long long)st->st_size);


Since you are here, you can also fix the length output, without using a forced (unsigned long long) conversion, but %zu.

adam900710 · 2024-11-05T03:28:24Z

Documentation/mkfs.btrfs.rst

@@ -212,6 +212,15 @@ OPTIONS

                $ mkfs.btrfs -O list-all

+--compress <algo>:<level>


Since level is optional, please use --compress <algo>[:<level>] instead.

adam900710 · 2024-11-05T03:31:03Z

common/extent-tree-utils.c

@@ -285,3 +285,93 @@ int btrfs_record_file_extent(struct btrfs_trans_handle *trans,
 	}
 	return ret;
 }
+
+int btrfs_record_file_extent_comp(struct btrfs_trans_handle *trans,


Please do no use comp as a short name for compress, it's a little confusing.

But it will no longer be a problem if we address the next comment.

It turns out that we should not use btrfs_record_file_extent() except for convert.

The extra handling inside btrfs_record_file_extent() is specially designed for convert, where we may have an existing data extent from the convert image, thus it is not a good way to support compressed data extents.

For mkfs cases, I'll craft a dedicated help for it with a more expandable interface during my cleanup.

adam900710 · 2024-11-05T03:32:42Z

common/extent-tree-utils.c

+				  struct btrfs_inode_item *inode,
+				  u64 file_pos, u64 disk_bytenr,
+				  u64 extent_num_bytes, u64 ram_bytes,
+				  enum btrfs_compression_type comp)


Instead of the longer and longer parameter list, what about something just like btrfs_ordered_extent inside the kernel, to record all the needed info?

So that we do not need two different btrfs_record_file_extent(), but a single one to handle them all, just like kernel.

This will need a dedicated cleanup patch first, to introduce some structure like btrfs_ordered_extent.

adam900710 · 2024-11-05T03:33:41Z

mkfs/main.c

@@ -443,6 +443,7 @@ static const char * const mkfs_usage[] = {
 	OPTLINE("-u|--subvol TYPE:SUBDIR", "create SUBDIR as subvolume rather than normal directory, can be specified multiple times"),
 	OPTLINE("--shrink", "(with --rootdir) shrink the filled filesystem to minimal size"),
 	OPTLINE("-K|--nodiscard", "do not perform whole device TRIM"),
+	OPTLINE("--compress ALGO:LEVEL", "compression algorithm and level to use; ALGO can be no (default), zlib"),


Also [] for the level.

adam900710 · 2024-11-05T03:40:13Z

mkfs/rootdir.c

+			u64 flags;
+
+			flags = btrfs_stack_inode_flags(btrfs_inode);
+			flags |= BTRFS_INODE_NOCOMPRESS;


I prefer not to pass a btrfs inode and handle its flags inside a compression function.

Such compression function should only do the compression. And let the caller handling the btrfs specific things.
(E.g. if the compressed size is larger than the input, return -E2BIG to inform the caller just like what kernel does).

adam900710 · 2024-11-05T03:42:45Z

mkfs/rootdir.c

+			break;
+		}
+
+		if (ret == 1) {


Please do not use immediate number other than 0, so this should be if (ret > 0) {

adam900710 · 2024-11-05T21:04:50Z

BTW, I have submitted the series cleaning up the file extent insertion code:
#919

Now for mkfs rootdir usage, we should go the new helper, insert_reserved_file_extent(), and it accepts an on-stack file extent item pointer, so caller can specify all the members, just like what we did inside the kernel.

This should allow the function to be utilized by both regular and compressed file extents, again, just like the kernel.

adam900710 reviewed Aug 30, 2024

View reviewed changes

mkfs/rootdir.c Outdated Show resolved Hide resolved

mkfs/rootdir.c Outdated Show resolved Hide resolved

mkfs/rootdir.c Outdated Show resolved Hide resolved

mkfs/rootdir.c Outdated Show resolved Hide resolved

adam900710 reviewed Aug 30, 2024

View reviewed changes

DaanDeMeyer mentioned this pull request Aug 30, 2024

repart: Add compression support systemd/systemd#34190

Merged

maharmstone force-pushed the compression branch from c1ce75f to 0aa0bc1 Compare September 2, 2024 15:32

adam900710 added this to the v6.12 milestone Sep 3, 2024

adam900710 approved these changes Sep 3, 2024

View reviewed changes

mkfs/rootdir.c Outdated Show resolved Hide resolved

DaanDeMeyer added a commit to DaanDeMeyer/systemd that referenced this pull request Sep 3, 2024

mkfs-util: Add support for btrfs compression

7fba5ee

Corresponding PR in btrfs-progs: kdave/btrfs-progs#882

DaanDeMeyer mentioned this pull request Sep 3, 2024

mkfs-util: Add support for btrfs compression systemd/systemd#34239

Open

maharmstone force-pushed the compression branch 2 times, most recently from ac86e57 to 802c51b Compare September 3, 2024 10:18

adam900710 added enhancement mkfs Changes in mkfs.btrfs labels Sep 4, 2024

kdave requested changes Sep 4, 2024

View reviewed changes

Documentation/mkfs.btrfs.rst Outdated Show resolved Hide resolved

maharmstone force-pushed the compression branch 2 times, most recently from 34f2cab to 6f29be7 Compare September 6, 2024 16:24

maharmstone force-pushed the compression branch from 6f29be7 to e6a2823 Compare September 9, 2024 10:28

maharmstone requested a review from kdave September 9, 2024 10:30

kdave force-pushed the devel branch 2 times, most recently from f6d1943 to 5ff9f62 Compare September 17, 2024 15:00

maharmstone added 4 commits October 15, 2024 11:14

btrfs-progs: mkfs: add --compress option

82cec85

Adds an option --compress to mkfs.btrfs, to allow creating files using zlib when using --rootdir. Signed-off-by: Mark Harmstone <[email protected]>

btrfs-progs: mkfs: add zstd to --compress option

c614436

Allows --compress to work with zstd, when compiled in. Signed-off-by: Mark Harmstone <[email protected]>

btrfs-progs: mkfs: add lzo to --compress option

094d7ef

Allows --compress to work with lzo. Signed-off-by: Mark Harmstone <[email protected]>

maharmstone force-pushed the compression branch from 196e7a7 to 094d7ef Compare October 15, 2024 10:20

adam900710 force-pushed the devel branch 2 times, most recently from b7cd74f to 66f08f9 Compare November 4, 2024 08:01

adam900710 merged commit 084228c into kdave:devel Nov 5, 2024

adam900710 mentioned this pull request Nov 5, 2024

Revert "btrfs-progs: mkfs: add --compression" #918

Merged

adam900710 reviewed Nov 5, 2024

View reviewed changes

maharmstone mentioned this pull request Nov 19, 2024

btrfs-progs: mkfs: add --compress #925

Open

		@@ -212,6 +212,15 @@ OPTIONS

		$ mkfs.btrfs -O list-all

		--compress <algo>:<level>

btrfs-progs: mkfs: add --compression #882

btrfs-progs: mkfs: add --compression #882

Conversation

maharmstone commented Aug 30, 2024

adam900710 left a comment

Choose a reason for hiding this comment

maharmstone commented Aug 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam900710 Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

adam900710 Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maharmstone commented Sep 2, 2024

adam900710 left a comment • edited Loading

Choose a reason for hiding this comment

DaanDeMeyer commented Sep 3, 2024

adam900710 commented Sep 3, 2024

DaanDeMeyer commented Sep 3, 2024

adam900710 commented Sep 3, 2024 • edited Loading

maharmstone commented Sep 3, 2024

Conan-Kudo commented Sep 4, 2024

maharmstone commented Sep 6, 2024

maharmstone commented Sep 9, 2024

maharmstone commented Sep 9, 2024

maharmstone commented Oct 15, 2024

adam900710 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam900710 commented Nov 5, 2024

adam900710 Sep 3, 2024 •

edited

Loading

adam900710 Sep 3, 2024 •

edited

Loading

adam900710 left a comment •

edited

Loading

adam900710 commented Sep 3, 2024 •

edited

Loading