lib/commonio: make lock failures more detailed #562
Conversation
This tweaks the database locking logic so that failures in the link-checking paths are more detailed. The rationale for this is that I've experienced a non-deterministic bug which seems to be coming from this logic, and I'd like to get more details about the actual failing condition.
| (void) fprintf (shadow_logfd, | ||
| "%s: %s file stat error: %s\n", | ||
| shadow_progname, file, strerror (errno)); | ||
| } | ||
| return 0; | ||
| } | ||
|
|
||
| if (sb.st_nlink != 2) { |
There was a problem hiding this comment.
I haven't yet managed to reproduce the failure in a controlled environment, but I'm suspicious of this st_nlink check.
In particular, I think it is theoretically possible on a multi-processor system to race the inode update and observe a stale link-count of 1 here after the link() call succeeded.
I've seen other comments hinting at similar caching effects, e.g. https://github.com/miquels/liblockfile/blob/v1.17/lockfile.c#L319-L320.
What is this check actually meant to protect against? Would it be possible to safely drop it? Or maybe perform a fsync() on the file before this?
There was a problem hiding this comment.
If two processes are calling commonio_lock() at the same time, the first will link %s to %s.$pid , causing %s to have link count2; then the second will link %s to %s.pid2, causing %s to have link count 3. (either $pid or $pid2 could end up finding the link count 3, or both).
I would be very surprised if the kernel's vs code was keeping the link count high, causing a race, but your results are interesting. One suspect would be overlay, if you're using that.
There was a problem hiding this comment.
Hmm, but here the file-linking goes in the other direction, i.e. link(dbname.pid, dbname.lock) will create dbname.lock as a new name for the existing dbname.pid.
At that point a second call like link(dbname.pid2, dbname.lock) will directly fail with EEXIST if the .lock file already exists, without even reaching this point.
I don't think the link count here could ever reach anything greater than 2. On the other hand I think a (wrong/cached) value of 1 could be observed in some situations.
Yes it is happening in a bit of complex environment with bindmounts and overlays involved, but I suspect the real culprit is some kind of weakly-synchronized caching on SMP machines.
|
@hallyn while we aren't fully aligned on where to pinpoint the real issue and how to fix it, I think this logging-only PR should be safe to land without introducing further havoc and may help shedding some light on the low-level behavior at play. Could you maybe review/merge this? |
Yes looks good, thanks. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (log) { | ||
| (void) fprintf (shadow_logfd, | ||
| "%s: %s: lock file already used (nlink: %u)\n", | ||
| shadow_progname, file, sb.st_nlink); |
Check failure
Code scanning / CodeQL
Wrong type of arguments to formatting function
This tweaks the database locking logic so that failures in the
link-checking paths are more detailed.
The rationale for this is that I've experienced a non-deterministic
bug which seems to be coming from this logic, and I'd like to get
more details about the actual failing condition.
Ref: coreos/fedora-coreos-tracker#1250