You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "JosiahWI (via GitHub)" <gi...@apache.org> on 2024/07/18 13:34:41 UTC
[PR] Extract Stripe superclass from StripeSM [trafficserver]
JosiahWI opened a new pull request, #11565:
URL: https://github.com/apache/trafficserver/pull/11565
The `StripeSM` class retains all the evacuation data and the mutex.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract Stripe superclass from StripeSM [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #11565:
URL: https://github.com/apache/trafficserver/pull/11565#discussion_r1690205165
##########
src/iocore/cache/P_CacheDir.h:
##########
@@ -266,29 +267,28 @@ struct CacheSync : public Continuation {
// Global Functions
-int dir_probe(const CacheKey *, StripeSM *, Dir *, Dir **);
-int dir_insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
-int dir_overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
-int dir_delete(const CacheKey *key, StripeSM *stripe, Dir *del);
-int dir_lookaside_probe(const CacheKey *key, StripeSM *stripe, Dir *result, EvacuationBlock **eblock);
-int dir_lookaside_insert(EvacuationBlock *b, StripeSM *stripe, Dir *to);
-int dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe);
-void dir_lookaside_cleanup(StripeSM *stripe);
-void dir_lookaside_remove(const CacheKey *key, StripeSM *stripe);
-void dir_free_entry(Dir *e, int s, StripeSM *stripe);
-void dir_sync_init();
-int check_dir(StripeSM *stripe);
-void dir_clean_vol(StripeSM *stripe);
-void dir_clear_range(off_t start, off_t end, StripeSM *stripe);
-int dir_segment_accounted(int s, StripeSM *stripe, int offby = 0, int *free = nullptr, int *used = nullptr, int *empty = nullptr,
- int *valid = nullptr, int *agg_valid = nullptr, int *avg_size = nullptr);
-uint64_t dir_entries_used(StripeSM *stripe);
+int dir_probe(const CacheKey *, StripeSM *, Dir *, Dir **);
+int dir_insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
+int dir_overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
+int dir_delete(const CacheKey *key, StripeSM *stripe, Dir *del);
+int dir_lookaside_probe(const CacheKey *key, StripeSM *stripe, Dir *result, EvacuationBlock **eblock);
+int dir_lookaside_insert(EvacuationBlock *b, StripeSM *stripe, Dir *to);
+int dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe);
+void dir_lookaside_cleanup(StripeSM *stripe);
+void dir_lookaside_remove(const CacheKey *key, StripeSM *stripe);
+void dir_free_entry(Dir *e, int s, Stripe *stripe);
Review Comment:
It looks odd that some of these take `StripeSM*` and some take `Stripe*`. It looks like it's because the mutex is checked which is part of the `Continuation` base class. Does it seem worth changing the signatures on these functions? Could these be part of the Stripe interface rather than bare functions?
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract Stripe superclass from StripeSM [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11565:
URL: https://github.com/apache/trafficserver/pull/11565#discussion_r1690272100
##########
src/iocore/cache/P_CacheDir.h:
##########
@@ -266,29 +267,28 @@ struct CacheSync : public Continuation {
// Global Functions
-int dir_probe(const CacheKey *, StripeSM *, Dir *, Dir **);
-int dir_insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
-int dir_overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
-int dir_delete(const CacheKey *key, StripeSM *stripe, Dir *del);
-int dir_lookaside_probe(const CacheKey *key, StripeSM *stripe, Dir *result, EvacuationBlock **eblock);
-int dir_lookaside_insert(EvacuationBlock *b, StripeSM *stripe, Dir *to);
-int dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe);
-void dir_lookaside_cleanup(StripeSM *stripe);
-void dir_lookaside_remove(const CacheKey *key, StripeSM *stripe);
-void dir_free_entry(Dir *e, int s, StripeSM *stripe);
-void dir_sync_init();
-int check_dir(StripeSM *stripe);
-void dir_clean_vol(StripeSM *stripe);
-void dir_clear_range(off_t start, off_t end, StripeSM *stripe);
-int dir_segment_accounted(int s, StripeSM *stripe, int offby = 0, int *free = nullptr, int *used = nullptr, int *empty = nullptr,
- int *valid = nullptr, int *agg_valid = nullptr, int *avg_size = nullptr);
-uint64_t dir_entries_used(StripeSM *stripe);
+int dir_probe(const CacheKey *, StripeSM *, Dir *, Dir **);
+int dir_insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
+int dir_overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
+int dir_delete(const CacheKey *key, StripeSM *stripe, Dir *del);
+int dir_lookaside_probe(const CacheKey *key, StripeSM *stripe, Dir *result, EvacuationBlock **eblock);
+int dir_lookaside_insert(EvacuationBlock *b, StripeSM *stripe, Dir *to);
+int dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe);
+void dir_lookaside_cleanup(StripeSM *stripe);
+void dir_lookaside_remove(const CacheKey *key, StripeSM *stripe);
+void dir_free_entry(Dir *e, int s, Stripe *stripe);
Review Comment:
I'd like to keep these separate from the `Stripe` interface; as I understand it they are essentially methods on an imaginary cache directory object that we don't yet have a class to represent.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract Stripe superclass from StripeSM [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI merged PR #11565:
URL: https://github.com/apache/trafficserver/pull/11565
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract Stripe superclass from StripeSM [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on PR #11565:
URL: https://github.com/apache/trafficserver/pull/11565#issuecomment-2248680930
This is an code organization change. The long term incentive is separation of concerns, and the short term goal is to split up massive source files into smaller, separate source files. In this case, we're separating the data structure from the event handling. The separation is still subpar because the event handling methods touch the data structure too much, but it's a start. This will be followed by a PR to move all the `StripeSM` event handling into a new `StripeSM.cc` source file.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Extract Stripe superclass from StripeSM [trafficserver]
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #11565:
URL: https://github.com/apache/trafficserver/pull/11565#discussion_r1690310029
##########
src/iocore/cache/P_CacheDir.h:
##########
@@ -266,29 +267,28 @@ struct CacheSync : public Continuation {
// Global Functions
-int dir_probe(const CacheKey *, StripeSM *, Dir *, Dir **);
-int dir_insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
-int dir_overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
-int dir_delete(const CacheKey *key, StripeSM *stripe, Dir *del);
-int dir_lookaside_probe(const CacheKey *key, StripeSM *stripe, Dir *result, EvacuationBlock **eblock);
-int dir_lookaside_insert(EvacuationBlock *b, StripeSM *stripe, Dir *to);
-int dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe);
-void dir_lookaside_cleanup(StripeSM *stripe);
-void dir_lookaside_remove(const CacheKey *key, StripeSM *stripe);
-void dir_free_entry(Dir *e, int s, StripeSM *stripe);
-void dir_sync_init();
-int check_dir(StripeSM *stripe);
-void dir_clean_vol(StripeSM *stripe);
-void dir_clear_range(off_t start, off_t end, StripeSM *stripe);
-int dir_segment_accounted(int s, StripeSM *stripe, int offby = 0, int *free = nullptr, int *used = nullptr, int *empty = nullptr,
- int *valid = nullptr, int *agg_valid = nullptr, int *avg_size = nullptr);
-uint64_t dir_entries_used(StripeSM *stripe);
+int dir_probe(const CacheKey *, StripeSM *, Dir *, Dir **);
+int dir_insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
+int dir_overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
+int dir_delete(const CacheKey *key, StripeSM *stripe, Dir *del);
+int dir_lookaside_probe(const CacheKey *key, StripeSM *stripe, Dir *result, EvacuationBlock **eblock);
+int dir_lookaside_insert(EvacuationBlock *b, StripeSM *stripe, Dir *to);
+int dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe);
+void dir_lookaside_cleanup(StripeSM *stripe);
+void dir_lookaside_remove(const CacheKey *key, StripeSM *stripe);
+void dir_free_entry(Dir *e, int s, Stripe *stripe);
Review Comment:
Maybe we could remove some of the assertions, but it will need careful examination of the code and I think it should be done in a separate PR for safety.
##########
src/iocore/cache/P_CacheDir.h:
##########
@@ -266,29 +267,28 @@ struct CacheSync : public Continuation {
// Global Functions
-int dir_probe(const CacheKey *, StripeSM *, Dir *, Dir **);
-int dir_insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
-int dir_overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
-int dir_delete(const CacheKey *key, StripeSM *stripe, Dir *del);
-int dir_lookaside_probe(const CacheKey *key, StripeSM *stripe, Dir *result, EvacuationBlock **eblock);
-int dir_lookaside_insert(EvacuationBlock *b, StripeSM *stripe, Dir *to);
-int dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe);
-void dir_lookaside_cleanup(StripeSM *stripe);
-void dir_lookaside_remove(const CacheKey *key, StripeSM *stripe);
-void dir_free_entry(Dir *e, int s, StripeSM *stripe);
-void dir_sync_init();
-int check_dir(StripeSM *stripe);
-void dir_clean_vol(StripeSM *stripe);
-void dir_clear_range(off_t start, off_t end, StripeSM *stripe);
-int dir_segment_accounted(int s, StripeSM *stripe, int offby = 0, int *free = nullptr, int *used = nullptr, int *empty = nullptr,
- int *valid = nullptr, int *agg_valid = nullptr, int *avg_size = nullptr);
-uint64_t dir_entries_used(StripeSM *stripe);
+int dir_probe(const CacheKey *, StripeSM *, Dir *, Dir **);
+int dir_insert(const CacheKey *key, StripeSM *stripe, Dir *to_part);
+int dir_overwrite(const CacheKey *key, StripeSM *stripe, Dir *to_part, Dir *overwrite, bool must_overwrite = true);
+int dir_delete(const CacheKey *key, StripeSM *stripe, Dir *del);
+int dir_lookaside_probe(const CacheKey *key, StripeSM *stripe, Dir *result, EvacuationBlock **eblock);
+int dir_lookaside_insert(EvacuationBlock *b, StripeSM *stripe, Dir *to);
+int dir_lookaside_fixup(const CacheKey *key, StripeSM *stripe);
+void dir_lookaside_cleanup(StripeSM *stripe);
+void dir_lookaside_remove(const CacheKey *key, StripeSM *stripe);
+void dir_free_entry(Dir *e, int s, Stripe *stripe);
Review Comment:
Maybe we could remove the assertions, but it will need careful examination of the code and I think it should be done in a separate PR for safety.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org