storage, overlay: stop using symlinks for new layers

[giuseppe]

The overlay driver maintained a l/ directory of symlinks solely to shorten mount option strings below the kernel's page-size limit.
This is unnecessary since mountOverlayFrom() already handles long mount options via fd-based mounting.

New layers now store relative diff paths (e.g. "layer-id/diff") directly in the lower file instead of short symlink references.

Old-format layers with l/ symlinks continue to work.

[giuseppe]

@mtrmac I believe you are going to like this PR :)

[mtrmac]

At a glance, yes I do :)

[Luap99]

I wonder, do we have a test case that mounts a 500 layer overlay? If not should we have one to ensure mounting always works?

[giuseppe]

Mounting with the new code gives always shorter paths since we use the fd number as the full path instead of a symlink that is still an absolute path. I think this code was left there just for historical reasons and there is no case where it is better

[kolyshkin]

Maybe it makes sense to add comments marking the remaining "legacy" code paths (dealing with l/ symlinks) as such, so they can be removed in the future. Otherwise it's hard to grok for future readers as it is -- we don't create any links but we handle them.

One other thing is, I'm not sure if we need the entire fork/exec open lower fds. This is a further optimization but maybe we can just do it in the same process? I mean, the fork/exec was done because of chdir, but it looks like we no longer need it (yet it's left there, and that's another part of "legacy" code I guess). So, maybe we should call mountOverlayFrom when working with legacy l/ symlinks, and do a via-fds in the same process?

As for the tests, I think this is covered by existing @test "allow storing images with more than 127 layers" which creates 300 layers (although I haven't checked it if actually crosses the pagesize threshold).

[kolyshkin]

BTW this removes the only user of the second value returned from dir2, so maybe it makes sense to drop it (perhaps in a separate subsequent commit).

[giuseppe]

thanks for spotting it, fixed now!

[kolyshkin]

I wonder, do we have a test case that mounts a 500 layer overlay? If not should we have one to ensure mounting always works?

We do have one for 300 layers, see storage/tests/layers.bats, although as I said I haven't checked we're actually crossing the pagesize threshold with it.

[TomSweeneyRedHat]

@giuseppe do you spin up a PR in Podman to see how this does there?
Other than @kolyshkin 's comments, LGTM, and happy green test buttons.

[giuseppe]

@giuseppe do you spin up a PR in Podman to see how this does there?
Other than @kolyshkin 's comments, LGTM, and happy green test buttons.

fixed, and opened a test PR here: containers/podman#28164

[TomSweeneyRedHat]

I restarted the failing test here, it looked to be a time out flake.
The Podman PR, however, is very red.

[giuseppe]

I restarted the failing test here, it looked to be a time out flake.
The Podman PR, however, is very red.

that failure is expected because we are not rebuilding Buildah. Buildah fails to find the symlink since we are not creating it anymore.

[giuseppe]

@Luap99 @mtrmac @kolyshkin PTAL

[Luap99]

that failure is expected because we are not rebuilding Buildah. Buildah fails to find the symlink since we are not creating it anymore.

Well how do you expect us to get CI passing then? We should not merge this if it breaks CI tests.
Also we cannot really control the update timings of podman/buildah/skopeo so if mixing them breaks them completely we have a big problem for the distributions shipping these tools. We can only control fedora/RHEL.

i.e. this test is using skopeo for example

[+0054s] not ok 39 [010] podman pull image with additional store in 1071ms
         # (in test file test/system/[010-images.bats, line 395](https://github.com/containers/podman/blob/2839aa038a596ef18fcf60a9731517fa8a1a6dc5/test/system/010-images.bats#L395))
         #   `skopeo copy containers-storage:$IMAGE \' failed
         #
<+     > # # podman  image exists quay.io/libpod/testimage:20241011
         # Getting image source signatures
         # Copying blob sha256:5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef
         # Copying blob sha256:b66a884aaf08f1c410c61682a7072d68a0d837ba8dc16ff13b9509bdceb32fd2
         # time="2026-02-27T11:30:24-06:00" level=fatal msg="reading blob sha256:5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef: creating file-getter: readlink /var/lib/containers/storage/overlay/b66a884aaf08f1c410c61682a7072d68a0d837ba8dc16ff13b9509bdceb32fd2/diff: invalid argument"
         # # [teardown]
         # rm: cannot remove '/tmp/CI_M4p5/podman_bats.ozboK6/imagestore/root/overlay': Device or resource busy

[giuseppe]

ok let's make it in two phases, I'll change this PR to not use the symlinks but still create them

[giuseppe]

Podman tests pass now

[Luap99]

LGTM

[kolyshkin]

Assuming the new functionality is covered by existing tests, LGTM

[mtrmac]

I think this is not immediately urgent if this PR will keep creating the link files, but per #703 (and code linked from there), the l subdirectory might be the only thing preventing us from deleting the driver’s whole directory, even on success paths if we currently have no layers (and that’s important at least for ScanPriorDrivers).

That would need a re-think/re-design before we remove the linkDir code, and probably a warning now so that we don’t forget.

[giuseppe]

I think this is not immediately urgent if this PR will keep creating the link files, but per #703 (and code linked from there), the l subdirectory might be the only thing preventing us from deleting the driver’s whole directory, even on success paths if we currently have no layers (and that’s important at least for ScanPriorDrivers).

That would need a re-think/re-design before we remove the linkDir code, and probably a warning now so that we don’t forget.

Once we decide to drop completely the l subdirectory, should we just temporarily add some code in Init to forcefully rm -rf l? Would that be enough?

[mtrmac]

The problem is the other way: if there is no l, and the user deleted all layers, we have code to delete the whole overlay subdirectory, and that will now succeed. That breaks our designed workflow for setting a quota on container storage.

[mtrmac]

… and the way we detect which graph driver is “currently in use”.

[giuseppe]

The problem is the other way: if there is no l, and the user deleted all layers, we have code to delete the whole overlay subdirectory, and that will now succeed. That breaks our designed workflow for setting a quota on container storage.

I see. Could we use staging and tempdirs? We would need to make sure these are never deleted (at least one)

[mtrmac]

That all works but those are untransparent implementation hacks. What are we doing here? If the directory presence matters it should be consciously managed as specific goal. Or move the information elsewhere, into a real metadata file, perhaps layers.json.

[giuseppe]

sure, we can handle that with an explicit file to prevent rmdir or find a better way to mark the graph as used,. Having that behavior today because of l is also a side effect today.

If there are no layers/containers, is the storage driver really used? Triggering again the heuristic to pick a driver is really the wrong thing to do? Upgrading from vfs to overlay (when possible), looks like a feature