✅ A new PR has been created in buildah to vendor these changes: podman-container-tools/buildah#6414

Podman PR podman-container-tools/podman#27251 and the buildah test PR podman-container-tools/buildah#6414 from the bot both look good so that means we can remove the special case from ApplyDiff() in overlay I think, ref podman-container-tools/podman#25862 (comment)

I still need to work on the actual feature here though to extract while the store in unlocked.

@mtrmac FYI I have not really addressed most of your comments yet, I am just trying to push things to see how much things break. Still seeing plenty of test failures.

Issue 1 I see is that I just use the 700 permission from the tmpdir due the rename instead of the proper diff dir creation permissions that are in the driver.create() code

	diff := path.Join(dir, "diff")
	if err := idtools.MkdirAs(diff, forcedSt.Mode, forcedSt.IDs.UID, forcedSt.IDs.GID); err != nil {
		return err
	}

	if d.options.forceMask != nil {
		st.Mode |= os.ModeDir
		if err := idtools.SetContainersOverrideXattr(diff, st); err != nil {
			return err
		}
	}

Not sure if I should expose that into the tmpdir creation logic, I guess that makes the most sense since only the dirver should now the exact permission that should be used?

Second problem I see are timeouts (in parallel running tests) which I guess mean I added a deadlock situation?
https://api.cirrus-ci.com/v1/artifact/task/5906611702595584/html/sys-podman-debian-13-rootless-host-sqlite.log.html

I guess looking at the code this unlock/lock again thing I did is indeed completely broken and unsafe due ABBA deadlock, i.e. in putlayer we also hold the containerStore lock so only unlocking the layer store makes it possible that another process can get the layer lock and then blocks on the still gold container store thus both process handing forever.
I haven't checked all the code paths but I guess with the locking order requirements what I did is basically impossible to achieve anyway and I have to indeed move this out to before we get the lock?

Issue 1 I see is that I just use the 700 permission from the tmpdir due the rename instead of the proper diff dir creation permissions that are in the driver.create() code

Not sure if I should expose that into the tmpdir creation logic, I guess that makes the most sense since only the dirver should now the exact permission that should be used?

I think that could work.

I was thinking StageAddition does not actually need to create (os.Create/os.Mkdir) the tmpAddPath at all. All of that happens inside a lock-protected td.tempDirPath, so there is ~nothing special, that I can see, about populating tmpAddPath — the provided callback can create the staged item without any help. (That could also mean StageDirectoryAddition and StageFileAddition could be consolidated into one. And I’m not immediately sure we need a callback — StageAddition could return a newStagingPathToPopulate — but I also didn’t now carefully re-read the tempdir package.)

I haven't checked all the code paths but I guess with the locking order requirements what I did is basically impossible to achieve anyway and I have to indeed move this out to before we get the lock?

Per the locking hierarchy documented at the top of store, I think you’re right here.

And I’m not immediately sure we need a callback — StageAddition could return a newStagingPathToPopulate — but I also didn’t now carefully re-read the tempdir package.)

Yeah my thinking was that the callback provides a "lifetime" of when the path is safe to use, if I return a string/struct with the path then the caller can cleanup/commit and then still use the path afterwards. This is really where I start to hate go because in rust this would be trivial to enforce so that there could only ever be one call to commit and then render the object useless afterwards.

But yes usage wise this callback is indeed getting quite ugly to the point where just returning the path is much simpler and well how go works in general. I do like the suggestion of just returning the path to consolidate both tmpdir functions into one so I will go with that.

@mtrmac I will push this into podman and run more tests tomorrow but I think like this it should be workable now. I fix the minor lint issues here of course on the next push. Let me know if this approach seem right to you, I guess the code could need some more better comments/function names likely.

Ok last issue noticed in podman the idmapping logic cannot be implemented unlocked I fear.

We have this code in putLayer()

	if options.HostUIDMapping {
		options.UIDMap = nil
	}
	if options.HostGIDMapping {
		options.GIDMap = nil
	}
	uidMap := options.UIDMap
	gidMap := options.GIDMap
	if parent != "" {
		var ilayer *Layer
		for _, l := range append([]roLayerStore{rlstore}, rlstores...) {
			lstore := l
			if lstore != rlstore {
				if err := lstore.startReading(); err != nil {
					return nil, -1, err
				}
				defer lstore.stopReading()
			}
			if l, err := lstore.Get(parent); err == nil && l != nil {
				ilayer = l
				parent = ilayer.ID
				break
			}
		}
		if ilayer == nil {
			return nil, -1, ErrLayerUnknown
		}
		parentLayer = ilayer

		if err := s.containerStore.startWriting(); err != nil {
			return nil, -1, err
		}
		defer s.containerStore.stopWriting()
		containers, err := s.containerStore.Containers()
		if err != nil {
			return nil, -1, err
		}
		for _, container := range containers {
			if container.LayerID == parent {
				return nil, -1, ErrParentIsContainer
			}
		}
		if !options.HostUIDMapping && len(options.UIDMap) == 0 {
			uidMap = ilayer.UIDMap
		}
		if !options.HostGIDMapping && len(options.GIDMap) == 0 {
			gidMap = ilayer.GIDMap
		}
	} else {
		// FIXME? It’s unclear why we are holding containerStore locked here at all
		// (and because we are not modifying it, why it is a write lock, not a read lock).
		if err := s.containerStore.startWriting(); err != nil {
			return nil, -1, err
		}
		defer s.containerStore.stopWriting()

		if !options.HostUIDMapping && len(options.UIDMap) == 0 {
			uidMap = s.uidMap
		}
		if !options.HostGIDMapping && len(options.GIDMap) == 0 {
			gidMap = s.gidMap
		}
	}
	if s.canUseShifting(uidMap, gidMap) {
		options.IDMappingOptions = types.IDMappingOptions{HostUIDMapping: true, HostGIDMapping: true, UIDMap: nil, GIDMap: nil}
	} else {
		options.IDMappingOptions = types.IDMappingOptions{
			HostUIDMapping: options.HostUIDMapping,
			HostGIDMapping: options.HostGIDMapping,
			UIDMap:         copySlicePreferringNil(uidMap),
			GIDMap:         copySlicePreferringNil(gidMap),
		}
	}

However we extract the layer with the caller specified layerOptions.UIDMap, layerOptions.GIDMap in stageWithUnlockedStore() before this code can get run.

I guess the s.uidMap case could be done without a lock but not the parent lookups? So I guess the simple solution would be to only use the unlocked extract path when options.HostGIDMapping is true.

(I didn’t look at the current code in this PR yet.) AFAICS layer.[UG]IDMap never changes once the layer is created, so it should be safe to look up the parent, read these values, and then unlock.

IIRC the plan was to start layer creation with an ID lookup (so that we don’t start expensively staging it if it already exists), so the parent lookup could be done within the same lock scope.

#!/usr/bin/env bpftrace

kfunc:fcntl_setlk
{
  $lockname = str(args->filp->f_path.dentry->d_name.name);

  if ($lockname == "storage.lock" || $lockname == "layers.lock" ||
      $lockname == "images.lock" || $lockname == "containers.lock" ) {

    @blockedTime[tid, $lockname] = nsecs;
  }
}

kretfunc:fcntl_setlk
{
  $lockname = str(args->filp->f_path.dentry->d_name.name);

  if ($lockname == "storage.lock" || $lockname == "layers.lock" ||
      $lockname == "images.lock" || $lockname == "containers.lock" ) {

    // lock duration in msec
    $lock_duration = (nsecs - @blockedTime[tid, $lockname])/1000000;
    if ($lock_duration) {
      printf("blocked %s time: %lld msec\n", $lockname, $lock_duration);
    }
    delete(@blockedTime[tid, $lockname]);
    @lockholdTime[pid, args->fd] = (nsecs, $lockname);
  }
}

tracepoint:syscalls:sys_enter_close
{
  $a = @lockholdTime[pid, args->fd];
  if ($a.0) {
    // lock duration in msec
    $lock_duration = (nsecs - $a.0)/1000000;
    if ($lock_duration) {
      printf("lock %s time: %lld msec\n", $a.1, $lock_duration);
    }
    delete(@lockholdTime[pid, args->fd]);
  }
}

FYI this is the script I used to measure lock times during my demo last week, not sure if there is a decent place to store this. I guess it could be helpful in the future to find more lock contention, should I add it under hack/ maybe?

@mtrmac I addressed all your comments now. Podman is also happy in podman-container-tools/podman#27251 (modulo some unrelated flakes)

#!/usr/bin/env bpftrace

kfunc:fcntl_setlk
{
  $lockname = str(args->filp->f_path.dentry->d_name.name);

  if ($lockname == "storage.lock" || $lockname == "layers.lock" ||
      $lockname == "images.lock" || $lockname == "containers.lock" ) {

    @blockedTime[tid, $lockname] = nsecs;
  }
}

kretfunc:fcntl_setlk
{
  $lockname = str(args->filp->f_path.dentry->d_name.name);

  if ($lockname == "storage.lock" || $lockname == "layers.lock" ||
      $lockname == "images.lock" || $lockname == "containers.lock" ) {

    // lock duration in msec
    $lock_duration = (nsecs - @blockedTime[tid, $lockname])/1000000;
    if ($lock_duration) {
      printf("blocked %s time: %lld msec\n", $lockname, $lock_duration);
    }

    // store max and create histogram that get printed on bpftrace exit
    @block_max[$lockname] = max($lock_duration);
    @block_duration[$lockname] = hist($lock_duration);

    delete(@blockedTime[tid, $lockname]);
    @lockholdTime[pid, args->fd] = (nsecs, $lockname);
  }
}

tracepoint:syscalls:sys_enter_close
{
  $a = @lockholdTime[pid, args->fd];
  if ($a.0) {
    // lock duration in msec
    $lock_duration = (nsecs - $a.0)/1000000;
    if ($lock_duration) {
      printf("lock %s time: %lld msec\n", $a.1, $lock_duration);
    }

    // store max and create histogram that get printed on bpftrace exit
    @lock_max[$a.1] = max($lock_duration);
    @lock_duration[$a.1] = hist($lock_duration);

    delete(@lockholdTime[pid, args->fd]);
  }
}

Updated test script which prints some nice histograms and the max time values on exit for each lock file. It could be useful to measure other code parts as well. Let me know if you like me to commit this.

Maybe into storage/contrib? It’s valuable right now, it could be useful as a starting point for a similar analysis of something else in the future, but I don’t think we’d want to commit to maintaining this script. I don’t have a strong preference.