Add new version 1 format that uses the new xwhiteout marking approach

[alexlarsson]

Linux upstream is making some change to how xwhiteout directories are marked. They are now marked with an opaque xattr with content "x".

See:
https://lore.kernel.org/linux-unionfs/CAOQ4uxh5x_-1j8HViCutVkghA1Uh-va+kJshCuvB+ep7WjmOFg@mail.gmail.com/T/#t

This creates a new image format version, 1, which adds these xattrs (and the old ones for backwards compat with linux 6.7). It also adds support to the CLI to specify the version to use for the image.

Note: This changes the output of the CLI tools to use the new version by default, and you need to pass --verison-format=0 to get the old version. This is ok at this point, as most images are unaffected by the version due to lack of whiteouts, and not many people use composefs yet.

We probably also want to change ostree to specify this new version as well.

Also, we should maybe wait a bit and ensure this lands as-is upstream before merging this.

[alexlarsson]

Alternatively we can not bump the version and "break" the format for existing images that contain whiteouts. I'm not sure how many real users with such images exists atm.

[cgwalters]

Overall LGTM, but I think the notable downside here is that we're creating the new format by default, and that will just break old consumers.

Code running mkcomposefs that wants to default to 0 will have to do something ugly like parse mkcomposefs --help to check if --format-version is there, and if so and it wants compat set it to zero.

It'd be really nice I guess to only turn on this version 1 if we detect it's needed - I don't know how hard that'd be, but it seems like it wouldn't be.

We could change the semantics then of --format-version to automatically downgrade to the oldest version that one needs? Or maybe we add --format-version-min and --format-version-max.

[cgwalters]

There's going to be an interesting question going forward of how quickly we bump the default version. Callers should probably pin it...but they couldn't do that before this change because we didn't expose the option.

[alexlarsson]

It was always exposed in the library, but yeah.

[alexlarsson]

Yeah, it's tricky how to best handle this.

I changed the default now because it really is pretty early in the history of composefs, and it will only affect a very small amount of images. Images that don't have any whiteouts will in fact produce identical images for v0 and v1, so we're already sort of automatically picking the version.

But yeah, I don't think we always want to change the default version whenever there is a new one.
Maybe we should have a VERSION_MAX and a VERSION_DEFAULT, and only very very rarely change the default. I think it is fine now, given the low amount of users, but basically almost never in the long term future. In particular, we should probably never change the default for a change that affects many images.

[cgwalters]

Images that don't have any whiteouts will in fact produce identical images for v0 and v1, so we're already sort of automatically picking the version.

Right, this seems like it agrees with my proposal that we could auto-detect.

Anyways overall...the thing is we did release a version 1.0 that says it is "stable". In FOSS it's so hard to know how many people are really using one's software.

---

Hmm wait...a random question here, are we not embedding the cfs version in the metadata? Why does composefs-dump take a --format-version?

[alexlarsson]

Ah, yeah, we do have a version in the erofs file, although I see its initialized currently with 1, not 0 so we'll have to be a bit careful about conversions if we use it like this.

However, I'm not sure this approach is quite right. Suppose we wanted to say switch ostree (since its still experimental) to basically default to version 1 instead of 0. If we only make the change for images with whiteouts that is probably fine, but then we can't specify version == 1 and also embedd the version in the image, because then all previous non-whiteout commits would change digest. To solve this we could then use UNSPECIFIED instead, but that will mean that whenever there is a version 2 we would pick up that.

I think we need the max-version/min-version approach instead. Then ostree can say version=1 min_version=0, which would pick version 1 (and record that in the image) only when needed, and otherwise pick version 0.

[cgwalters]

To solve this we could then use UNSPECIFIED instead, but that will mean that whenever there is a version 2 we would pick up that.

My current strawman patch above bumps explicitly to 1, not max. That said in theory to handle a future version 2 (that might also have a similar "use only if needed" semantic), we should probably make "unspecified" a persistent state, and then only increment the version as we see, instead of having it be a -1 field.

I think we need the max-version/min-version approach instead. Then ostree can say version=1 min_version=0, which would pick version 1 (and record that in the image) only when needed, and otherwise pick version 0.

max-version is clearly useful for a case where we want to just error out if an image would be too new. A little uncertain as to the value of min-version, but we could clearly add it later if demanded.

I think what I'm proposing here of supporting "unspecified" i.e. "use the oldest you can" is useful and nicer than a max-version in the immediate term, because it means existing callers will Just Work.

[cgwalters]

Ah, yeah, we do have a version in the erofs file, although I see its initialized currently with 1, not 0 so we'll have to be a bit careful about conversions if we use it like this.

(Should we break out a distinct issue for this? I don't understand the intended relationship of the erofs version and this format-version)

[cgwalters]

Updated proposal:

index d24984c..92cb65f 100644
--- a/libcomposefs/lcfs-writer-erofs.c
+++ b/libcomposefs/lcfs-writer-erofs.c
@@ -1149,6 +1149,11 @@ static int add_overlayfs_xattrs(struct lcfs_ctx_s *ctx, struct lcfs_node_s *node
 		if (ret < 0)
 			return ret;
 
+		// We detected an embedded whiteout, so switch to v1 if we're in auto mode
+		if (ctx->options->unspecified_version && ctx->options->version < 1) {
+			ctx->options->version = 1;
+		}
+
 		/* Mark dir containing whiteouts with new format as of version 1 */
 		if (ctx->options->version >= 1) {
 			ret = lcfs_node_set_xattr(
diff --git a/libcomposefs/lcfs-writer.h b/libcomposefs/lcfs-writer.h
index 0e58571..414066d 100644
--- a/libcomposefs/lcfs-writer.h
+++ b/libcomposefs/lcfs-writer.h
@@ -48,7 +48,9 @@ enum lcfs_flags_t {
 	LCFS_FLAGS_MASK = 0,
 };
 
+#define LCFS_VERSION_DEFAULT 0
 #define LCFS_VERSION_MAX 1
+
 /* Version history:
  * 0 - Initial version
  * 1 - Mark xwhitouts using the new opaque=x format as needed by Linux 6.8
@@ -60,6 +62,9 @@ typedef ssize_t (*lcfs_write_cb)(void *file, void *buf, size_t count);
 struct lcfs_write_options_s {
 	uint32_t format;
 	uint32_t version;
+	// If true, then version acts as a minimum, but it will be automatically
+	// incremented 
+	bool unspecified_version;
 	uint32_t flags;
 	uint8_t *digest_out;
 	void *file;
diff --git a/tools/composefs-dump.c b/tools/composefs-dump.c
index a14309a..eb36939 100644
--- a/tools/composefs-dump.c
+++ b/tools/composefs-dump.c
@@ -48,7 +48,7 @@ int main(int argc, char **argv)
 	const char *src_path = NULL;
 	const char *dst_path = NULL;
 	struct lcfs_write_options_s options = { 0 };
-	int format_version = LCFS_VERSION_MAX;
+	int format_version = LCFS_VERSION_UNSPECIFIED;
 
 	if (argc <= 1) {
 		fprintf(stderr, "No source path specified\n");
diff --git a/tools/mkcomposefs.c b/tools/mkcomposefs.c
index 6a999d1..b54eb95 100644
--- a/tools/mkcomposefs.c
+++ b/tools/mkcomposefs.c
@@ -326,8 +326,8 @@ static void usage(const char *argv0)
 		"  --print-digest        Print the digest of the image\n"
 		"  --print-digest-only   Print the digest of the image, don't write image\n"
 		"  --from-file           The source is a dump file, not a directory\n"
-		"  --format-version=N    Use this format version (default=%d)\n",
-		bin, LCFS_VERSION_MAX);
+		"  --format-version=N    Use this format version (default=lowest version compatible with input)\n",
+		bin);
 }
 
 #define OPT_SKIP_XATTRS 102
@@ -903,7 +903,8 @@ int main(int argc, char **argv)
 	int opt;
 	FILE *out_file;
 	char *failed_path;
-	long format_version = LCFS_VERSION_MAX;
+	bool version_was_specified = false;
+	long format_version = LCFS_VERSION_DEFAULT;
 	char *end;
 
 	/* We always compute the digest and reference by digest */
@@ -941,6 +942,7 @@ int main(int argc, char **argv)
 				fprintf(stderr, "Invalid format version %s\n", optarg);
 				exit(EXIT_FAILURE);
 			}
+			version_was_specified = true;
 			break;
 		case ':':
 			fprintf(stderr, "option needs a value\n");
@@ -1035,7 +1037,13 @@ int main(int argc, char **argv)
 		options.digest_out = digest;
 
 	options.format = LCFS_FORMAT_EROFS;
-	options.version = (int)format_version;
+	if (!version_was_specified) {
+		options.unspecified_version = true;
+		// In theory we can add a --min-version and --max-version in the future
+		options.version = LCFS_VERSION_DEFAULT;
+	} else {
+		options.version = (uint32_t)format_version;
+	}
 
 	if (lcfs_write_to(root, &options) < 0)
 		err(EXIT_FAILURE, "cannot write file");

[alexlarsson]

@cgwalters Take a look at my updated commits.

[alexlarsson]

To solve this we could then use UNSPECIFIED instead, but that will mean that whenever there is a version 2 we would pick up that.

My current strawman patch above bumps explicitly to 1, not max. That said in theory to handle a future version 2 (that might also have a similar "use only if needed" semantic), we should probably make "unspecified" a persistent state, and then only increment the version as we see, instead of having it be a -1 field.

I think we need the max-version/min-version approach instead. Then ostree can say version=1 min_version=0, which would pick version 1 (and record that in the image) only when needed, and otherwise pick version 0.

max-version is clearly useful for a case where we want to just error out if an image would be too new. A little uncertain as to the value of min-version, but we could clearly add it later if demanded.

Not necessarily error out. max version is useful if a new version allows a new feature but we don't want to use that particular one, as that would invalidate the digest of previous images. I.e. if you set max_version to 0, you just don't get the new whiteout fix, but otoh you don't generate files with a different digest for those.

I think what I'm proposing here of supporting "unspecified" i.e. "use the oldest you can" is useful and nicer than a max-version in the immediate term, because it means existing callers will Just Work.

This is basically what I did in my patch. "version" is the oldest version to generate, but max_version is the maximum version it can gets bumped to if that would apply to the image. It never unnecessarily bumps above "version".

[cgwalters]

Read the code and this LGTM.

[alexlarsson]

Also, this change now landed in upstream, so this will work with the latest kernel.