afpd: force-close stale forks during FPDelete to unblock deletion#2851
afpd: force-close stale forks during FPDelete to unblock deletion#2851
Conversation
andylemin
force-pushed
the
delete-fix-stale-forks
branch
from
de34a85 to
af33cd5
Compare
andylemin
force-pushed
the
delete-fix-stale-forks
branch
from
af33cd5 to
66bbc2c
Compare
andylemin
force-pushed
the
delete-fix-stale-forks
branch
from
66bbc2c to
5c07511
Compare
rdmark
left a comment
rdmark
left a comment
There was a problem hiding this comment.
does Preview.app have an actual built-in AFP client?
do you know of other macOS apps that do as well?
Well this is one of those changes that has affected me personally for a long time. It seems to be related to the Ventura (and some other versions - not tested all) kernel module itself, with any application that peeks or previews (quick view) data (including Preview, image magick CLI on terminal, and Photoshop) - many tools used by photographers. I suspect these kinds of apps open read only, and the kernel module upgrades to open an extra Write mode on behalf of the app, but does not close all when the app closes. Needs some testing before merge |
andylemin
force-pushed
the
delete-fix-stale-forks
branch
from
5c07511 to
85bd5e3
Compare
|
augment review |
andylemin
force-pushed
the
delete-fix-stale-forks
branch
from
b3853b0 to
4975082
Compare
andylemin
force-pushed
the
delete-fix-stale-forks
branch
from
4975082 to
0e01b31
Compare
…ling
When a client deletes a file via FPDelete, afpd now automatically
force-closes stale open forks that are not dirty and hold no locks,
allowing the delete to succeed without requiring the client to
explicitly close every fork first.
For forks holding byte-range locks, a three-tier safety check applies:
1. Dirty forks (pending metadata flush) — always block deletion.
2. Write-locked forks (F_WRLCK) — always block deletion, as write
locks indicate a potentially active writer.
3. Read-locked forks (F_RDLCK only) — controlled by the new
'close stale rlocks' config option (default: no). When enabled,
read-locked stale forks are force-closed since the file is being
destroyed and read locks only guard against concurrent writers.
Implementation:
- Add adf_has_wrlocks() helper to ad_lock.c to distinguish read-only
locked forks from write-locked forks by scanning the adf_lock array
for F_WRLCK entries.
- Add of_close_stale_forks() to ofork.c, called from afp_delete() in
filedir.c when open forks are detected on the target file.
- Add OPTION_CLOSE_STALE_RLOCKS flag and 'close stale rlocks' config
option (default: no) parsed via getoption_bool() in netatalk_conf.c.
New afp.conf option:
close stale rlocks = BOOLEAN (default: no) [Global]
Docker env var: AFP_CLOSE_STALE_RLOCKS (default: no)
andylemin
force-pushed
the
delete-fix-stale-forks
branch
from
0e01b31 to
409f413
Compare
|
🔥 Spectest (AFP 3.4) - Flamegraph (AFP_ASSERT active)Commit: 🔥 Open interactive Flamegraph (SVG) 🔝 Top 10 leaf functions
|
🤖 Augment PR SummarySummary: This PR makes Changes:
Technical Notes: Stale forks are skipped when flagged DIRTY or when the shared AppleDouble state indicates outstanding locks, aiming to avoid metadata loss or lock violations. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| * The zero-length resource fork check in of_closefork() may also | ||
| * unlink the AppleDouble file early. This is safe because the | ||
| * delete path handles ENOENT gracefully via netatalk_unlinkat(). */ | ||
| of_closefork(obj, of); |
There was a problem hiding this comment.
of_close_stale_forks() force-closes forks based only on AFPFORK_DIRTY and lockcount, but it can still close a fork that was opened with write access (AFPFORK_ACCWR) if it currently has no locks/dirty flag. That would allow FPDelete to proceed by terminating an active writer, which seems different from the intended “stale read fork” cleanup behavior.
Severity: high
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
2 participants
Non-compliant AFP clients (e.g. Preview.app) sometimes leave open forks after completing operations, causing FPDelete to return AFPERR_BUSY when trying to delete after viewing.
Add of_close_stale_forks() which iterates all open forks matching the target file (by dev/ino) and force-closes any fork that passes both safety checks:
AFPFORK_MODIFIED is intentionally not checked since the file is being deleted and any data written via FPWrite is already on disk.
In afp_delete(), when of_findname() detects open forks, attempt stale fork cleanup before returning AFPERR_BUSY. If all forks are closed, re-verify with of_findname() and proceed to deletion. If any fork remains active (dirty or locked), return AFPERR_BUSY as before.
This is safe because:
Extensive investigation and debugging (2 weeks of testing with logs and Wireshark - I found it hard to believe Apple are the ones who violate the spec sometimes) was performed to validate that this is NOT a Netatalk issue.
Tracing AFP operations from some clients (such as macOS Ventura) using specific applications, it was proven that the AFP spec is sometimes violated by clients.
The spec states if a client opens a file for Read, and opens it for Writing later, the client must still close both open's - not just the most privileged open.
As there is only a single file descriptor on the client, it is understandable that sometimes this tracking fails and only one close is received for the client's fd (instead of an exact close for each original open), leaving a stale fork on the server - subsequently blocking deletions.
This forced-cleanup does not violate the AFP specification.