Btrfs subvolume creation #19849

jelly · 2024-01-16T19:36:03Z

Support creating and optionally mounting subvolumes. Subvolumes can only be created when any parent in the tree is mounted.

pkg/storaged/stratis/pool.jsx

pkg/storaged/btrfs/subvolume.jsx

pkg/storaged/block/format-dialog.jsx

pkg/storaged/btrfs/subvolume.jsx

mvollmer · 2024-01-19T08:51:29Z

pkg/storaged/btrfs/subvolume.jsx

+    if (!mounted && subvol.id == 5)
+        create_excuse = _("Subvolume needs to be mounted");
+    if (mounted && opt_ro)
+        create_excuse = _("Subvolume needs to be mounted rw");


Let's not say "rw" but "Subvolume needs to be writable".

Also, I think creation is possible if one of the parents is mounted rw, and get_mount_point_in_parent takes that into account.

So what about this:

if (!mount_point_in_parent) { if (!mounted) create_excuse = _("Subvolume needs to be mounted"); if (!mounted && subvol.id == 5) create_excuse = _("Subvolume needs to be mounted"); if (mounted && opt_ro) create_excuse = _("Subvolume needs to be mounted writable"); }

Thus, we only talk to the user in terms of the current subvolume and never about parents.

Hmm I don't think this is true. mount_point_in_parent returns the mount_point for the current parent and doesn't become true. It breaks the current test_case for mounted && opt_ro.

/dev/sda on /run/ro type btrfs (ro,relatime,seclabel,ssd,space_cache=v2,subvolid=264,subvol=/ro)

It breaks the current test_case

There is a test case where we expect that creation inside the subvolume "ro" (mounted at "/run/ro") should be disabled, but with my proposed changes, it is enabled. Right?

This happens because the root subvol is still mounted read-write at "/run/butter" and creation of subvolumes of "ro" can be done inside the mount point "/run/butter/ro".

(I think, I'll check this now.)

See my "PROPOSAL" commit.

mvollmer · 2024-01-19T08:54:30Z

pkg/storaged/btrfs/utils.jsx

+    if (name.length > 255)
+        return _("Name cannot be longer than 255 characters.");
+    if (name.includes('/'))
+        return cockpit.format(_("Name cannot contain the character '/'."));


What about subvolumes in directories? Should we allow those to be created in Cockpit?

Well, we don't offer listing directories so how would a user be able to create such?

By giving a name like "dir/dir/subvol" when creating a new one, maybe. Cockpit would do the necessary "mkdir -p".

But I agree that this might be confusing.

Yeah, we could but I honestly found this a bit confusing:

btrfs subvolume create -p foo/bar/ding
ID 264 gen 145 parent 256 top level 256 path root/root/foo/bar/ding

So foo/bar are just normal directories. Not super obvious if you ask me.

Oh, btrfs itself has a -p option, neat! But, yeah, no strong opinions here.

pkg/storaged/btrfs/subvolume.jsx

mvollmer · 2024-01-19T13:06:46Z

test/verify/check-storage-btrfs

+                    "Subvolume needs to be mounted")
+        b.click(self.dropdown_toggle(root_sel))
+
+        self.click_dropdown(self.card_row("Storage", name=label) + " + tr", "Mount")


There is self.click_card_dropdown which probably works here and is cleaner.

mvollmer

See the PROPOSAL commit.

Allow a user to create a new subvolume if the parent subvolume is mounted or any of it's parent subvolumes is mounted. We don't use btrfs's CreateSubvolume as it uses the first mount point from the block device, so with multiple subvol's mounted it is not possible to predict / influence on what mount point it will be created.

cockpituous · 2024-01-19T15:22:14Z

pkg/storaged/btrfs/subvolume.jsx

+    if (!mount_now || vals.at_boot == "never") {
+        mount_options.push("noauto");


These 2 added lines are not executed by any test. Details

cockpituous · 2024-01-19T15:22:14Z

pkg/storaged/btrfs/subvolume.jsx

+    if (vals.mount_options.ro)
+        mount_options.push("ro");
+    if (vals.at_boot == "never")
+        mount_options.push("x-cockpit-never-auto");


This added line is not executed by any test. Details

cockpituous · 2024-01-19T15:22:14Z

pkg/storaged/btrfs/subvolume.jsx

+    if (vals.at_boot == "never")
+        mount_options.push("x-cockpit-never-auto");
+    if (vals.at_boot == "nofail")
+        mount_options.push("nofail");


This added line is not executed by any test. Details

cockpituous · 2024-01-19T15:22:15Z

pkg/storaged/btrfs/subvolume.jsx

+    if (vals.at_boot == "nofail")
+        mount_options.push("nofail");
+    if (vals.at_boot == "netdev")
+        mount_options.push("_netdev");


This added line is not executed by any test. Details

cockpituous · 2024-01-19T15:22:15Z

pkg/storaged/btrfs/subvolume.jsx

+    if (vals.at_boot == "netdev")
+        mount_options.push("_netdev");
+
+    const name = (subvol.pathname == "/" ? vals.name : subvol.pathname + "/" + vals.name);


This added line is not executed by any test. Details

cockpituous · 2024-01-19T15:22:15Z

pkg/storaged/btrfs/subvolume.jsx

+    const name = (subvol.pathname == "/" ? vals.name : subvol.pathname + "/" + vals.name);
+    mount_options.push("subvol=" + name);
+    if (vals.mount_options.extra)
+        mount_options.push(vals.mount_options.extra);


This added line is not executed by any test. Details

cockpituous · 2024-01-19T15:22:15Z

pkg/storaged/btrfs/subvolume.jsx

+
+    let mount_point = vals.mount_point;
+    if (mount_point[0] != "/")
+        mount_point = "/" + mount_point;


This added line is not executed by any test. Details

cockpituous · 2024-01-19T15:22:15Z

pkg/storaged/btrfs/subvolume.jsx

+                      {
+                          dir: { t: 'ay', v: encode_filename(mount_point) },
+                          type: { t: 'ay', v: encode_filename("btrfs") },
+                          opts: { t: 'ay', v: encode_filename(mount_options.join(",") || "defaults") },


This added line is not executed by any test. Details

cockpituous · 2024-01-19T15:22:15Z

pkg/storaged/btrfs/subvolume.jsx

+                if (mount_now) {
+                    return client.mount_at(block, mount_point);
+                } else
+                    return Promise.resolve();


This added line is not executed by any test. Details

cockpituous · 2024-01-19T15:22:15Z

pkg/storaged/stratis/pool.jsx

-                          ]
-                      }),
+            mount_options(false, false),
+            at_boot_input(client.in_anaconda_mode() ? "local" : "nofail"),


This added line is not executed by any test. Details

mvollmer

Thanks!

jelly added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jan 16, 2024

github-advanced-security bot found potential problems Jan 16, 2024

View reviewed changes

pkg/storaged/stratis/pool.jsx Fixed Show fixed Hide fixed

jelly force-pushed the btrfs-subvolume-creation branch from 8de1263 to 8e2c223 Compare January 16, 2024 20:00

jelly added the release-note label Jan 16, 2024

jelly force-pushed the btrfs-subvolume-creation branch from 8e2c223 to 2dfecb5 Compare January 17, 2024 08:31

jelly removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jan 17, 2024

jelly force-pushed the btrfs-subvolume-creation branch from 2dfecb5 to 4979304 Compare January 17, 2024 11:56

jelly marked this pull request as ready for review January 17, 2024 12:18

jelly requested review from garrett and mvollmer January 17, 2024 12:58

mvollmer reviewed Jan 18, 2024

View reviewed changes

pkg/storaged/btrfs/subvolume.jsx Outdated Show resolved Hide resolved

mvollmer reviewed Jan 18, 2024

View reviewed changes

pkg/storaged/btrfs/subvolume.jsx Outdated Show resolved Hide resolved

mvollmer reviewed Jan 18, 2024

View reviewed changes

pkg/storaged/block/format-dialog.jsx Outdated Show resolved Hide resolved

jelly force-pushed the btrfs-subvolume-creation branch 2 times, most recently from d9be999 to 0fd0594 Compare January 18, 2024 15:41

storaged: make mounting dialog options re-usable

dec6d77

jelly force-pushed the btrfs-subvolume-creation branch from 0fd0594 to b2bc543 Compare January 18, 2024 15:44

jelly requested a review from mvollmer January 18, 2024 15:48

mvollmer requested changes Jan 19, 2024

View reviewed changes

pkg/storaged/btrfs/subvolume.jsx Outdated Show resolved Hide resolved

pkg/storaged/btrfs/subvolume.jsx Outdated Show resolved Hide resolved

jelly force-pushed the btrfs-subvolume-creation branch 2 times, most recently from 68fa567 to 8d2067d Compare January 19, 2024 10:49

jelly requested a review from mvollmer January 19, 2024 10:49

mvollmer reviewed Jan 19, 2024

View reviewed changes

mvollmer self-requested a review January 19, 2024 13:37

mvollmer requested changes Jan 19, 2024

View reviewed changes

jelly force-pushed the btrfs-subvolume-creation branch from 7e8c062 to 3021c69 Compare January 19, 2024 14:19

jelly requested a review from mvollmer January 19, 2024 14:19

cockpituous reviewed Jan 19, 2024

View reviewed changes

mvollmer approved these changes Jan 22, 2024

View reviewed changes

jelly merged commit ad5d2a5 into cockpit-project:main Jan 22, 2024
92 of 93 checks passed

jelly deleted the btrfs-subvolume-creation branch January 22, 2024 09:02

mvollmer removed the release-note label Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Btrfs subvolume creation #19849

Btrfs subvolume creation #19849

jelly commented Jan 16, 2024 •

edited

Loading

mvollmer Jan 19, 2024

jelly Jan 19, 2024

mvollmer Jan 19, 2024

mvollmer Jan 19, 2024

mvollmer Jan 19, 2024

jelly Jan 19, 2024

mvollmer Jan 19, 2024

jelly Jan 19, 2024

mvollmer Jan 19, 2024

mvollmer Jan 19, 2024 •

edited

Loading

mvollmer left a comment

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

cockpituous Jan 19, 2024

mvollmer left a comment

		if (!mount_now \|\| vals.at_boot == "never") {
		mount_options.push("noauto");

Btrfs subvolume creation #19849

Btrfs subvolume creation #19849

Conversation

jelly commented Jan 16, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvollmer Jan 19, 2024 • edited Loading

Choose a reason for hiding this comment

mvollmer 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

mvollmer left a comment

Choose a reason for hiding this comment

jelly commented Jan 16, 2024 •

edited

Loading

mvollmer Jan 19, 2024 •

edited

Loading