[Tarantool-patches] [PATCH 0/2] A few fixes for gc and xlog

[Tarantool-patches] [PATCH 0/2] A few fixes for gc and xlog

[Cyrill Gorcunov via Tarantool-patches]

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

[Tarantool-patches] [PATCH 1/2] gc: use wide integer for schedule counting

[Cyrill Gorcunov via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH 1/2] gc: use wide integer for schedule counting

[Vladislav Shpilevoy via Tarantool-patches]

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.

Re: [Tarantool-patches] [PATCH 1/2] gc: use wide integer for schedule counting

[Cyrill Gorcunov via Tarantool-patches]

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

[Tarantool-patches] [PATCH 2/2] xlog: do not sort sole entry

[Cyrill Gorcunov via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH 0/2] A few fixes for gc and xlog

[Serge Petrenko via Tarantool-patches]

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

Re: [Tarantool-patches] [PATCH 0/2] A few fixes for gc and xlog

[Cyrill Gorcunov via Tarantool-patches]

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.

Re: [Tarantool-patches] [PATCH 0/2] A few fixes for gc and xlog

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

LGTM.

> branch gorcunov/gh-5806-xlog-gc

Re: [Tarantool-patches] [PATCH 0/2] A few fixes for gc and xlog

[Kirill Yukhin via Tarantool-patches]

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