masaori335 commented on code in PR #13328:
URL: https://github.com/apache/trafficserver/pull/13328#discussion_r3702473239
##########
src/iocore/cache/StripeSM.cc:
##########
@@ -1326,9 +1347,26 @@ StripeSM::shutdown(EThread *shutdown_thread)
SCOPED_MUTEX_LOCK(lock, this->mutex, shutdown_thread);
if (DISK_BAD(this->disk)) {
- Dbg(dbg_ctl_cache_dir_sync, "Dir %s: ignoring -- bad disk",
this->hash_text.get());
+ // Bad disk: invalidate the shm copy so the next start recovers from disk
+ // (mirrors the flush-failure branch below).
+ Dbg(dbg_ctl_cache_dir_sync, "Dir %s: bad disk -- invalidating shm copy for
disk recovery", this->hash_text.get());
+ CacheShm::invalidate_stripe_directory(this->directory.raw_dir);
return;
}
+
+ bool shm_backed = CacheShm::is_shm_pointer(this->directory.raw_dir);
+
+ // In-flight AIO write: aggWriteDone advances write_pos again after we drop
this mutex, so
+ // the shm header is not final to trust verbatim. Invalidate it and fall
through to the
+ // on-disk write so the next start recovers from a fresh snapshot (short
recover_data()
+ // scan) instead of a stale periodic-sync copy -- the same path a non-shm
stripe takes,
+ // and recover_data() rescans the tail so the imprecise write_pos is
reconciled.
+ if (shm_backed && this->is_io_in_progress()) {
+ Dbg(dbg_ctl_cache_dir_sync, "Dir %s: AIO write in flight -- invalidating
shm copy, syncing dir to disk", this->hash_text.get());
+ CacheShm::invalidate_stripe_directory(this->directory.raw_dir);
+ shm_backed = false;
+ }
Review Comment:
Good catch, and thanks — I went with the second option you suggested: a
separate per-stripe invalidation marker that never touches the disk-write
buffer.
`CacheShm::invalidate_stripe_directory()` no longer stores through `raw_dir`
at all. It sets a `dir_untrusted` byte on the stripe's `StripeEntry` in the
control segment, which is never a source buffer for a disk write. On the next
start `attach_or_create_stripe()` refuses a marked entry and creates a fresh
segment; that segment's zero magic sends `StripeSM::init()` down the ordinary
disk-read + `recover_data()` path, so the recovery outcome is unchanged — only
the mechanism for signalling it moved.
Worth noting your diagnosis was broader than my first attempt at it.
Deferring the invalidation to function exit stopped `shutdown()`'s own pwrite
from carrying the zeroed magic, but the periodic dir sync reaches the same
buffer from a different continuation (`memcpy(buf, stripe->directory.raw_dir,
dirlen)` in `CacheDir.cc`), so that route was still open — and on that one the
consequence is worse than extended recovery: a zeroed magic reaching the
on-disk A/B copy makes the next start *clear* the stripe instead of recovering
it. Moving the marker out of `raw_dir` closes both routes by construction, and
let the deferral be deleted rather than kept as an ordering constraint.
Regression test: `"StripeSM::shutdown marks the stripe untrusted without
touching the directory"` in `test_CacheShmShutdown.cc` enters this exact
`is_io_in_progress()` path and asserts both that the in-shm magic is still
`STRIPE_MAGIC` and that the on-disk copy reads back `STRIPE_MAGIC`. I checked
it fails if the change is reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
