[Tarantool-patches] [PATCH 0/2] Fix crash in case of lack of FDs during recovery

[Tarantool-patches] [PATCH 0/2] Fix crash in case of lack of FDs during recovery

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/commits/np/gh-4805-too-many-fds
Issue: https://github.com/tarantool/tarantool/issues/4805

First patch adds simple macro which allows error injection to be delayed.
It also can be used in this series:
https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016367.html

Nikita Pettik (2):
  errinj: introduce delayed injection
  vinyl: drop wasted runs in case range recovery fails

 src/box/vy_lsm.c                              |  14 ++-
 src/box/vy_run.c                              |   4 +
 src/errinj.h                                  |  10 ++
 test/box/errinj.result                        |   1 +
 test/vinyl/errinj_recovery.lua                |  10 ++
 .../gh-4805-open-run-err-recovery.result      | 101 ++++++++++++++++++
 .../gh-4805-open-run-err-recovery.test.lua    |  38 +++++++
 test/vinyl/suite.ini                          |   2 +-
 8 files changed, 176 insertions(+), 4 deletions(-)
 create mode 100644 test/vinyl/errinj_recovery.lua
 create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.result
 create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.test.lua

-- 
2.17.1

[Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Nikita Pettik]

With new macro ERROR_INJECT_DELAYED it is possible to delay error
injection by DELAY parameter: injection will be set only after DELAY
times the path is executed. For instance:

void
foo(int i)
{
	/* 2 is delay counter. */
	ERROR_INJECT_DELAYED(ERRINJ_FOO, 2, {
		 printf("Error injection on %d cycle!\n", i);
		});
}

void
boo(void)
{
	for (int i = 0; i < 10; ++i)
		foo(i);
}

The result is "Error injection on 2 cycle!". This type of error
injection can turn out to be useful to set injection in the middle of
query processing. Imagine following scenario:

void
foo(void)
{
	int *fds[10];
	for (int i = 0; i < 10; ++i) {
		fds[i] = malloc(sizeof(int));
		if (fds[i] == NULL)
			goto cleanup;
	}
cleanup:
	free(fds[0]);
}

"cleanup" section obviously contains error and leads to memory leak.
But using means of casual error injection without delay such situation
can't be detected: OOM can be set only for first cycle iteration and in
this particular case no leaks take place.
---
 src/errinj.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/errinj.h b/src/errinj.h
index 2cb090b68..990f4921d 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -149,6 +149,7 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
 #ifdef NDEBUG
 #  define ERROR_INJECT(ID, CODE)
 #  define errinj(ID, TYPE) ((struct errinj *) NULL)
+#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE)
 #else
 #  /* Returns the error injection by id */
 #  define errinj(ID, TYPE) \
@@ -162,6 +163,14 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
 		if (errinj(ID, ERRINJ_BOOL)->bparam) \
 			CODE; \
 	} while (0)
+#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
+	do { \
+		static int _DELAY##ID = DELAY; \
+		if (errinj(ID, ERRINJ_BOOL)->bparam) { \
+			if (_DELAY##ID-- == 0) \
+				CODE; \
+		} \
+	} while (0)
 #endif
 
 #define ERROR_INJECT_RETURN(ID) ERROR_INJECT(ID, return -1)
-- 
2.17.1

Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/04/30 22:51]:

This is not exactly a time delay, this is a skip-first-n kind of
postponed enable?

Having a time delay is not such a bad idea (as well as a
probabilistic failure).

> With new macro ERROR_INJECT_DELAYED it is possible to delay error
> injection by DELAY parameter: injection will be set only after DELAY
> times the path is executed. For instance:
> 
> void
> foo(int i)
> {
> 	/* 2 is delay counter. */
> 	ERROR_INJECT_DELAYED(ERRINJ_FOO, 2, {
> 		 printf("Error injection on %d cycle!\n", i);
> 		});
> }
> 
> void
> boo(void)
> {
> 	for (int i = 0; i < 10; ++i)
> 		foo(i);
> }
> 
> The result is "Error injection on 2 cycle!". This type of error
> injection can turn out to be useful to set injection in the middle of
> query processing. Imagine following scenario:
> 
> void
> foo(void)
> {
> 	int *fds[10];
> 	for (int i = 0; i < 10; ++i) {
> 		fds[i] = malloc(sizeof(int));
> 		if (fds[i] == NULL)
> 			goto cleanup;
> 	}
> cleanup:
> 	free(fds[0]);
> }
> 
> "cleanup" section obviously contains error and leads to memory leak.
> But using means of casual error injection without delay such situation
> can't be detected: OOM can be set only for first cycle iteration and in
> this particular case no leaks take place.
> ---
>  src/errinj.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/errinj.h b/src/errinj.h
> index 2cb090b68..990f4921d 100644
> --- a/src/errinj.h
> +++ b/src/errinj.h
> @@ -149,6 +149,7 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
>  #ifdef NDEBUG
>  #  define ERROR_INJECT(ID, CODE)
>  #  define errinj(ID, TYPE) ((struct errinj *) NULL)
> +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE)
>  #else
>  #  /* Returns the error injection by id */
>  #  define errinj(ID, TYPE) \
> @@ -162,6 +163,14 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
>  		if (errinj(ID, ERRINJ_BOOL)->bparam) \
>  			CODE; \
>  	} while (0)
> +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
> +	do { \
> +		static int _DELAY##ID = DELAY; \
> +		if (errinj(ID, ERRINJ_BOOL)->bparam) { \
> +			if (_DELAY##ID-- == 0) \
> +				CODE; \
> +		} \
> +	} while (0)
>  #endif
>  
>  #define ERROR_INJECT_RETURN(ID) ERROR_INJECT(ID, return -1)
> -- 
> 2.17.1
> 

-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com

Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Nikita Pettik]

On 30 Apr 23:15, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [20/04/30 22:51]:
> 
> This is not exactly a time delay, this is a skip-first-n kind of
> postponed enable?

Yep, it's more accurate definition.  I can rename to
ERROR_INJECT_POSPONED, if it fits better.
 
> Having a time delay is not such a bad idea (as well as a
> probabilistic failure).
> 
> > With new macro ERROR_INJECT_DELAYED it is possible to delay error
> > injection by DELAY parameter: injection will be set only after DELAY
> > times the path is executed. For instance:
> > 
> > void
> > foo(int i)
> > {
> > 	/* 2 is delay counter. */
> > 	ERROR_INJECT_DELAYED(ERRINJ_FOO, 2, {
> > 		 printf("Error injection on %d cycle!\n", i);
> > 		});
> > }
> > 
> > void
> > boo(void)
> > {
> > 	for (int i = 0; i < 10; ++i)
> > 		foo(i);
> > }
> > 
> > The result is "Error injection on 2 cycle!". This type of error
> > injection can turn out to be useful to set injection in the middle of
> > query processing. Imagine following scenario:
> > 
> > void
> > foo(void)
> > {
> > 	int *fds[10];
> > 	for (int i = 0; i < 10; ++i) {
> > 		fds[i] = malloc(sizeof(int));
> > 		if (fds[i] == NULL)
> > 			goto cleanup;
> > 	}
> > cleanup:
> > 	free(fds[0]);
> > }
> > 
> > "cleanup" section obviously contains error and leads to memory leak.
> > But using means of casual error injection without delay such situation
> > can't be detected: OOM can be set only for first cycle iteration and in
> > this particular case no leaks take place.
> > ---
> >  src/errinj.h | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/src/errinj.h b/src/errinj.h
> > index 2cb090b68..990f4921d 100644
> > --- a/src/errinj.h
> > +++ b/src/errinj.h
> > @@ -149,6 +149,7 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >  #ifdef NDEBUG
> >  #  define ERROR_INJECT(ID, CODE)
> >  #  define errinj(ID, TYPE) ((struct errinj *) NULL)
> > +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE)
> >  #else
> >  #  /* Returns the error injection by id */
> >  #  define errinj(ID, TYPE) \
> > @@ -162,6 +163,14 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >  		if (errinj(ID, ERRINJ_BOOL)->bparam) \
> >  			CODE; \
> >  	} while (0)
> > +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
> > +	do { \
> > +		static int _DELAY##ID = DELAY; \
> > +		if (errinj(ID, ERRINJ_BOOL)->bparam) { \
> > +			if (_DELAY##ID-- == 0) \
> > +				CODE; \
> > +		} \
> > +	} while (0)
> >  #endif
> >  
> >  #define ERROR_INJECT_RETURN(ID) ERROR_INJECT(ID, return -1)
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Konstantin Osipov, Moscow, Russia
> https://scylladb.com

Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/04/30 23:58]:
> On 30 Apr 23:15, Konstantin Osipov wrote:
> > * Nikita Pettik <korablev@tarantool.org> [20/04/30 22:51]:
> > 
> > This is not exactly a time delay, this is a skip-first-n kind of
> > postponed enable?
> 
> Yep, it's more accurate definition.  I can rename to
> ERROR_INJECT_POSPONED, if it fits better.

postponed

ERROR_INJECT_NTH
ERROR_INJECT_COUNTDOWN


-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com

Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Vladislav Shpilevoy]

Thanks for the patch!

> diff --git a/src/errinj.h b/src/errinj.h
> index 2cb090b68..990f4921d 100644
> --- a/src/errinj.h
> +++ b/src/errinj.h
> @@ -149,6 +149,7 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
>  #ifdef NDEBUG
>  #  define ERROR_INJECT(ID, CODE)
>  #  define errinj(ID, TYPE) ((struct errinj *) NULL)
> +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE)
>  #else
>  #  /* Returns the error injection by id */
>  #  define errinj(ID, TYPE) \
> @@ -162,6 +163,14 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
>  		if (errinj(ID, ERRINJ_BOOL)->bparam) \
>  			CODE; \
>  	} while (0)
> +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
> +	do { \
> +		static int _DELAY##ID = DELAY; \
> +		if (errinj(ID, ERRINJ_BOOL)->bparam) { \
> +			if (_DELAY##ID-- == 0) \
> +				CODE; \
> +		} \
> +	} while (0)

The method is fine unless you will ever want to set the same
error injection twice without restarting Tarantool. With this
solution there is no way to reset _DELAY##ID back to the same or
a different value. It will stay 0 until restart. This a serious
enough problem to reconsider the approach. There are injections
used in multiple tests, and we can't count on restart after each
one.

This is the reason why you won't be able to use this delayed
injection for VY_STMT_ALLOC in 'uninitialized memory' patchset.

'Delayed' easily can be implemented by iparam. For example, look
at ERRINJ_WAL_FALLOCATE and your own VY_STMT_ALLOC. You set
iparam to some value, then you decrement it until 0, and inject
the error at this moment. Works perfectly fine, and without
introducing any new unrecoverable ERRINJ_ counters. However if you
want to simplify that, just wrap the iparam-method into a macros.
Even the same name could be fine - ERROR_INJECT_DELAYED.

Moreover, it simplifies check that the injection really happened,
and its progress. You just look at iparam in Lua and see how many
times it has executed, and whether it is equal to 0 already
(with bparam you can do the same though, by setting it to false
explicitly).

Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Nikita Pettik]

On 03 May 21:20, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/errinj.h b/src/errinj.h
> > index 2cb090b68..990f4921d 100644
> > --- a/src/errinj.h
> > +++ b/src/errinj.h
> > @@ -149,6 +149,7 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >  #ifdef NDEBUG
> >  #  define ERROR_INJECT(ID, CODE)
> >  #  define errinj(ID, TYPE) ((struct errinj *) NULL)
> > +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE)
> >  #else
> >  #  /* Returns the error injection by id */
> >  #  define errinj(ID, TYPE) \
> > @@ -162,6 +163,14 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >  		if (errinj(ID, ERRINJ_BOOL)->bparam) \
> >  			CODE; \
> >  	} while (0)
> > +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
> > +	do { \
> > +		static int _DELAY##ID = DELAY; \
> > +		if (errinj(ID, ERRINJ_BOOL)->bparam) { \
> > +			if (_DELAY##ID-- == 0) \
> > +				CODE; \
> > +		} \
> > +	} while (0)
> 
> The method is fine unless you will ever want to set the same
> error injection twice without restarting Tarantool. With this
> solution there is no way to reset _DELAY##ID back to the same or
> a different value. It will stay 0 until restart. This a serious
> enough problem to reconsider the approach. There are injections
> used in multiple tests, and we can't count on restart after each
> one.

Yep, this is obvious drawback of chosen implementation.
 
> This is the reason why you won't be able to use this delayed
> injection for VY_STMT_ALLOC in 'uninitialized memory' patchset.
> 
> 'Delayed' easily can be implemented by iparam. For example, look
> at ERRINJ_WAL_FALLOCATE and your own VY_STMT_ALLOC. You set
> iparam to some value, then you decrement it until 0, and inject
> the error at this moment. Works perfectly fine, and without
> introducing any new unrecoverable ERRINJ_ counters. However if you
> want to simplify that, just wrap the iparam-method into a macros.
> Even the same name could be fine - ERROR_INJECT_DELAYED.

Ok, here's diff:

diff --git a/src/errinj.h b/src/errinj.h
index 990f4921d..945abb939 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -163,12 +163,10 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
                if (errinj(ID, ERRINJ_BOOL)->bparam) \
                        CODE; \
        } while (0)
-#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
+#  define ERROR_INJECT_COUNTDOWN(ID, CODE) \
        do { \
-               static int _DELAY##ID = DELAY; \
-               if (errinj(ID, ERRINJ_BOOL)->bparam) { \
-                       if (_DELAY##ID-- == 0) \
-                               CODE; \
+               if (errinj(ID, ERRINJ_INT)->iparam-- == 0) { \
+                       CODE; \
                } \
        } while (0)
 #endif


> Moreover, it simplifies check that the injection really happened,
> and its progress. You just look at iparam in Lua and see how many
> times it has executed, and whether it is equal to 0 already
> (with bparam you can do the same though, by setting it to false
> explicitly).

Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

>>> diff --git a/src/errinj.h b/src/errinj.h
>>> index 2cb090b68..990f4921d 100644
>>> --- a/src/errinj.h
>>> +++ b/src/errinj.h
>>> @@ -149,6 +149,7 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
>>>  #ifdef NDEBUG
>>>  #  define ERROR_INJECT(ID, CODE)
>>>  #  define errinj(ID, TYPE) ((struct errinj *) NULL)
>>> +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE)
>>>  #else
>>>  #  /* Returns the error injection by id */
>>>  #  define errinj(ID, TYPE) \
>>> @@ -162,6 +163,14 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
>>>  		if (errinj(ID, ERRINJ_BOOL)->bparam) \
>>>  			CODE; \
>>>  	} while (0)
>>> +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
>>> +	do { \
>>> +		static int _DELAY##ID = DELAY; \
>>> +		if (errinj(ID, ERRINJ_BOOL)->bparam) { \
>>> +			if (_DELAY##ID-- == 0) \
>>> +				CODE; \
>>> +		} \
>>> +	} while (0)
>>
>> The method is fine unless you will ever want to set the same
>> error injection twice without restarting Tarantool. With this
>> solution there is no way to reset _DELAY##ID back to the same or
>> a different value. It will stay 0 until restart. This a serious
>> enough problem to reconsider the approach. There are injections
>> used in multiple tests, and we can't count on restart after each
>> one.
> 
> Yep, this is obvious drawback of chosen implementation.
>  
>> This is the reason why you won't be able to use this delayed
>> injection for VY_STMT_ALLOC in 'uninitialized memory' patchset.
>>
>> 'Delayed' easily can be implemented by iparam. For example, look
>> at ERRINJ_WAL_FALLOCATE and your own VY_STMT_ALLOC. You set
>> iparam to some value, then you decrement it until 0, and inject
>> the error at this moment. Works perfectly fine, and without
>> introducing any new unrecoverable ERRINJ_ counters. However if you
>> want to simplify that, just wrap the iparam-method into a macros.
>> Even the same name could be fine - ERROR_INJECT_DELAYED.
> 
> Ok, here's diff:
> 
> diff --git a/src/errinj.h b/src/errinj.h
> index 990f4921d..945abb939 100644
> --- a/src/errinj.h
> +++ b/src/errinj.h
> @@ -163,12 +163,10 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
>                 if (errinj(ID, ERRINJ_BOOL)->bparam) \
>                         CODE; \
>         } while (0)
> -#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
> +#  define ERROR_INJECT_COUNTDOWN(ID, CODE) \
>         do { \
> -               static int _DELAY##ID = DELAY; \
> -               if (errinj(ID, ERRINJ_BOOL)->bparam) { \
> -                       if (_DELAY##ID-- == 0) \
> -                               CODE; \
> +               if (errinj(ID, ERRINJ_INT)->iparam-- == 0) { \
> +                       CODE; \
>                 } \
>         } while (0)
>  #endif

One last thing - could you please align '\' to the end of the lines?
By 80 symbols, using tabs. Recently we had a discussion with Cyrill G,
and he mentioned such not aligned things are hard to read. Physically.
Eyes literally are getting tired faster.

He also wants us to align struct members everywhere, but that is a
different topic to discuss.

Re: [Tarantool-patches] [PATCH 1/2] errinj: introduce delayed injection

[Nikita Pettik]

On 07 May 23:47, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> >>> diff --git a/src/errinj.h b/src/errinj.h
> >>> index 2cb090b68..990f4921d 100644
> >>> --- a/src/errinj.h
> >>> +++ b/src/errinj.h
> >>> @@ -149,6 +149,7 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >>>  #ifdef NDEBUG
> >>>  #  define ERROR_INJECT(ID, CODE)
> >>>  #  define errinj(ID, TYPE) ((struct errinj *) NULL)
> >>> +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE)
> >>>  #else
> >>>  #  /* Returns the error injection by id */
> >>>  #  define errinj(ID, TYPE) \
> >>> @@ -162,6 +163,14 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >>>  		if (errinj(ID, ERRINJ_BOOL)->bparam) \
> >>>  			CODE; \
> >>>  	} while (0)
> >>> +#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
> >>> +	do { \
> >>> +		static int _DELAY##ID = DELAY; \
> >>> +		if (errinj(ID, ERRINJ_BOOL)->bparam) { \
> >>> +			if (_DELAY##ID-- == 0) \
> >>> +				CODE; \
> >>> +		} \
> >>> +	} while (0)
> >>
> >> The method is fine unless you will ever want to set the same
> >> error injection twice without restarting Tarantool. With this
> >> solution there is no way to reset _DELAY##ID back to the same or
> >> a different value. It will stay 0 until restart. This a serious
> >> enough problem to reconsider the approach. There are injections
> >> used in multiple tests, and we can't count on restart after each
> >> one.
> > 
> > Yep, this is obvious drawback of chosen implementation.
> >  
> >> This is the reason why you won't be able to use this delayed
> >> injection for VY_STMT_ALLOC in 'uninitialized memory' patchset.
> >>
> >> 'Delayed' easily can be implemented by iparam. For example, look
> >> at ERRINJ_WAL_FALLOCATE and your own VY_STMT_ALLOC. You set
> >> iparam to some value, then you decrement it until 0, and inject
> >> the error at this moment. Works perfectly fine, and without
> >> introducing any new unrecoverable ERRINJ_ counters. However if you
> >> want to simplify that, just wrap the iparam-method into a macros.
> >> Even the same name could be fine - ERROR_INJECT_DELAYED.
> > 
> > Ok, here's diff:
> > 
> > diff --git a/src/errinj.h b/src/errinj.h
> > index 990f4921d..945abb939 100644
> > --- a/src/errinj.h
> > +++ b/src/errinj.h
> > @@ -163,12 +163,10 @@ errinj_foreach(errinj_cb cb, void *cb_ctx);
> >                 if (errinj(ID, ERRINJ_BOOL)->bparam) \
> >                         CODE; \
> >         } while (0)
> > -#  define ERROR_INJECT_DELAYED(ID, DELAY, CODE) \
> > +#  define ERROR_INJECT_COUNTDOWN(ID, CODE) \
> >         do { \
> > -               static int _DELAY##ID = DELAY; \
> > -               if (errinj(ID, ERRINJ_BOOL)->bparam) { \
> > -                       if (_DELAY##ID-- == 0) \
> > -                               CODE; \
> > +               if (errinj(ID, ERRINJ_INT)->iparam-- == 0) { \
> > +                       CODE; \
> >                 } \
> >         } while (0)
> >  #endif
> 
> One last thing - could you please align '\' to the end of the lines?
> By 80 symbols, using tabs. Recently we had a discussion with Cyrill G,

My editor doesn't provide an ability to align by custom offset using
tabs, so I aligned \ by 74 symbols. Feel free to force push fix if
it still looks wrong.

> and he mentioned such not aligned things are hard to read. Physically.
> Eyes literally are getting tired faster.
> 
> He also wants us to align struct members everywhere, but that is a
> different topic to discuss.

[Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Nikita Pettik]

If recovery process fails during range restoration, range itself is
deleted and recovery is assumed to be finished as failed (in case of
casual i.e. not forced recovery). During recovery of particular range,
runs to be restored are reffed twice: once when they are created at
vy_run_new() and once when they are attached to slice. This fact is
taken into consideration and after all ranges are recovered all runs of
lsm tree are unreffed so that slices own run resources. However, if
range recovery fails, it is dropped alongside with slices which in turn
results in unreffing runs - this is not accounted. In this case, once
again unreffing such runs would lead to their destruction. On the other
hand, iteration over runs may turn out to be unsafe, so we should use
rlist_foreach_entry_safe(). Moreover, we should explicitly unaccount
these runs calling vy_lsm_remove_run().

Closes #4805
---
 src/box/vy_lsm.c                              |  14 ++-
 src/box/vy_run.c                              |   4 +
 src/errinj.h                                  |   1 +
 test/box/errinj.result                        |   1 +
 test/vinyl/errinj_recovery.lua                |  10 ++
 .../gh-4805-open-run-err-recovery.result      | 101 ++++++++++++++++++
 .../gh-4805-open-run-err-recovery.test.lua    |  38 +++++++
 test/vinyl/suite.ini                          |   2 +-
 8 files changed, 167 insertions(+), 4 deletions(-)
 create mode 100644 test/vinyl/errinj_recovery.lua
 create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.result
 create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.test.lua

diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 3d3f41b7a..81b011c69 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -604,9 +604,17 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
 	 * of each recovered run. We need to drop the extra
 	 * references once we are done.
 	 */
-	struct vy_run *run;
-	rlist_foreach_entry(run, &lsm->runs, in_lsm) {
-		assert(run->refs > 1);
+	struct vy_run *run, *next_run;
+	rlist_foreach_entry_safe(run, &lsm->runs, in_lsm, next_run) {
+		/*
+		 * In case vy_lsm_recover_range() failed, slices
+		 * are already deleted and runs are unreffed. So
+		 * we have nothing to do but finish run clean-up.
+		 */
+		if (run->refs == 1) {
+			assert(rc != 0);
+			vy_lsm_remove_run(lsm, run);
+		}
 		vy_run_unref(run);
 	}
 
diff --git a/src/box/vy_run.c b/src/box/vy_run.c
index bd8a7a430..eeaeb88b4 100644
--- a/src/box/vy_run.c
+++ b/src/box/vy_run.c
@@ -1676,6 +1676,10 @@ vy_run_recover(struct vy_run *run, const char *dir,
 			    space_id, iid, run->id, VY_FILE_INDEX);
 
 	struct xlog_cursor cursor;
+	ERROR_INJECT_DELAYED(ERRINJ_VY_RUN_OPEN, 2, {
+		diag_set(SystemError, "failed to open '%s' file", path);
+		goto fail;
+	});
 	if (xlog_cursor_open(&cursor, path))
 		goto fail;
 
diff --git a/src/errinj.h b/src/errinj.h
index 990f4921d..273458d59 100644
--- a/src/errinj.h
+++ b/src/errinj.h
@@ -127,6 +127,7 @@ struct errinj {
 	_(ERRINJ_VY_COMPACTION_DELAY, ERRINJ_BOOL, {.bparam = false}) \
 	_(ERRINJ_DYN_MODULE_COUNT, ERRINJ_INT, {.iparam = 0}) \
 	_(ERRINJ_INDEX_RESERVE, ERRINJ_BOOL, {.bparam = false})\
+	_(ERRINJ_VY_RUN_OPEN, ERRINJ_BOOL, {.bparam = false})\
 
 ENUM0(errinj_id, ERRINJ_LIST);
 extern struct errinj errinjs[];
diff --git a/test/box/errinj.result b/test/box/errinj.result
index 8090deedc..785f6394b 100644
--- a/test/box/errinj.result
+++ b/test/box/errinj.result
@@ -70,6 +70,7 @@ evals
   - ERRINJ_VY_READ_PAGE_TIMEOUT: 0
   - ERRINJ_VY_RUN_DISCARD: false
   - ERRINJ_VY_RUN_FILE_RENAME: false
+  - ERRINJ_VY_RUN_OPEN: false
   - ERRINJ_VY_RUN_WRITE: false
   - ERRINJ_VY_RUN_WRITE_DELAY: false
   - ERRINJ_VY_RUN_WRITE_STMT_TIMEOUT: 0
diff --git a/test/vinyl/errinj_recovery.lua b/test/vinyl/errinj_recovery.lua
new file mode 100644
index 000000000..925d12a85
--- /dev/null
+++ b/test/vinyl/errinj_recovery.lua
@@ -0,0 +1,10 @@
+#!/usr/bin/env tarantool
+
+box.error.injection.set('ERRINJ_VY_RUN_OPEN', true)
+assert(box.error.injection.get('ERRINJ_VY_RUN_OPEN'))
+
+box.cfg {
+    listen = os.getenv("LISTEN"),
+}
+
+require('console').listen(os.getenv('ADMIN'))
diff --git a/test/vinyl/gh-4805-open-run-err-recovery.result b/test/vinyl/gh-4805-open-run-err-recovery.result
new file mode 100644
index 000000000..31e0e7857
--- /dev/null
+++ b/test/vinyl/gh-4805-open-run-err-recovery.result
@@ -0,0 +1,101 @@
+-- test-run result file version 2
+fiber = require('fiber')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+test_run:cmd('create server err_recovery with script = "vinyl/errinj_recovery.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server err_recovery')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch err_recovery')
+ | ---
+ | - true
+ | ...
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+ | ---
+ | ...
+
+digest = require('digest')
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function dump(big)
+    local step = big and 1 or 5
+    for i = 1, 20, step do
+        s:replace{i, digest.urandom(1000)}
+    end
+    box.snapshot()
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+dump(true)
+ | ---
+ | ...
+dump()
+ | ---
+ | ...
+dump()
+ | ---
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server err_recovery')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server err_recovery with crash_expected=True')
+ | ---
+ | - false
+ | ...
+
+fio = require('fio')
+ | ---
+ | ...
+fh = fio.open(fio.pathjoin(fio.cwd(), 'errinj_recovery.log'), {'O_RDONLY'})
+ | ---
+ | ...
+size = fh:seek(0, 'SEEK_END')
+ | ---
+ | ...
+fh:seek(-256, 'SEEK_END') ~= nil
+ | ---
+ | - true
+ | ...
+line = fh:read(256)
+ | ---
+ | ...
+fh:close()
+ | ---
+ | - true
+ | ...
+string.match(line, 'failed to open') ~= nil
+ | ---
+ | - true
+ | ...
+
+test_run:cmd('delete server err_recovery')
+ | ---
+ | - true
+ | ...
diff --git a/test/vinyl/gh-4805-open-run-err-recovery.test.lua b/test/vinyl/gh-4805-open-run-err-recovery.test.lua
new file mode 100644
index 000000000..8b5895d41
--- /dev/null
+++ b/test/vinyl/gh-4805-open-run-err-recovery.test.lua
@@ -0,0 +1,38 @@
+fiber = require('fiber')
+test_run = require('test_run').new()
+
+test_run:cmd('create server err_recovery with script = "vinyl/errinj_recovery.lua"')
+test_run:cmd('start server err_recovery')
+test_run:cmd('switch err_recovery')
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
+
+digest = require('digest')
+test_run:cmd("setopt delimiter ';'")
+function dump(big)
+    local step = big and 1 or 5
+    for i = 1, 20, step do
+        s:replace{i, digest.urandom(1000)}
+    end
+    box.snapshot()
+end;
+test_run:cmd("setopt delimiter ''");
+
+dump(true)
+dump()
+dump()
+
+test_run:cmd('switch default')
+test_run:cmd('stop server err_recovery')
+test_run:cmd('start server err_recovery with crash_expected=True')
+
+fio = require('fio')
+fh = fio.open(fio.pathjoin(fio.cwd(), 'errinj_recovery.log'), {'O_RDONLY'})
+size = fh:seek(0, 'SEEK_END')
+fh:seek(-256, 'SEEK_END') ~= nil
+line = fh:read(256)
+fh:close()
+string.match(line, 'failed to open') ~= nil
+
+test_run:cmd('delete server err_recovery')
diff --git a/test/vinyl/suite.ini b/test/vinyl/suite.ini
index 1417d7156..ce02eb83d 100644
--- a/test/vinyl/suite.ini
+++ b/test/vinyl/suite.ini
@@ -2,7 +2,7 @@
 core = tarantool
 description = vinyl integration tests
 script = vinyl.lua
-release_disabled = errinj.test.lua errinj_ddl.test.lua errinj_gc.test.lua errinj_stat.test.lua errinj_tx.test.lua errinj_vylog.test.lua partial_dump.test.lua quota_timeout.test.lua recovery_quota.test.lua replica_rejoin.test.lua
+release_disabled = errinj.test.lua errinj_ddl.test.lua errinj_gc.test.lua errinj_stat.test.lua errinj_tx.test.lua errinj_vylog.test.lua partial_dump.test.lua quota_timeout.test.lua recovery_quota.test.lua replica_rejoin.test.lua gh-4805-open-run-err-recovery.test.lua
 config = suite.cfg
 lua_libs = suite.lua stress.lua large.lua txn_proxy.lua ../box/lua/utils.lua
 use_unix_sockets = True
-- 
2.17.1

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Vladislav Shpilevoy]

Thanks for the patch!

On 30/04/2020 21:27, Nikita Pettik wrote:
> If recovery process fails during range restoration, range itself is
> deleted and recovery is assumed to be finished as failed (in case of
> casual i.e. not forced recovery). During recovery of particular range,
> runs to be restored are reffed twice: once when they are created at
> vy_run_new() and once when they are attached to slice. This fact is
> taken into consideration and after all ranges are recovered all runs of
> lsm tree are unreffed so that slices own run resources. However, if

Nit: 'unreffed' -> 'unrefed'. Since this is a shortcut for
'unreferenced', where number of 'f' is 1.

The same for 'reffed'.

> range recovery fails, it is dropped alongside with slices which in turn
> results in unreffing runs - this is not accounted. In this case, once
> again unreffing such runs would lead to their destruction. On the other
> hand, iteration over runs may turn out to be unsafe, so we should use
> rlist_foreach_entry_safe(). Moreover, we should explicitly unaccount
> these runs calling vy_lsm_remove_run().

Sorry, I didn't understand almost everything from the message :D
But I did from the code. You may want to rephrase/restruct the text
if you think it may help.

> diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> index 3d3f41b7a..81b011c69 100644
> --- a/src/box/vy_lsm.c
> +++ b/src/box/vy_lsm.c
> @@ -604,9 +604,17 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
>  	 * of each recovered run. We need to drop the extra
>  	 * references once we are done.
>  	 */
> -	struct vy_run *run;
> -	rlist_foreach_entry(run, &lsm->runs, in_lsm) {
> -		assert(run->refs > 1);
> +	struct vy_run *run, *next_run;
> +	rlist_foreach_entry_safe(run, &lsm->runs, in_lsm, next_run) {
> +		/*
> +		 * In case vy_lsm_recover_range() failed, slices
> +		 * are already deleted and runs are unreffed. So
> +		 * we have nothing to do but finish run clean-up.
> +		 */
> +		if (run->refs == 1) {

Reference counter looks like not a good information channel.
Could you use run->fd to check whether the run was really recovered?
vy_run_recover() leaves it -1, when fails.

Otherwise this won't work the second when we will ref the run anywhere
else.

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Nikita Pettik]

On 03 May 21:21, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 30/04/2020 21:27, Nikita Pettik wrote:
> > If recovery process fails during range restoration, range itself is
> > deleted and recovery is assumed to be finished as failed (in case of
> > casual i.e. not forced recovery). During recovery of particular range,
> > runs to be restored are reffed twice: once when they are created at
> > vy_run_new() and once when they are attached to slice. This fact is
> > taken into consideration and after all ranges are recovered all runs of
> > lsm tree are unreffed so that slices own run resources. However, if
> 
> Nit: 'unreffed' -> 'unrefed'. Since this is a shortcut for
> 'unreferenced', where number of 'f' is 1.
> 
> The same for 'reffed'.

Fixed.
 
> > range recovery fails, it is dropped alongside with slices which in turn
> > results in unreffing runs - this is not accounted. In this case, once
> > again unreffing such runs would lead to their destruction. On the other
> > hand, iteration over runs may turn out to be unsafe, so we should use
> > rlist_foreach_entry_safe(). Moreover, we should explicitly unaccount
> > these runs calling vy_lsm_remove_run().
> 
> Sorry, I didn't understand almost everything from the message :D
> But I did from the code. You may want to rephrase/restruct the text
> if you think it may help.

A bit reworked commit message and provided a brief schema in pseudocode.
Hope this will help:

    vinyl: drop wasted runs in case range recovery fails
    
    If recovery process fails during range restoration, range itself is
    deleted and recovery is assumed to be finished as failed (in case of
    casual i.e. not forced recovery). During recovery of particular range,
    runs to be restored are refed twice: once when they are created at
    vy_run_new() and once when they are attached to slice. This fact is
    taken into consideration and after all ranges are recovered: all runs of
    lsm tree are unrefed so that slices own run resources (as a result, when
    slice is to be deleted its runs unrefed and deleted as well). However, if
    range recovery fails, range is dropped alongside with already recovered
    slices. This leads to unrefing runs - this is not accounted. To sum up
    recovery process below is a brief schema:
    
    foreach range in lsm.ranges {
      vy_lsm_recover_range(range) {
        foreach slice in range.slices {
          // inside recover_slice() each run is refed twice
          if vy_lsm_recover_slice() != 0 {
            // here all already restored slices are deleted and
            // corresponding runs are unrefed, so now they have 1 ref.
            range_delete()
          }
        }
      }
    }
    foreach run in lsm.runs {
      assert(run->refs > 1)
      vy_run_unref(run)
    }
    
    In this case, unrefing such runs one more time would lead to their
    destruction. On the other hand, iteration over runs may turn out to
    be unsafe, so we should use rlist_foreach_entry_safe(). Moreover, we
    should explicitly clean-up these runs calling vy_lsm_remove_run().
    
    Closes #4805


> > diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> > index 3d3f41b7a..81b011c69 100644
> > --- a/src/box/vy_lsm.c
> > +++ b/src/box/vy_lsm.c
> > @@ -604,9 +604,17 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
> >  	 * of each recovered run. We need to drop the extra
> >  	 * references once we are done.
> >  	 */
> > -	struct vy_run *run;
> > -	rlist_foreach_entry(run, &lsm->runs, in_lsm) {
> > -		assert(run->refs > 1);
> > +	struct vy_run *run, *next_run;
> > +	rlist_foreach_entry_safe(run, &lsm->runs, in_lsm, next_run) {
> > +		/*
> > +		 * In case vy_lsm_recover_range() failed, slices
> > +		 * are already deleted and runs are unreffed. So
> > +		 * we have nothing to do but finish run clean-up.
> > +		 */
> > +		if (run->refs == 1) {
> 
> Reference counter looks like not a good information channel.
> Could you use run->fd to check whether the run was really recovered?
> vy_run_recover() leaves it -1, when fails.
> 
> Otherwise this won't work the second when we will ref the run anywhere
> else.

Firstly, lsm at this point is not restored, ergo it is not functional
and run can't be refed somewehere else - it's life span is clearly
defined. Secondly, the problem is not in the last run (which failed to
recover) but in those which are already recovered at the moment.
Recovered runs feature valid fds. Finally, slice recover may fail
not only in vy_run_recover(), but also due to oom, broken vylog etc.
All these scenarios lead to the same situation.

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [20/05/07 16:05]:
> > 
> > Reference counter looks like not a good information channel.
> > Could you use run->fd to check whether the run was really recovered?
> > vy_run_recover() leaves it -1, when fails.
> > 
> > Otherwise this won't work the second when we will ref the run anywhere
> > else.
> 
> Firstly, lsm at this point is not restored, ergo it is not functional
> and run can't be refed somewehere else - it's life span is clearly
> defined. Secondly, the problem is not in the last run (which failed to
> recover) but in those which are already recovered at the moment.
> Recovered runs feature valid fds. Finally, slice recover may fail
> not only in vy_run_recover(), but also due to oom, broken vylog etc.
> All these scenarios lead to the same situation.

It should be partially restored in case of force_recovery. It's
another bug force_recovery is not respected, I've been sending a
fix to the list a few months ago.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Vladislav Shpilevoy]

On 07/05/2020 16:16, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [20/05/07 16:05]:
>>>
>>> Reference counter looks like not a good information channel.
>>> Could you use run->fd to check whether the run was really recovered?
>>> vy_run_recover() leaves it -1, when fails.
>>>
>>> Otherwise this won't work the second when we will ref the run anywhere
>>> else.
>>
>> Firstly, lsm at this point is not restored, ergo it is not functional
>> and run can't be refed somewehere else - it's life span is clearly
>> defined. Secondly, the problem is not in the last run (which failed to
>> recover) but in those which are already recovered at the moment.
>> Recovered runs feature valid fds. Finally, slice recover may fail
>> not only in vy_run_recover(), but also due to oom, broken vylog etc.
>> All these scenarios lead to the same situation.
> 
> It should be partially restored in case of force_recovery. It's
> another bug force_recovery is not respected, I've been sending a
> fix to the list a few months ago.

Is there a test case and an issue on that? Nikita, could you please
find it/file it/confirm it/reject it?

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Nikita Pettik]

On 07 May 23:47, Vladislav Shpilevoy wrote:
> On 07/05/2020 16:16, Konstantin Osipov wrote:
> > * Nikita Pettik <korablev@tarantool.org> [20/05/07 16:05]:
> >>>
> >>> Reference counter looks like not a good information channel.
> >>> Could you use run->fd to check whether the run was really recovered?
> >>> vy_run_recover() leaves it -1, when fails.
> >>>
> >>> Otherwise this won't work the second when we will ref the run anywhere
> >>> else.
> >>
> >> Firstly, lsm at this point is not restored, ergo it is not functional
> >> and run can't be refed somewehere else - it's life span is clearly
> >> defined. Secondly, the problem is not in the last run (which failed to
> >> recover) but in those which are already recovered at the moment.
> >> Recovered runs feature valid fds. Finally, slice recover may fail
> >> not only in vy_run_recover(), but also due to oom, broken vylog etc.
> >> All these scenarios lead to the same situation.
> > 
> > It should be partially restored in case of force_recovery. It's
> > another bug force_recovery is not respected, I've been sending a
> > fix to the list a few months ago.
> 
> Is there a test case and an issue on that? Nikita, could you please
> find it/file it/confirm it/reject it?

I've found this issue:
https://github.com/tarantool/tarantool/issues/4474

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Vladislav Shpilevoy]

Thanks for the explanation and the new commit message!

>>> diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
>>> index 3d3f41b7a..81b011c69 100644
>>> --- a/src/box/vy_lsm.c
>>> +++ b/src/box/vy_lsm.c
>>> @@ -604,9 +604,17 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
>>>  	 * of each recovered run. We need to drop the extra
>>>  	 * references once we are done.
>>>  	 */
>>> -	struct vy_run *run;
>>> -	rlist_foreach_entry(run, &lsm->runs, in_lsm) {
>>> -		assert(run->refs > 1);
>>> +	struct vy_run *run, *next_run;
>>> +	rlist_foreach_entry_safe(run, &lsm->runs, in_lsm, next_run) {
>>> +		/*
>>> +		 * In case vy_lsm_recover_range() failed, slices
>>> +		 * are already deleted and runs are unreffed. So
>>> +		 * we have nothing to do but finish run clean-up.
>>> +		 */
>>> +		if (run->refs == 1) {
>>
>> Reference counter looks like not a good information channel.
>> Could you use run->fd to check whether the run was really recovered?
>> vy_run_recover() leaves it -1, when fails.
>>
>> Otherwise this won't work the second when we will ref the run anywhere
>> else.
> 
> Firstly, lsm at this point is not restored, ergo it is not functional
> and run can't be refed somewehere else - it's life span is clearly
> defined. Secondly, the problem is not in the last run (which failed to
> recover) but in those which are already recovered at the moment.
> Recovered runs feature valid fds. Finally, slice recover may fail
> not only in vy_run_recover(), but also due to oom, broken vylog etc.
> All these scenarios lead to the same situation.

Yeah, fair. Then what about run->slice_count? If it is zero, then it
is not kept by any slice. So we can look at slice_count == 0 and
assert ref == 1. Or look at ref == 1, and assert slice_count == 0.
Whatever. Will that work?

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Nikita Pettik]

On 07 May 23:47, Vladislav Shpilevoy wrote:
> Thanks for the explanation and the new commit message!
> 
> >>> diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
> >>> index 3d3f41b7a..81b011c69 100644
> >>> --- a/src/box/vy_lsm.c
> >>> +++ b/src/box/vy_lsm.c
> >>> @@ -604,9 +604,17 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
> >>>  	 * of each recovered run. We need to drop the extra
> >>>  	 * references once we are done.
> >>>  	 */
> >>> -	struct vy_run *run;
> >>> -	rlist_foreach_entry(run, &lsm->runs, in_lsm) {
> >>> -		assert(run->refs > 1);
> >>> +	struct vy_run *run, *next_run;
> >>> +	rlist_foreach_entry_safe(run, &lsm->runs, in_lsm, next_run) {
> >>> +		/*
> >>> +		 * In case vy_lsm_recover_range() failed, slices
> >>> +		 * are already deleted and runs are unreffed. So
> >>> +		 * we have nothing to do but finish run clean-up.
> >>> +		 */
> >>> +		if (run->refs == 1) {
> >>
> >> Reference counter looks like not a good information channel.
> >> Could you use run->fd to check whether the run was really recovered?
> >> vy_run_recover() leaves it -1, when fails.
> >>
> >> Otherwise this won't work the second when we will ref the run anywhere
> >> else.
> > 
> > Firstly, lsm at this point is not restored, ergo it is not functional
> > and run can't be refed somewehere else - it's life span is clearly
> > defined. Secondly, the problem is not in the last run (which failed to
> > recover) but in those which are already recovered at the moment.
> > Recovered runs feature valid fds. Finally, slice recover may fail
> > not only in vy_run_recover(), but also due to oom, broken vylog etc.
> > All these scenarios lead to the same situation.
> 
> Yeah, fair. Then what about run->slice_count? If it is zero, then it
> is not kept by any slice. So we can look at slice_count == 0 and
> assert ref == 1. Or look at ref == 1, and assert slice_count == 0.
> Whatever. Will that work?

diff --git a/src/box/vy_lsm.c b/src/box/vy_lsm.c
index 7755f04f0..005dde3b2 100644
--- a/src/box/vy_lsm.c
+++ b/src/box/vy_lsm.c
@@ -613,6 +613,7 @@ vy_lsm_recover(struct vy_lsm *lsm, struct vy_recovery *recovery,
                 */
                if (run->refs == 1) {
                        assert(rc != 0);
+                       assert(run->slice_count == 0);
                        vy_lsm_remove_run(lsm, run);
                }
                vy_run_unref(run);

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

See 1 comment below.

> diff --git a/test/vinyl/gh-4805-open-run-err-recovery.result b/test/vinyl/gh-4805-open-run-err-recovery.result
> new file mode 100644
> index 000000000..31e0e7857
> --- /dev/null
> +++ b/test/vinyl/gh-4805-open-run-err-recovery.result
> @@ -0,0 +1,101 @@
> +-- test-run result file version 2
> +fiber = require('fiber')
> + | ---
> + | ...
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +
> +test_run:cmd('create server err_recovery with script = "vinyl/errinj_recovery.lua"')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server err_recovery')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('switch err_recovery')
> + | ---
> + | - true
> + | ...
> +
> +s = box.schema.space.create('test', {engine = 'vinyl'})
> + | ---
> + | ...
> +_ = s:create_index('pk', {run_count_per_level = 100, page_size = 128, range_size = 1024})
> + | ---
> + | ...
> +
> +digest = require('digest')
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ';'")
> + | ---
> + | - true
> + | ...
> +function dump(big)
> +    local step = big and 1 or 5
> +    for i = 1, 20, step do
> +        s:replace{i, digest.urandom(1000)}
> +    end
> +    box.snapshot()
> +end;
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | - true
> + | ...
> +
> +dump(true)
> + | ---
> + | ...
> +dump()
> + | ---
> + | ...
> +dump()
> + | ---
> + | ...
> +
> +test_run:cmd('switch default')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('stop server err_recovery')
> + | ---
> + | - true
> + | ...
> +test_run:cmd('start server err_recovery with crash_expected=True')
> + | ---
> + | - false
> + | ...
> +
> +fio = require('fio')
> + | ---
> + | ...
> +fh = fio.open(fio.pathjoin(fio.cwd(), 'errinj_recovery.log'), {'O_RDONLY'})
> + | ---
> + | ...
> +size = fh:seek(0, 'SEEK_END')
> + | ---
> + | ...
> +fh:seek(-256, 'SEEK_END') ~= nil
> + | ---
> + | - true
> + | ...
> +line = fh:read(256)
> + | ---
> + | ...
> +fh:close()
> + | ---
> + | - true
> + | ...
> +string.match(line, 'failed to open') ~= nil

Why can't you use test_run:grep_log()? It supports
searching in logs of an already crashed instance too.
See its parameters in test_run.lua. Looks like it can
solve your task.

Re: [Tarantool-patches] [PATCH 2/2] vinyl: drop wasted runs in case range recovery fails

[Nikita Pettik]

On 10 May 21:59, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> See 1 comment below.
> 
> > +line = fh:read(256)
> > + | ---
> > + | ...
> > +fh:close()
> > + | ---
> > + | - true
> > + | ...
> > +string.match(line, 'failed to open') ~= nil
> 
> Why can't you use test_run:grep_log()? It supports
> searching in logs of an already crashed instance too.
> See its parameters in test_run.lua. Looks like it can
> solve your task.
>

Indeed. Here's diff:

diff --git a/test/vinyl/gh-4805-open-run-err-recovery.test.lua b/test/vinyl/gh-4805-open-run-err-recovery.test.lua
index 8b5895d41..0b97f09da 100644
--- a/test/vinyl/gh-4805-open-run-err-recovery.test.lua
+++ b/test/vinyl/gh-4805-open-run-err-recovery.test.lua
@@ -27,12 +27,8 @@ test_run:cmd('switch default')
 test_run:cmd('stop server err_recovery')
 test_run:cmd('start server err_recovery with crash_expected=True')
 
-fio = require('fio')
-fh = fio.open(fio.pathjoin(fio.cwd(), 'errinj_recovery.log'), {'O_RDONLY'})
-size = fh:seek(0, 'SEEK_END')
-fh:seek(-256, 'SEEK_END') ~= nil
-line = fh:read(256)
-fh:close()
-string.match(line, 'failed to open') ~= nil
+opts = {}
+opts.filename = 'errinj_recovery.log'
+test_run:grep_log('err_recovery', 'failed to open', 1000, opts) ~= nil
 
 test_run:cmd('delete server err_recovery')
 

Re: [Tarantool-patches] [PATCH 0/2] Fix crash in case of lack of FDs during recovery

[Vladislav Shpilevoy]

Why do you base your patches on top of 1.10? Doesn't the
bug exist on master?

On 30/04/2020 21:27, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-4805-too-many-fds
> Issue: https://github.com/tarantool/tarantool/issues/4805
> 
> First patch adds simple macro which allows error injection to be delayed.
> It also can be used in this series:
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016367.html
> 
> Nikita Pettik (2):
>   errinj: introduce delayed injection
>   vinyl: drop wasted runs in case range recovery fails
> 
>  src/box/vy_lsm.c                              |  14 ++-
>  src/box/vy_run.c                              |   4 +
>  src/errinj.h                                  |  10 ++
>  test/box/errinj.result                        |   1 +
>  test/vinyl/errinj_recovery.lua                |  10 ++
>  .../gh-4805-open-run-err-recovery.result      | 101 ++++++++++++++++++
>  .../gh-4805-open-run-err-recovery.test.lua    |  38 +++++++
>  test/vinyl/suite.ini                          |   2 +-
>  8 files changed, 176 insertions(+), 4 deletions(-)
>  create mode 100644 test/vinyl/errinj_recovery.lua
>  create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.result
>  create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.test.lua
> 

Re: [Tarantool-patches] [PATCH 0/2] Fix crash in case of lack of FDs during recovery

[Nikita Pettik]

On 03 May 21:20, Vladislav Shpilevoy wrote:
> Why do you base your patches on top of 1.10? Doesn't the
> bug exist on master?

Yep, master is affected as well. Originally I found it on 1.10 so firstly
prepared patch for 1.10. There's no big difference in lsm recovery code, so
patch can be backported with ease (from master to 1.10). Will push to master
firstly, when patch-set is ready-to-push. Anyway, thanks for noting that.
 
> On 30/04/2020 21:27, Nikita Pettik wrote:
> > Branch: https://github.com/tarantool/tarantool/commits/np/gh-4805-too-many-fds
> > Issue: https://github.com/tarantool/tarantool/issues/4805
> > 
> > First patch adds simple macro which allows error injection to be delayed.
> > It also can be used in this series:
> > https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016367.html
> > 
> > Nikita Pettik (2):
> >   errinj: introduce delayed injection
> >   vinyl: drop wasted runs in case range recovery fails
> > 
> >  src/box/vy_lsm.c                              |  14 ++-
> >  src/box/vy_run.c                              |   4 +
> >  src/errinj.h                                  |  10 ++
> >  test/box/errinj.result                        |   1 +
> >  test/vinyl/errinj_recovery.lua                |  10 ++
> >  .../gh-4805-open-run-err-recovery.result      | 101 ++++++++++++++++++
> >  .../gh-4805-open-run-err-recovery.test.lua    |  38 +++++++
> >  test/vinyl/suite.ini                          |   2 +-
> >  8 files changed, 176 insertions(+), 4 deletions(-)
> >  create mode 100644 test/vinyl/errinj_recovery.lua
> >  create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.result
> >  create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.test.lua
> > 

Re: [Tarantool-patches] [PATCH 0/2] Fix crash in case of lack of FDs during recovery

[Vladislav Shpilevoy]

Hi! Thanks for the patchset and the fixes!

LGTM.

On 30/04/2020 21:27, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-4805-too-many-fds
> Issue: https://github.com/tarantool/tarantool/issues/4805
> 
> First patch adds simple macro which allows error injection to be delayed.
> It also can be used in this series:
> https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016367.html
> 
> Nikita Pettik (2):
>   errinj: introduce delayed injection
>   vinyl: drop wasted runs in case range recovery fails
> 
>  src/box/vy_lsm.c                              |  14 ++-
>  src/box/vy_run.c                              |   4 +
>  src/errinj.h                                  |  10 ++
>  test/box/errinj.result                        |   1 +
>  test/vinyl/errinj_recovery.lua                |  10 ++
>  .../gh-4805-open-run-err-recovery.result      | 101 ++++++++++++++++++
>  .../gh-4805-open-run-err-recovery.test.lua    |  38 +++++++
>  test/vinyl/suite.ini                          |   2 +-
>  8 files changed, 176 insertions(+), 4 deletions(-)
>  create mode 100644 test/vinyl/errinj_recovery.lua
>  create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.result
>  create mode 100644 test/vinyl/gh-4805-open-run-err-recovery.test.lua
> 

Re: [Tarantool-patches] [PATCH 0/2] Fix crash in case of lack of FDs during recovery

[Vladislav Shpilevoy]

Ah, yes, almost forgot:

Reviewed-by: Vladislav Shpilevoy <vshpilevoi@mail.ru> 😎

Re: [Tarantool-patches] [PATCH 0/2] Fix crash in case of lack of FDs during recovery

[Nikita Pettik]

On 12 May 22:56, Vladislav Shpilevoy wrote:
> Ah, yes, almost forgot:
> 
> Reviewed-by: Vladislav Shpilevoy <vshpilevoi@mail.ru> 😎

Is this official now? Is SOP already updated?

Pushed to master; backported to 2.4, 2.3 and 1.10. Changelogs are updated
correspondingly. Branch is dropped.

Re: [Tarantool-patches] [PATCH 0/2] Fix crash in case of lack of FDs during recovery

[Vladislav Shpilevoy]

On 13/05/2020 00:45, Nikita Pettik wrote:
> On 12 May 22:56, Vladislav Shpilevoy wrote:
>> Ah, yes, almost forgot:
>>
>> Reviewed-by: Vladislav Shpilevoy <vshpilevoi@mail.ru> 😎
> 
> Is this official now? Is SOP already updated?
> 
> Pushed to master; backported to 2.4, 2.3 and 1.10. Changelogs are updated
> correspondingly. Branch is dropped.

It is voluntary, from what I understood. I saw some people already
adding it. You can omit this if you want.