While work on https://github.com/tarantool/tarantool/issues/5806 is still in progress I would like to share a few patches which I think are useful. Please take a look. branch gorcunov/gh-5806-xlog-gc Cyrill Gorcunov (2): gc: use wide integer for schedule counting xlog: do not sort sole entry src/box/gc.c | 6 +++--- src/box/gc.h | 2 +- src/box/xlog.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) base-commit: 3a7c21023367424baa9bf2d4d823102cb2b3c751 -- 2.29.2
Currently we use unsined int for "cleanup" schedule counting, this is safe while this routine is not called too often. Still there is a chance to hit a number wrap on code modification because there is no strict rule on how to use this garbage collector. Lets use wide integers instead, we have only one gc instance and such approach eliminates potential problems in future (actually this should had been done from the beginning since the current gc code flow developed without wrapping in mind). In-scope-of #5806 Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> --- src/box/gc.c | 6 +++--- src/box/gc.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/box/gc.c b/src/box/gc.c index 1f8cc818d..86c229c34 100644 --- a/src/box/gc.c +++ b/src/box/gc.c @@ -239,15 +239,15 @@ gc_cleanup_fiber_f(va_list ap) { (void)ap; while (!fiber_is_cancelled()) { - int delta = gc.cleanup_scheduled - gc.cleanup_completed; + int64_t delta = gc.cleanup_scheduled - gc.cleanup_completed; if (delta == 0) { /* No pending garbage collection. */ fiber_sleep(TIMEOUT_INFINITY); continue; } - assert(delta > 0); gc_run_cleanup(); gc.cleanup_completed += delta; + assert(delta > 0 && gc.cleanup_completed > 0); fiber_cond_signal(&gc.cleanup_cond); } return 0; @@ -278,7 +278,7 @@ gc_schedule_cleanup(void) static void gc_wait_cleanup(void) { - unsigned scheduled = gc.cleanup_scheduled; + int64_t scheduled = gc.cleanup_scheduled; while (gc.cleanup_completed < scheduled) fiber_cond_wait(&gc.cleanup_cond); } diff --git a/src/box/gc.h b/src/box/gc.h index 829aaf479..2a568c5f9 100644 --- a/src/box/gc.h +++ b/src/box/gc.h @@ -146,7 +146,7 @@ struct gc_state { * sleep until @completed reaches the value of @scheduled * taken at that moment of time. */ - unsigned cleanup_completed, cleanup_scheduled; + int64_t cleanup_completed, cleanup_scheduled; /** * Set if there's a fiber making a checkpoint right now. */ -- 2.29.2
In case if there only one snapshot or xlog file there is no need to call sorting procedure at all. In-scope-of #5806 Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com> --- src/box/xlog.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/box/xlog.c b/src/box/xlog.c index 974f460be..89df7e485 100644 --- a/src/box/xlog.c +++ b/src/box/xlog.c @@ -588,7 +588,7 @@ xdir_scan(struct xdir *dir, bool is_dir_required) signatures[s_count++] = signature; } /** Sort the list of files */ - if (s_count > 0) + if (s_count > 1) qsort(signatures, s_count, sizeof(*signatures), cmp_i64); /** * Update the log dir index with the current state: -- 2.29.2
10.03.2021 23:47, Cyrill Gorcunov пишет:
> While work on https://github.com/tarantool/tarantool/issues/5806
> is still in progress I would like to share a few patches which
> I think are useful. Please take a look.
>
> branch gorcunov/gh-5806-xlog-gc
>
> Cyrill Gorcunov (2):
> gc: use wide integer for schedule counting
> xlog: do not sort sole entry
>
> src/box/gc.c | 6 +++---
> src/box/gc.h | 2 +-
> src/box/xlog.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
>
> base-commit: 3a7c21023367424baa9bf2d4d823102cb2b3c751
Thanks for the patchset!
LGTM, but I doubt Vlad will like this =)
--
Serge Petrenko
On Fri, Mar 12, 2021 at 08:11:25PM +0300, Serge Petrenko wrote:
>
> 10.03.2021 23:47, Cyrill Gorcunov пишет:
> > While work on https://github.com/tarantool/tarantool/issues/5806
> > is still in progress I would like to share a few patches which
> > I think are useful. Please take a look.
> >
> > branch gorcunov/gh-5806-xlog-gc
> >
> > Cyrill Gorcunov (2):
> > gc: use wide integer for schedule counting
> > xlog: do not sort sole entry
> >
> > src/box/gc.c | 6 +++---
> > src/box/gc.h | 2 +-
> > src/box/xlog.c | 2 +-
> > 3 files changed, 5 insertions(+), 5 deletions(-)
> >
> >
> > base-commit: 3a7c21023367424baa9bf2d4d823102cb2b3c751
> Thanks for the patchset!
>
> LGTM, but I doubt Vlad will like this =)
Thanks! Both fixes are trivial but definitely needed ones.
Hi! Thanks for the patch!
> diff --git a/src/box/gc.c b/src/box/gc.c
> index 1f8cc818d..86c229c34 100644
> --- a/src/box/gc.c
> +++ b/src/box/gc.c
> @@ -239,15 +239,15 @@ gc_cleanup_fiber_f(va_list ap)
> {
> (void)ap;
> while (!fiber_is_cancelled()) {
> - int delta = gc.cleanup_scheduled - gc.cleanup_completed;
> + int64_t delta = gc.cleanup_scheduled - gc.cleanup_completed;
> if (delta == 0) {
> /* No pending garbage collection. */
> fiber_sleep(TIMEOUT_INFINITY);
> continue;
> }
> - assert(delta > 0);
> gc_run_cleanup();
> gc.cleanup_completed += delta;
> + assert(delta > 0 && gc.cleanup_completed > 0);
You didn't need to change the assertion. If `delta` is always
positive, and `cleanup_completed` is populated only from `delta`,
it is also positive always.
On Sun, Mar 14, 2021 at 05:30:57PM +0100, Vladislav Shpilevoy wrote: > > - assert(delta > 0); > > gc_run_cleanup(); > > gc.cleanup_completed += delta; > > + assert(delta > 0 && gc.cleanup_completed > 0); > > You didn't need to change the assertion. If `delta` is always > positive, and `cleanup_completed` is populated only from `delta`, > it is also positive always. This allows to make sure that gc.cleanup_completed didn't change in gc_run_cleanup in unpredicted way. I'll update if you prefer, sure thing. Force pushed. --- diff --git a/src/box/gc.c b/src/box/gc.c index 1f8cc818d..9af4ef958 100644 --- a/src/box/gc.c +++ b/src/box/gc.c @@ -239,7 +239,7 @@ gc_cleanup_fiber_f(va_list ap) { (void)ap; while (!fiber_is_cancelled()) { - int delta = gc.cleanup_scheduled - gc.cleanup_completed; + int64_t delta = gc.cleanup_scheduled - gc.cleanup_completed; if (delta == 0) { /* No pending garbage collection. */ fiber_sleep(TIMEOUT_INFINITY); @@ -278,7 +278,7 @@ gc_schedule_cleanup(void) static void gc_wait_cleanup(void) { - unsigned scheduled = gc.cleanup_scheduled; + int64_t scheduled = gc.cleanup_scheduled; while (gc.cleanup_completed < scheduled) fiber_cond_wait(&gc.cleanup_cond); } diff --git a/src/box/gc.h b/src/box/gc.h index 829aaf479..2a568c5f9 100644 --- a/src/box/gc.h +++ b/src/box/gc.h @@ -146,7 +146,7 @@ struct gc_state { * sleep until @completed reaches the value of @scheduled * taken at that moment of time. */ - unsigned cleanup_completed, cleanup_scheduled; + int64_t cleanup_completed, cleanup_scheduled; /** * Set if there's a fiber making a checkpoint right now. */ -- 2.30.2
Hi! Thanks for the patch!
LGTM.
> branch gorcunov/gh-5806-xlog-gc
Hello,
On 10 мар 23:47, Cyrill Gorcunov wrote:
> While work on https://github.com/tarantool/tarantool/issues/5806
> is still in progress I would like to share a few patches which
> I think are useful. Please take a look.
>
> branch gorcunov/gh-5806-xlog-gc
I've checked your patchset into 2.6, 2.7 and master.
--
Regards, Kirill Yukhin