nscd: bump GC cycle during cache pruning (bug 26130)
Commit Message
While nscd prunes a cache it becomes inconsistent temporarily, which is
visible to clients if that cache is shared. Bump the GC cycle counter so
that the clients notice the modification window.
---
nscd/cache.c | 9 +++++++++
1 file changed, 9 insertions(+)
Comments
* Andreas Schwab:
> While nscd prunes a cache it becomes inconsistent temporarily, which is
> visible to clients if that cache is shared. Bump the GC cycle counter so
> that the clients notice the modification window.
> ---
> nscd/cache.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/nscd/cache.c b/nscd/cache.c
> index 94163d9ce3..2ceac94c23 100644
> --- a/nscd/cache.c
> +++ b/nscd/cache.c
> @@ -453,6 +453,11 @@ prune_cache (struct database_dyn *table, time_t now, int fd)
> pthread_rwlock_wrlock (&table->lock);
> }
>
> + /* Now we start modifying the data. Make sure all readers of the
> + data are aware of this and temporarily don't use the data. */
> + ++table->head->gc_cycle;
> + assert ((table->head->gc_cycle & 1) == 1);
> +
> while (first <= last)
> {
> if (mark[first])
> @@ -493,6 +498,10 @@ prune_cache (struct database_dyn *table, time_t now, int fd)
> ++first;
> }
>
> + /* Now we are done modifying the data. */
> + ++table->head->gc_cycle;
> + assert ((table->head->gc_cycle & 1) == 0);
> +
> /* It's all done. */
> pthread_rwlock_unlock (&table->lock);
I think this needs barriers after and before the increments.
Thanks,
Florian
On Jun 29 2020, Florian Weimer wrote:
> I think this needs barriers after and before the increments.
Why doesn't gc need those barriers?
Andreas.
* Andreas Schwab:
> On Jun 29 2020, Florian Weimer wrote:
>
>> I think this needs barriers after and before the increments.
>
> Why doesn't gc need those barriers?
I think it needs them as well. I thought we had them there. 8-(
Thanks,
Florian
On Jun 29 2020, Florian Weimer via Libc-alpha wrote:
> * Andreas Schwab:
>
>> On Jun 29 2020, Florian Weimer wrote:
>>
>>> I think this needs barriers after and before the increments.
>>
>> Why doesn't gc need those barriers?
>
> I think it needs them as well. I thought we had them there. 8-(
Should they use atomic_increment?
Andreas.
On 6/29/20 10:09 AM, Andreas Schwab wrote:
> On Jun 29 2020, Florian Weimer via Libc-alpha wrote:
>
>> * Andreas Schwab:
>>
>>> On Jun 29 2020, Florian Weimer wrote:
>>>
>>>> I think this needs barriers after and before the increments.
>>>
>>> Why doesn't gc need those barriers?
>>
>> I think it needs them as well. I thought we had them there. 8-(
>
> Should they use atomic_increment?
There are many concurrency problems with the cache, and we do see
these issues with downstream customers. Torvald reviewed this at
one point and produced a patch that I've attached to bug 25888.
Our latest attempt to fix this is here:
https://sourceware.org/bugzilla/show_bug.cgi?id=25888
https://sourceware.org/bugzilla/attachment.cgi?id=12493
* Andreas Schwab:
> On Jun 29 2020, Florian Weimer via Libc-alpha wrote:
>
>> * Andreas Schwab:
>>
>>> On Jun 29 2020, Florian Weimer wrote:
>>>
>>>> I think this needs barriers after and before the increments.
>>>
>>> Why doesn't gc need those barriers?
>>
>> I think it needs them as well. I thought we had them there. 8-(
>
> Should they use atomic_increment?
Yes, that as well. I don't know if the legacy atomic_increment function
implies a barrier. I think we need the equivalent of __atomic_fetch_add
(or __atomic_add_fetch) with __ATOMIC_ACQ_REL, and our <atomic.h> does
not have this, and we are not supposed to use the compiler built-ins.
atomic_fetch_add_relaxed together with atomic_full_barrier should work,
though.
Thanks,
Florian
On Jun 29 2020, Carlos O'Donell via Libc-alpha wrote:
> https://sourceware.org/bugzilla/attachment.cgi?id=12493
That doesn't fix this bug.
Andreas.
@@ -453,6 +453,11 @@ prune_cache (struct database_dyn *table, time_t now, int fd)
pthread_rwlock_wrlock (&table->lock);
}
+ /* Now we start modifying the data. Make sure all readers of the
+ data are aware of this and temporarily don't use the data. */
+ ++table->head->gc_cycle;
+ assert ((table->head->gc_cycle & 1) == 1);
+
while (first <= last)
{
if (mark[first])
@@ -493,6 +498,10 @@ prune_cache (struct database_dyn *table, time_t now, int fd)
++first;
}
+ /* Now we are done modifying the data. */
+ ++table->head->gc_cycle;
+ assert ((table->head->gc_cycle & 1) == 0);
+
/* It's all done. */
pthread_rwlock_unlock (&table->lock);