URL:
https://github.com/SSSD/sssd/pull/999
Title: #999: Mem-cache fixes/improvements
alexey-tikhonov commented:
"""
> But while testing I realized that I see the mem cache full log
message not the at the first time the cache was full. I think this is caused by an older
issue where `mcc->next-slot` is not updated while the cache is filled for the first
time.
...
You are right that this is a bug in the original code. And while
being quite harmless in the original implementation, this old bug indeed creates issue in
the context of my patches.
This is not that straightforward, actually.
The
[
doc](https://docs.pagure.org/SSSD.sssd/developers/mmap_cache_1.15.html#ho...
doesn't describe clearly what is actually implemented on that part, so it is hard to
say if this was done intentionally or by coincidence, but missed update of
`sss_mc_ctx::next_slot` (in case slots were found via explicit lookup of free space) can
be treated as a kind of "feature"...
Let me explain.
Initially we start with an empty buffer and fill it consequently without any gaps
(actually that's not always the case, but more on this later (*)).
```
|R1|R1|R2|R2|R3|R3|...|Rn|Rn|Rn|0|0|...L slots...|0|
```
Until we get to a situation when we need to store record with sizeof(R(n+1)) > L
At this point it doesn't matter if `next_slot` was actually tracking current
"tail": in either way we will end up storing R(n+1) in the beginning of the
buffer:
```
|R(n+1)|R(n+1)|R(n+1)|R(n+1)|R(n+1)|0|...|Rn|Rn|Rn|0|0|...L slots...|0|
```
Now imagine sizeof(R(n+2)) < L so we store it in a free space at the of the buffer:
```
|R(n+1)|R(n+1)|R(n+1)|R(n+1)|R(n+1)|0|...|Rn|Rn|Rn|R(n+2)|R(n+2)|0|...L2...|0|
```
And let sizeof(R(n+3))>L2. This is the point where it becomes important if `next_slot`
was updated on a previous step.
If we fix the "bug" and `next_slot` was updated, we end up rewriting
"fresh" R(n+1).
If `next_slot` was not updated (current implementation), we rewrite "oldest"
record after R(n+1)
The latter makes more sense (most probably only coincidentally), but frankly speaking
implementation is clumsy / inefficient and imo "profit/complexity" trade off is
not the best.
At this point I am very tempted to say that we should follow [this
comment](https://github.com/SSSD/sssd/blob/f3e89aa02ca9c10ab2ac456ca643b3...
and completely get rid of attempt to lookup free space.
This would result in under utilization of the space in the buffer.
For the end of the buffer this should not be a problem as in general sizeof(any single
record) << sizeof(mem-cache)
Another potential source of the "gap" (*) is update of the record that changed
its size. This also should be very rare case.
@sumit-bose, what would you say?
"""
See the full comment at
https://github.com/SSSD/sssd/pull/999#issuecomment-606657210