Rename atomicWrite to writeToCompletion

[wzshiming]

The function does not guarantee atomicity, because the same temporary file will be opened multiple times.

containerd/content/local/store.go

Lines 689 to 701 in 75a0c2b

	func atomicWrite(path string, data []byte, mode os.FileMode) error {
	tmp := fmt.Sprintf("%s.tmp", path)
	f, err := os.OpenFile(tmp, os.O_RDWR|os.O_CREATE|os.O_TRUNC|os.O_SYNC, mode)
	if err != nil {
	return errors.Wrap(err, "create tmp file")
	}
	_, err = f.Write(data)
	f.Close()
	if err != nil {
	return errors.Wrap(err, "write atomic data")
	}
	return os.Rename(tmp, path)
	}

Should be changed to write a random temporary file Instead of writing a fixed temporary file.
Like this https://github.com/containerd/continuity/blob/93e15499afd51cdc830773dc804c89c11673fc63/ioutils.go#L34-L63
Signed-off-by: Shiming Zhang wzshiming@foxmail.com

[k8s-ci-robot]

Hi @wzshiming. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[fuweid]

Sorry I didn't get your point. could you mind to explain more? thanks

[wzshiming]

Sorry I didn't get your point. could you mind to explain more? thanks

@fuweid
Should be changed to write a random temporary file Instead of writing a fixed temporary file.
Like this https://github.com/containerd/continuity/blob/93e15499afd51cdc830773dc804c89c11673fc63/ioutils.go#L34-L63

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 6m 15s (non-voting)

[fuweid]

Should be changed to write a random temporary file Instead of writing a fixed temporary file.

The path will be locked by content store. In content store, I think it is safe to use %s.tmp in here.

Anyway, I am ok to align with continuity package. :) And why not just use the existing function?

[wzshiming]

If it does not concurrent writing of %s.tmp, then there is actually no need to atomic write, right?
I found that it is to fix some failures in Windows CI #3324
However, this modification cannot guarantee Linux atomic writing, nor does it guarantee windows atomic writing.
It's just a temporary solution that can pass CI.
I think it is necessary to implement the atomic write of Windows and then use it here

[fuweid]

The atomic write is to make sure that the content is completeness. Sometimes, the disk has pressure and the content will be truncated somehow. The atomic write is like transaction, not just for concurrent.

[wzshiming]

Thank you, I understand, i found the original PR #2580.
it was forcibly interrupted when the file was just opened and no data was written. error caused by empty data when reading.
Although I think we should deal with empty data in this case, instead of guaranteeing to write data here, but thank you.

[fuweid]

The atomicWrite is used to ensure the content should be completeness, not empty or part of it. :)

[mikebrow]

The function does not guarantee atomicity, because the same temporary file will be opened multiple times

Any two attempts to write to the same path (for example: async close operations with updated timestamps for the close operation) should already be atomic using sem locks. As Fu Wei says the "atomic" part of this call is to ensure if the renamed file (path) exists it has the contents (timestamp). I don't think it would hurt to use a unique random file name extension here, but I also don't believe it is necessary, given the callers are in charge of making sure the write to path is atomic. Maybe we should rename the call to writeToCompletion()

[dmcgowan]

Renaming is fine is to prevent unintended use of the function. I don't think this change is necessary and if it is, then there is a larger problem to solve with the ref lock. If the locks aren't working and there is a race at the filesystem level, the behavior should already be considered undefined.

If there was a "bad resume" detected as indicated by the above Info level log message, then this change would necessitate cleaning up any of the relevant tmp files since the new attempt to recreate would no longer handle it (now it just truncs and retries). We should only rely on cleanup (via Abort or Commit) to handle removal of known files or anything unexpectedly placed in the directory, our goal is always to keep the filesystem as clean as possible at all times.

[wzshiming]

@fuweid @mikebrow @dmcgowan
Thanks, i got it. Updated this PR to rename atomicWrite to writeToCompletion.

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 5m 51s (non-voting)

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 5m 44s (non-voting)

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 5m 50s (non-voting)

[fuweid]

please rebase, thanks~

[theopenlab-ci]

Build succeeded.

• containerd-build-arm64 : SUCCESS in 8m 26s (non-voting)
• containerd-test-arm64 : SUCCESS in 5m 40s (non-voting)
• containerd-integration-test-arm64 : FAILURE in 20m 36s (non-voting)