Add in-progress 304 rock updates to mgr:openfd_objects

[eduard-bagdasaryan]

No description provided.

[eduard-bagdasaryan]

I checked separately that sizeof(swapoutInterest) is 16 (vs 4, if without virtual destructor), i.e., it keeps vtable pointer. That means that we should consider moving to a boolean variable instead (implying other related problems fixed first).

[rousskov]

That means that we should consider moving to a boolean variable instead

I see several other alternatives to consider. I will enumerate all the current candidates for completeness sake:

1. Use boolean swapoutInterest (as you have suggested in this change request). Creates a problem for future update-during-swapout support. Implies prohibiting (at compile time) Ipc::StoreMapUpdate copying. AFAICT, that copying prohibition is not possible without losing features or serious refactoring!
2. Remove virtual table from Lock. I have not checked the details, but I doubt that is easy to do while preserving existing class hierarchy safety: Even if RefCount is the only child that needs a virtual destructor, prohibiting (at compile time) deletion of Lock pointers in all contexts may be impossible because Lock code can always delete a pointer to a Lock object.
3. Add a simple "Level" counter (that Lock will reuse). This is the simplest solution that I can see:

class Lock {
...
private:
    mutable Level level;
};

class MemObject: ... {
...
   Level swapoutInterest; 
};

4. Storing a pointer to the header updating job/state inside MemObject. This is similar to what this PR has implemented in the very beginning. The advantage of this design is that cache manager can (eventually) report details specific to the ongoing header updates. I have criticized this option earlier, in part, because it bloats MemObject (which is used for all non-SMP memory cache entries), but one could argue that we should not be responsible for MemObject design mistakes -- the class should be split into several classes so that idle memory cache objects do not have to keep state relevant to active transactions. Until that split is implemented, perhaps one has a right to add stuff to the "active transaction" part of MemObject, even if doing so bloats non-SMP memory caches. Moreover, all the solutions mentioned above increase MemObject memory footprint anyway.

5. Register header updating jobs in some new global std::set. The cache manager reporting code will use that global to report those updates (in addition to iterating through all MemObjects). If MemObject will be split as discussed in item 3, then there will be nothing wrong with iterating through all active objects, but it may take us a very long time to get there. Meanwhile, this solution avoids memory cache bloat present in earlier items. Perhaps we can even generalize this to report all AsyncJobs? That would be useful in triage of a variety of problems! Squid already reports all event handlers and all active requests. Why not all jobs?

6. Use existing MemObject::abortCallback as a "Store writing interest" signal. StoreEntry::registerAbortCallback() is meant to be used by StoreEntry writers. The header updating job is a StoreEntry writer. It is probably a bug that it does not registerAbortCallback() -- the entry may be aborted by a 3rd party while we are updating it, right? If we can start using this interface, we will fix that bug and improve reporting. Can we find a way to trigger that bug in the current code (i.e. to trigger a call to StoreEntry::abort() while a header upgrade is in progress)? Perhaps Store::Controller::syncCollapsed() is able to trigger such StoreEntry aborts?

7. Use existing Transients::hasWriter(StoreEntry) check to detect header updates. This solution requires creating or marking the corresponding "writing" Transients entry (and iterating through all transients when preparing the cache manager report). Similar to the above item, this is probably something correct SMP header updating code should do anyway!

Overall, I like the last two items because they feel like potentially important bug fixes, but I am worried that it would be difficult to implement either of them. I love the general usefulness and simplicity of the "report all jobs" variation in item 4. I suggest adding a mgr:jobs report that will ask each AsyncJob object to report its state, possibly using the existing status() method. I would do that in a new/dedicated PR/issue.

[rousskov]

That means that we should consider moving to a boolean variable instead

I see several other alternatives to consider. I will enumerate all the current candidates for completeness sake:

1. Use boolean swapoutInterest (as you have suggested in this change request). Creates a problem for future update-during-swapout support. Implies prohibiting (at compile time) Ipc::StoreMapUpdate copying. AFAICT, that copying prohibition is not possible without losing features or serious refactoring!
2. Remove virtual table from Lock. I have not checked the details, but I doubt that is easy to do while preserving existing class hierarchy safety: Even if RefCount is the only child that needs a virtual destructor, prohibiting (at compile time) deletion of Lock pointers in all contexts may be impossible because Lock code can always delete a pointer to a Lock object.
3. Add a simple "Level" counter (that Lock will reuse). This is the simplest solution that I can see:

class Lock {
...
private:
    mutable Level level;
};

class MemObject: ... {
...
   Level swapoutInterest; 
};

4. Storing a pointer to the header updating job/state inside MemObject. This is similar to what this PR has implemented in the very beginning. The advantage of this design is that cache manager can (eventually) report details specific to the ongoing header updates. I have criticized this option earlier, in part, because it bloats MemObject (which is used for all non-SMP memory cache entries), but one could argue that we should not be responsible for MemObject design mistakes -- the class should be split into several classes so that idle memory cache objects do not have to keep state relevant to active transactions. Until that split is implemented, perhaps one has a right to add stuff to the "active transaction" part of MemObject, even if doing so bloats non-SMP memory caches. Moreover, all the solutions mentioned above increase MemObject memory footprint anyway.

5. Register header updating jobs in some new global std::set. The cache manager reporting code will use that global to report those updates (in addition to iterating through all MemObjects). If MemObject will be split as discussed in item 3, then there will be nothing wrong with iterating through all active objects, but it may take us a very long time to get there. Meanwhile, this solution avoids memory cache bloat present in earlier items. Perhaps we can even generalize this to report all AsyncJobs? That would be useful in triage of a variety of problems! Squid already reports all event handlers and all active requests. Why not all jobs?

6. Use existing MemObject::abortCallback as a "Store writing interest" signal. StoreEntry::registerAbortCallback() is meant to be used by StoreEntry writers. The header updating job is a StoreEntry writer. It is probably a bug that it does not registerAbortCallback() -- the entry may be aborted by a 3rd party while we are updating it, right? If we can start using this interface, we will fix that bug and improve reporting. Can we find a way to trigger that bug in the current code (i.e. to trigger a call to StoreEntry::abort() while a header upgrade is in progress)? Perhaps Store::Controller::syncCollapsed() is able to trigger such StoreEntry aborts?

7. Use existing Transients::hasWriter(StoreEntry) check to detect header updates. This solution requires creating or marking the corresponding "writing" Transients entry (and iterating through all transients when preparing the cache manager report). Similar to the above item, this is probably something correct SMP header updating code should do anyway!

Overall, I like the last two items because they feel like potentially important bug fixes, but I am worried that it would be difficult to implement either of them. I love the general usefulness and simplicity of the "report all jobs" variation in item 4. I suggest adding a mgr:jobs report that will ask each AsyncJob object to report its state, possibly using the existing status() method. I would do that in a new/dedicated PR/issue.