Copilot commented on code in PR #12794:
URL: https://github.com/apache/trafficserver/pull/12794#discussion_r2684178684
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -107,17 +112,27 @@ OpenDir::open_write(CacheVC *cont, int allow_if_writers,
int max_writers)
dir_clear(&od->first_dir);
cont->od = od;
cont->write_vector = &od->vector;
- bucket[b].push(od);
+ _bucket[b].push(od);
return 1;
}
+/**
+ This event handler is called in two cases:
+
+ 1. Direct call from OpenDir::close_write - writer lock is already acquired
+ 2. Self retry through event system - need to acquire writer lock
+ */
int
-OpenDir::signal_readers(int /* event ATS_UNUSED */, Event * /* e ATS_UNUSED */)
+OpenDir::signal_readers(int event, Event * /* ATS UNUSED*/)
Review Comment:
The comment formatting uses "ATS UNUSED" with a space, but elsewhere in the
codebase it's typically "ATS_UNUSED" with an underscore. Consider maintaining
consistency with the existing pattern.
```suggestion
OpenDir::signal_readers(int event, Event * /* ATS_UNUSED */)
```
##########
src/iocore/cache/StripeSM.h:
##########
@@ -101,14 +101,15 @@ class StripeSM : public Continuation, public Stripe
int recover_data();
- int open_write(CacheVC *cont, int allow_if_writers, int max_writers);
- int open_write_lock(CacheVC *cont, int allow_if_writers, int max_writers);
- int close_write(CacheVC *cont);
- int begin_read(CacheVC *cont) const;
- // unused read-write interlock code
- // currently http handles a write-lock failure by retrying the read
+ // OpenDir API
+ int open_write(CacheVC *cont, int allow_if_writers, int
max_writers);
+ int open_write_lock(CacheVC *cont, int allow_if_writers, int
max_writers);
+ int close_write(CacheVC *cont);
OpenDirEntry *open_read(const CryptoHash *key) const;
- int close_read(CacheVC *cont) const;
+
+ // PreservationTable API
+ int begin_read(CacheVC *cont) const;
+ int close_read(CacheVC *cont) const;
Review Comment:
Inconsistent formatting: The comment "// OpenDir API" and "//
PreservationTable API" use double slashes, but there are also methods without
such grouping comments. Consider whether this level of API grouping is helpful,
and if so, ensure it's applied consistently throughout the class. For example,
other method groups could benefit from similar API comments.
##########
src/iocore/cache/Cache.cc:
##########
@@ -543,20 +565,24 @@ Cache::open_read(Continuation *cont, const CacheKey *key,
CacheHTTPHdr *request,
OpenDirEntry *od = nullptr;
CacheVC *c = nullptr;
+ // Read-While-Writer
+ // This OpenDirEntry lookup doesn't need stripe mutex lock because OpenDir
has own reader-writer lock
+ od = stripe->open_read(key);
Review Comment:
Potential lock ordering issue: This code calls stripe->open_read() (which
now acquires _shared_mutex as a reader) while holding stripe->mutex. In other
places like CacheRead.cc line 212, the same pattern exists. However, in
Cache.cc line 570, open_read() is called without holding stripe->mutex. While
this change is intentional to reduce contention, ensure that this mixed lock
ordering (sometimes stripe->mutex then _shared_mutex, sometimes just
_shared_mutex) doesn't introduce potential deadlock scenarios. The Bravo
reader-writer lock should handle this safely since multiple readers can
coexist, but this should be verified through testing under high concurrency.
##########
src/iocore/cache/P_CacheDir.h:
##########
@@ -225,16 +226,23 @@ struct OpenDirEntry {
}
};
-struct OpenDir : public Continuation {
- Queue<CacheVC, Link_CacheVC_opendir_link> delayed_readers;
- DLL<OpenDirEntry> bucket[OPEN_DIR_BUCKETS];
+class OpenDir : public Continuation
+{
+public:
+ OpenDir();
int open_write(CacheVC *c, int allow_if_writers, int max_writers);
int close_write(CacheVC *c);
OpenDirEntry *open_read(const CryptoHash *key) const;
- int signal_readers(int event, Event *e);
- OpenDir();
+ // event handler
+ int signal_readers(int event, Event *e);
+
+private:
+ mutable ts::bravo::shared_mutex _shared_mutex;
+
+ Queue<CacheVC, Link_CacheVC_opendir_link> _delayed_readers;
+ DLL<OpenDirEntry> _bucket[OPEN_DIR_BUCKETS];
};
Review Comment:
Missing documentation: The class documentation should be updated to explain
the new locking mechanism. Specifically, document that OpenDir now uses its own
reader-writer lock (_shared_mutex) instead of relying on the StripeSM mutex,
which allows concurrent reads while writes are in progress. This is a
significant architectural change that should be clearly documented for
maintainability.
##########
src/iocore/cache/CacheDir.cc:
##########
@@ -46,6 +49,7 @@ DbgCtl dbg_ctl_dir_clean{"dir_clean"};
#ifdef DEBUG
DbgCtl dbg_ctl_cache_stats{"cache_stats"};
+DbgCtl dbg_ctl_cache_open_dir{"cache_open_dir"};
Review Comment:
Unused debug control: The debug control variable dbg_ctl_cache_open_dir is
defined but never used in the code. If this was added for future debugging
purposes, consider adding a comment explaining its intended use. Otherwise, it
should be removed to avoid clutter.
```suggestion
```
--
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]
