[vagokuln-msft]

- What I did

Preserve hard links when committing a Windows container.

writeTarFromLayer wrote a full copy for every name of a file, so files sharing a file ID in a layer became independent copies in the committed image. The link relationship was lost and the payload was stored once per name.

- How I did it

The layer reader already exposes LinkInfo, which returns the link count and file ID of the current file, so no new API was needed.

For a file reporting more than one link, the file ID of the first name seen is recorded. Any later name with the same file ID is written as a tar.TypeLink header pointing at that first name instead of streaming the contents again.

The import side already handles tar.TypeLink through legacyLayerWriter.AddLink, so no change was required there.

- How to verify it

Unit tests are included in daemon/graphdriver/windows/windows_test.go. They drive writeTarFromLayer through a scripted LayerReader, so the behaviour can be checked without a container or a particular Windows build:

go test ./daemon/graphdriver/windows/

They assert that a later name for a file ID becomes a tar.TypeLink entry carrying no payload, that later names point at the first name rather than chaining to the previous one, and that distinct file IDs and files with a single link are never linked. Reverting the change in windows.go makes them fail.

Please also read this part, because there is a dependency for the end to end behaviour.

NewLayerReader obtains its link count from the layer export produced by the Windows container storage layer. On current Windows builds that export writes an independent copy for every name, so LinkInfo always reports a link count of one, the new branch never runs, and this change is inert with no behaviour difference. A corresponding fix to the Windows container storage export is being made separately, and only with both changes present is the link preserved end to end.

Verified on a Windows Server build carrying that matching OS change:

1. Run a container and create a file with two additional hard linked names:

docker run --name src mcr.microsoft.com/windows/servercore:ltsc2025 cmd /c ^
  "mkdir C:\hltest && echo hello > C:\hltest\original.dat && ^
   mklink /H C:\hltest\link1.dat C:\hltest\original.dat && ^
   mklink /H C:\hltest\link2.dat C:\hltest\original.dat"

2. docker commit src img:latest

3. Inspect the resulting image layer on the host with fsutil hardlink list against Files\hltest\original.dat.

Before this change the layer reports one link, meaning three independent copies. With this change it reports three names sharing one file ID.

A docker save, docker image rm, docker load round trip was also run and the reloaded layer still reports three links with intact file contents.

Note that hard links in a read only layer are not observable from inside a running container, because the container filter presents layer files as separate per name objects. This is existing behaviour and is also true of hard links that ship in base images. Verification should be done against the image layer on the host.

- Size impact

Measured on the same source container, with a 200 MB payload and two additional hard linked names, so 600 MB of logical content:

	Committed layer on disk
Before	608.2 MB
After	208.2 MB

The saving scales with the number of names sharing the file ID.

- Description for the changelog

Windows: Preserve hard links when committing a container, so a file with multiple names is stored once in the resulting image layer instead of once per name.

[thaJeztah]

Thanks! Overall looks good, but left some comments

[thaJeztah]

Nit; can scope these vars to the if;

if nlinks, id, err := r.LinkInfo(); err == nil && nlinks > 1 {

[thaJeztah]

This one looks to be only used if it's not yet present, so could inline this;

linkRecords[id.FileID] = filepath.ToSlash(name)

[thaJeztah]

If you're updating; perhaps also use the canonical (0o644 ) format here, to prevent gofumpt from updating it later 😅

hdr.Mode = 0o644

[thaJeztah]

In case useful; here's what I had locally; feel free to copy (and amend your commit); I also added a short comment to explain what the intent is;

			// For files with multiple hardlinks, write the first occurrence normally
			// and record its path. Subsequent occurrences are emitted as hardlinks to it.
			if nlinks, id, err := r.LinkInfo(); err == nil && nlinks > 1 {
				if prev, ok := linkRecords[id.FileID]; ok {
					hdr := backuptar.BasicInfoHeader(name, 0, fileInfo)
					hdr.Mode = 0o644
					hdr.Typeflag = tar.TypeLink
					hdr.Linkname = prev
					if err := t.WriteHeader(hdr); err != nil {
						return err
					}
					continue
				}
				linkRecords[id.FileID] = filepath.ToSlash(name)
			}

[thaJeztah]

Unrelated to this PR; I can open a follow-up PR for that, or if you want, you can add a second commit for it; looks like there's a potential resource leak here; the writer isn't closed on early returns, so we should probably add a defer (and a named output variable);

func writeTarFromLayer(r hcsshim.LayerReader, w io.Writer) (retErr error) {
	linkRecords := make(map[[16]byte]string)
	t := tar.NewWriter(w)
	defer func() {
		if err := t.Close(); err != nil && retErr == nil {
			retErr = err
		}
	}()

Then remove the return t.Close() at the end of the function (just return nil)

[vvoland]

Thanks!

2 suggestions from my side:

1. The verification you mentioned could be a nice integration test (probably also for non-Windows too?)
2. Can be squashed to one commit, no need to separate fix from the tests

[vagokuln-msft]

Thanks both, updated.

@thaJeztah I took your suggested block as is, so the vars are now scoped to the if, filepath.ToSlash(name) is inlined, the mode uses 0o644, and your comment explaining the intent is included. I also added the tar writer fix as a separate commit since it is unrelated to hard links, credited to you in the commit message. Happy to drop it if you would rather land that as your own follow-up.

@vvoland squashed the fix and the tests into one commit.

On the integration test, I looked at this and I do not think one would be meaningful today, for two reasons.

For Windows, the link count comes from the layer export performed by the Windows container storage layer. On every currently shipping Windows build that export writes an independent copy per name, so LinkInfo always reports a link count of one and this code path never runs. An integration test would therefore either assert the old behaviour or be permanently skipped until the matching OS change ships. That is exactly why I went with the unit test, it drives writeTarFromLayer through a scripted LayerReader so the logic is verifiable on a current build. Reverting the change makes the test fail, so it is not vacuous.

For non-Windows, I do not think there is anything to test. This is specific to the Windows graphdriver writing its own tar. The Linux path goes through go-archive, which already preserves hard links using its SeenFiles map and emits tar.TypeLink (archive.go, around lines 312 and 372 to 384).

Once the OS side lands I am happy to follow up with an integration test if you still want one.

[thaJeztah]

Code changes look good to me, but can you remove this snipped from the second commit message? We try to avoid referencing GitHub PR / ticket numbers from commit messages, as they tend to produce much noise on Github;

This is a pre-existing issue, unrelated to hard link handling. Suggested
by thaJeztah during review of https://github.com/moby/moby/pull/53624.

[vagokuln-msft]

Done, removed the reference. The second commit message now just notes that it is a pre-existing issue unrelated to the hard link handling in the preceding commit. Only the message changed, the tree is identical.

[thaJeztah]

LGTM, thx!