As is, this would add a .Glob to every RW lock acquisition. It would be interesting to try to optimize that.

let's just use a parent directory for the temp directories so we don't have to look at their name.

instead of: /foo/run/vfs-layers/temp-dir-28400 use something like /foo/run/vfs-layers/temp-dirs/28400 so we avoid the glob

(I’m wondering about even more optimization — we could have an “might have a staging directory” bool in layers.json, and use the layerStore.lockfile.RecordWrite notification mechanism to make all of this ~zero-cost to users of layerStore.startReading. But, one step at a time.)

Uh… do we need to worry about forward compatibility, i.e. an older version of this code running and unaware of tempDir?

I'm not sure, but I moved it to stagingDir. But I'm not sure how it is with recreateSymlinks(). I think it should ignore stagingDir.

I'm not sure, but I moved it to stagingDir.

That might make things worse, see it would interfere with pruneStagingDirectories.

But I'm not sure how it is with recreateSymlinks(). I think it should ignore stagingDir.

Yes. It probably works well enough now, because $stagingDir/link doesn’t exist, so the directory is ignored - but it seems attractive to clean this up, going forward.

The very last resort which always works would be to leave d.home for layer directories, and have the caller of the driver provide a separate d.metadataHome where we have no prior commitments.

But, maybe, the answer is that we can leave tempDir in d.home, as long as $tempDir/link is never created?? And should we now at least reserve a new path prefix, e.g. defining that ${d.home}/meta* is never a layer directory?

Or, alternatively, we could define a storage version number, and refuse to work with future versions. That would be drastic but clearly correct, OTOH we would be signing up for very precisely managing this to keep CRI-O and Podman in sync (and for not forgetting to update the version number…)

All of this forward-compatibility design seems valuable, but also somewhat out of scope of this “unlocked deletes” effort — or at least clearly out of scope of this prototype.

Cc: @giuseppe

Now that this is no longer just a prototype, we’ll need to find a solution.

@giuseppe With this, old versions’ GarbageCollect == podman system prune --external can delete all of tempDir.

I think that’s good enough, it’s supposed to be run at boot at quiet state, but I’m highlighting this in case I’m missing something.

#2325 (comment) still holds.

This structure is subject to a race condition when the .lock file is deleted before the other files in the directory.

So you have process A that holds the lock while it runs rm -rf temp-dir-XYZ, what a new process B should do when it attempts to lock the directory but doesn't find the lock file? 1: Assume the directory is being deleted (process A could crash), or 2: delete it as well?

I think it is safer to have:

temp-dir-XZY/data/
temp-dir-XZY/.lock

first temp-dir-XZY/data/ and if that succeeded proceed to remove temp-dir-XZY.

When process B tries to lock the directory, it will find the lock file until data/ exists, and if it doesn't find lock then it can do rmdir("temp-dir-XZY") because that could happen if the process A crashed after removing the lock file but before removing the directory (or after creating the directory but before the lock file was created)

I don’t think we need that directory at all — $root/${instanceID}.lock could mark ownership of $root/${instanceID}.data.

On creation, allocate the lock first. On deletion, unlock and delete the lock last.

(This could even, hypothetically, allow ….data to be a regular file, without the overhead of any extra directories.)

what if the content we are trying to move to the temp-dir has a file/directory named .lock?

This structure is subject to a race condition when the .lock file is deleted before the other files in the directory.

I think that part is good enough — things like os.RemoveAll really shouldn’t crash if a parent is removed in the meantime.

The problem with moving the lock inside the instance directory is at creation time, where we have the instance directory without any lock, so we need to protect against cleanup/recovery immediately deleting it. The current implementation does solve that, by holding the global lock — but that also means that the cleanup/recovery needs to obtain the global lock every time just to check whether anything needs to be cleaned up.

Together with #2325 (review) arguing that we should do the cleanups frequently, not just at process shutdown, that implies obtaining an extra lock file on both layerStore.startReading and layerStore.startWriting. I’d rather prefer to avoid that cost. (I’d even like to avoid the Readdir on every start…ing, but that will require more cooperation with layerStore.)

what if the content we are trying to move to the temp-dir has a file/directory named .lock?

I think that’s fine?

• File names under $root are owned by the tempdir package. Callers have no claim to decide names, we would simply not give them the API (and the current code in .Add that bases the file name on the base name of the input can be changed).
• So, a regular file would be named $root/${instanceID}.data. No ambiguity.
• If the instance were a larger directory structure (our primary use case now:, a layer deletion in progress), $root/${instanceID}.data would be a directory containing arbitrary contents. We would not be looking for locks inside the ….data subdirectory at all.

we don't need the lock here since it is released when the function exits, so for the caller it doesn't matter whether the code below runs with the lock or not.

As @mtrmac pointed out, we need to use os.ReadDir not Walk it recursively

d.dir can be filepath.Join(d.home, "dir", …) or filepath.Join(d.imageStore, d.String(), "dir", …) (or something with d.additionalHomes[], although those are presumably in R/O image stores and should not be possible to delete).

d.home and d.imageStore can be on different filesystems (and, I think, that would frequently would be the case: the “image” layers would be in durable storage and the “container” layers in something faster but less durable).

So, I think the GetTempDirRootDir driver API needs to be generalized to return a list of directories. (And that would remove the current “"" means N/A” special case.)

Similarly to VFS, this can point to d.imageStore, not to d.home.

Now that this is no longer just a prototype, we’ll need to find a solution.

… EVEN IF the function returns an error.

I think this homeDir is [undocumented :( and] “where to create layers”; it does not, AFAICS, consistently match the directory where id is found — so this does not return the right directory.

Really I think having dir2, in both graph drivers, return the parent(?) directory would be cleaner (centralizing knowledge/policy, and allowing to clearly track where data comes from) than hard-coding any knowledge in the DeferredRemove callers.

Admittedly, yes, that would imply documenting what dir2 does, and that’s a bit of a puzzle. But, as this PR shows, that would be valuable.

The rework and documentation of dir2 should be done in another PR. Should I create an issue for this?

I think that’s ultimately up to you.

I think after this merges you are likely to be pinged if this behaves in a surprising way, and right now you understand the code better than most people do most of the time. So, I think it would help your future self to document it very soon. But that’s a guess about the future, I don’t think I’m 100% justified in requiring you to do that investment now.