obj: handle ENOMEM during vector allocations #5537

bfaccini · 2023-02-13T13:53:22Z

When missing, Handle ENOMEM during vector allocations to avoid later crashing.

Ref: pmem/issues #5515
Signed-off-by: Bruno Faccini [email protected]

This change is

pbalcer · 2023-02-13T14:03:16Z

src/libpmemobj/tx.c

@@ -1013,8 +1019,16 @@ pmemobj_tx_commit(void)
 		operation_start(tx->lane->external);

 		struct user_buffer_def *userbuf;
-		VEC_FOREACH_BY_PTR(userbuf, &tx->redo_userbufs)
-			operation_add_user_buffer(tx->lane->external, userbuf);
+		struct operation_context *ctx = tx->lane->external;


Like I indicated earlier in the issue, I don't think we need to check for this error here. Instead, we simply need to ensure that the operation_add_user_buffer never fails (like you've already done in tx_construct_user_buffer).

To have the FATAL/ASSERT that you added you can change the operation_add_user_buffer to return an error (-1) if VEC_PUSH_BACK(&ctx->next, buffer_offset); fails. This would let you avoid making all the memops structs public.

not sure to fully follow your reasoning, but anyway I will try to push a new commit based on what I think I have understood...

pbalcer · 2023-02-13T14:04:06Z

src/libpmemobj/memops.h

@@ -39,7 +40,53 @@ struct user_buffer_def {
 	size_t size;
 };

-struct operation_context;
+enum operation_state {


It would be better if we kept the structs private and instead relied on some inter-module API if necessary.

pbalcer · 2023-02-15T13:35:00Z

src/libpmemobj/tx.c

@@ -1013,8 +1015,12 @@ pmemobj_tx_commit(void)
 		operation_start(tx->lane->external);

 		struct user_buffer_def *userbuf;
+		struct operation_context *ctx = tx->lane->external;
+		uint64_t buf_off;


buf_off is unusued.

Oops, sorry to have missed that

lukaszstolarczuk

Reviewable status: 0 of 5 files reviewed, 6 unresolved discussions (waiting on @bfaccini and @pbalcer)

-- commits line 20 at r3:
you can squash all the commits into a single one, please

src/libpmemobj/memops.h line 2 at r3 (raw file):

/* SPDX-License-Identifier: BSD-3-Clause */
/* Copyright 2016-2020, Intel Corporation */

Since you're using an Intel e-mail address for your commits, you have to update all dates to -2023.
E.g., in this file: 2016-2023

src/libpmemobj/tx.c line 714 at r3 (raw file):

			userbuf.size - TX_INTENT_LOG_BUFFER_OVERHEAD;
	} else {
		if (operation_add_user_buffer(ctx, &userbuf) == -1);

is this ; expected here?

bfaccini · 2023-03-01T08:03:56Z

you can squash all the commits into a single one, please

Ok

Since you're using an Intel e-mail address for your commits, you have to update all dates to -2023. E.g., in this file: 2016-2023

Ok

  if (operation_add_user_buffer(ctx, &userbuf) == -1);

is this ; expected here?

Oops, sorry about the typo :-(

All these changes will be in next commit.

When missing, Handle ENOMEM during vector allocations to avoid later crashing. Ref: pmem/issues#5515 Signed-off-by: Bruno Faccini <[email protected]>

lukaszstolarczuk

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @bfaccini and @pbalcer)

src/libpmemobj/memops.c line 618 at r4 (raw file):

	if (VEC_PUSH_BACK(&ctx->next, buffer_offset) != 0)
		return -1;

just wondering, should we clean something up in this case? I can see we're persisting something above 😉

bfaccini · 2023-03-01T20:11:12Z

just wondering, should we clean something up in this case? I can see we're persisting something above 😉

Well, I thought this kind of concern could araise when I started to work on how to fix this, as usual for error paths...
The problem is that I don't know what exactly does/provisions pmemops_persist() nor I have been able to identify which method/operation can revert its effects...
Also, and strangely (!!), I only found one error path after pmemops_persist() has been called (other place do not show any error path at all !!...), in src/libpmemobj/obj.c:obj_runtime_init(), and there is no clean-up there too !!

grom72 · 2023-04-04T17:56:32Z

@bfaccini are you going to fix this PR to pass tests?

Looks like doing so affects VEC_PUSH_BACK() execution, surprisingly for non-debug build only, preventing next vector allocation... Signed-off-by: Bruno Faccini <[email protected]>

bfaccini · 2023-04-06T14:15:52Z

@bfaccini are you going to fix this PR to pass tests?

Looks like embeding VEC_PUSH_BACK() in ASSERT() affects VEC_PUSH_BACK() execution, surprisingly for non-debug build only, preventing next vector allocation...

Signed-off-by: Bruno Faccini <[email protected]>

bfaccini · 2023-04-07T14:21:57Z

Well, now that I have fixed the original errors/SEGVs during "./RUNTESTS -b nondebug obj_action -s TEST0", I now get new ones/asserts/aborts, but unfortunately I am unable to reproduce any of them locally :-(
@grom72 , any help/idea would be appreciated.

testing return value from VEC_PUSH_BACK() equal to 0 for error is not good !!... Ref: pmem/issues#5515 Signed-off-by: Bruno Faccini <[email protected]>

bfaccini · 2023-04-14T11:35:25Z

Well, now that I have fixed the original errors/SEGVs during "./RUNTESTS -b nondebug obj_action -s TEST0", I now get new ones/asserts/aborts, but unfortunately I am unable to reproduce any of them locally :-(

Now that I have found that these asserts/aborts were due to the "wrong/reverse test" in my previous changes, some workflows are canceled and one has failed, but I am not able to find any reason in the logs... Can somebody help ??

bfaccini · 2023-04-19T21:31:28Z

Can somebody help ??

???

codecov · 2023-04-20T07:01:23Z

Codecov Report

Merging #5537 (e717f1a) into master (3c623ae) will increase coverage by 0.56%.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##           master    #5537      +/-   ##
==========================================
+ Coverage   71.62%   72.18%   +0.56%     
==========================================
  Files         162      146      -16     
  Lines       24256    22655    -1601     
  Branches        0     3778    +3778     
==========================================
- Hits        17373    16354    -1019     
+ Misses       6883     6301     -582

pbalcer · 2023-04-20T07:04:09Z

I restarted the CI and most things pass, and the things that fail seem unrelated:


2023-04-13T08:00:40.9198249Z obj_basic_integration/TEST7: pmemdetect: /dev/dax0.2 -- permission denied
2023-04-13T08:00:40.9205687Z RUNTESTS: stopping: obj_basic_integration/TEST7 failed, TEST=check FS=none BUILD=debug
2023-04-13T08:00:41.0181676Z obj_basic_integration/TEST8: pmemdetect: /dev/dax0.2 -- permission denied
2023-04-13T08:00:41.0189683Z RUNTESTS: stopping: obj_basic_integration/TEST8 failed, TEST=check FS=none BUILD=debug
2023-04-13T08:00:41.1141737Z obj_basic_integration/TEST9: pmemdetect: /dev/dax0.2 -- permission denied
2023-04-13T08:00:41.1149866Z RUNTESTS: stopping: obj_basic_integration/TEST9 failed, TEST=check FS=none BUILD=debug
2023-04-13T08:00:41.2133923Z obj_basic_integration/TEST10: pmemdetect: /dev/dax0.2 -- permission denied
2023-04-13T08:00:41.2141758Z RUNTESTS: stopping: obj_basic_integration/TEST10 failed, TEST=check FS=none BUILD=debug

Can you rebase to see if this goes away?

wlemkows · 2023-04-20T07:15:05Z

I restarted the CI and most things pass, and the things that fail seem unrelated:


2023-04-13T08:00:40.9198249Z obj_basic_integration/TEST7: pmemdetect: /dev/dax0.2 -- permission denied
2023-04-13T08:00:40.9205687Z RUNTESTS: stopping: obj_basic_integration/TEST7 failed, TEST=check FS=none BUILD=debug
2023-04-13T08:00:41.0181676Z obj_basic_integration/TEST8: pmemdetect: /dev/dax0.2 -- permission denied
2023-04-13T08:00:41.0189683Z RUNTESTS: stopping: obj_basic_integration/TEST8 failed, TEST=check FS=none BUILD=debug
2023-04-13T08:00:41.1141737Z obj_basic_integration/TEST9: pmemdetect: /dev/dax0.2 -- permission denied
2023-04-13T08:00:41.1149866Z RUNTESTS: stopping: obj_basic_integration/TEST9 failed, TEST=check FS=none BUILD=debug
2023-04-13T08:00:41.2133923Z obj_basic_integration/TEST10: pmemdetect: /dev/dax0.2 -- permission denied
2023-04-13T08:00:41.2141758Z RUNTESTS: stopping: obj_basic_integration/TEST10 failed, TEST=check FS=none BUILD=debug

Can you rebase to see if this goes away?

Unfortunately, this is a known issue (platform setup issue), it is not related to your PR. We are working on it.

bfaccini · 2023-04-20T12:35:43Z

Can you rebase to see if this goes away?

Unfortunately, this is a known issue (platform setup issue), it is not related to your PR. We are working on it.

But should I rebase finally ?

pbalcer · 2023-04-21T08:18:01Z

No, everything seems fine.

LGTM overall.

lukaszstolarczuk

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @bfaccini and @pbalcer)

src/libpmemobj/memops.c line 618 at r4 (raw file):
@wlemkows, can you please take a look at this piece of code...?

I allow myself to quote bfaccini's words (it was out of this issues' thread):

Well, I thought this kind of concern could araise when I started to work on how to fix this, as usual for error paths...
The problem is that I don't know what exactly does/provisions pmemops_persist() nor I have been able to identify which method/operation can revert its effects...
Also, and strangely (!!), I only found one error path after pmemops_persist() has been called (other place do not show any error path at all !!...), in src/libpmemobj/obj.c:obj_runtime_init(), and there is no clean-up there too !!

bfaccini mentioned this pull request Feb 13, 2023

resiliency can be improved when facing constrained memory situations #5515

Open

pbalcer reviewed Feb 13, 2023

View reviewed changes

bfaccini requested a review from pbalcer February 14, 2023 19:00

pbalcer approved these changes Feb 15, 2023

View reviewed changes

bfaccini requested a review from pbalcer February 15, 2023 13:47

bfaccini marked this pull request as ready for review February 23, 2023 17:40

lukaszstolarczuk reviewed Feb 28, 2023

View reviewed changes

obj: handle ENOMEM during vector allocations

be023c9

When missing, Handle ENOMEM during vector allocations to avoid later crashing. Ref: pmem/issues#5515 Signed-off-by: Bruno Faccini <[email protected]>

bfaccini force-pushed the changes_for_issue_5515_2 branch from 6b9e943 to be023c9 Compare March 1, 2023 08:39

bfaccini requested review from lukaszstolarczuk and removed request for pbalcer March 1, 2023 08:45

lukaszstolarczuk reviewed Mar 1, 2023

View reviewed changes

bfaccini requested a review from lukaszstolarczuk March 1, 2023 20:11

obj: do not embed VEC_PUSH_BACK() in ASSERT()

5a2cc1b

Looks like doing so affects VEC_PUSH_BACK() execution, surprisingly for non-debug build only, preventing next vector allocation... Signed-off-by: Bruno Faccini <[email protected]>

obj: comply to tab vs space rule for pmdk...

1eb27e7

Signed-off-by: Bruno Faccini <[email protected]>

obj: oops wrong/reverse test

e717f1a

testing return value from VEC_PUSH_BACK() equal to 0 for error is not good !!... Ref: pmem/issues#5515 Signed-off-by: Bruno Faccini <[email protected]>

lukaszstolarczuk requested changes Apr 24, 2023

View reviewed changes

janekmi added this to the 1.13 milestone Apr 25, 2023

lukaszstolarczuk removed this from the 1.13 milestone Apr 26, 2023

janekmi added this to the 1.14 milestone Apr 27, 2023

janekmi modified the milestones: 2.0.0, 2.0.1 Jul 21, 2023

grom72 assigned janekmi Sep 15, 2023

janekmi modified the milestones: 2.0.1, 2.0.2 Nov 23, 2023

janekmi modified the milestones: 2.1.0, 2.x Feb 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

obj: handle ENOMEM during vector allocations #5537

obj: handle ENOMEM during vector allocations #5537

bfaccini commented Feb 13, 2023 •

edited by grom72

Loading

pbalcer Feb 13, 2023

bfaccini Feb 14, 2023

pbalcer Feb 13, 2023

bfaccini Feb 14, 2023

pbalcer Feb 15, 2023

bfaccini Feb 15, 2023

lukaszstolarczuk left a comment

bfaccini commented Mar 1, 2023

lukaszstolarczuk left a comment

bfaccini commented Mar 1, 2023

grom72 commented Apr 4, 2023

bfaccini commented Apr 6, 2023

bfaccini commented Apr 7, 2023

bfaccini commented Apr 14, 2023

bfaccini commented Apr 19, 2023

codecov bot commented Apr 20, 2023

pbalcer commented Apr 20, 2023

wlemkows commented Apr 20, 2023

bfaccini commented Apr 20, 2023

pbalcer commented Apr 21, 2023

lukaszstolarczuk left a comment

obj: handle ENOMEM during vector allocations #5537

Are you sure you want to change the base?

obj: handle ENOMEM during vector allocations #5537

Conversation

bfaccini commented Feb 13, 2023 • edited by grom72 Loading

pbalcer Feb 13, 2023

Choose a reason for hiding this comment

bfaccini Feb 14, 2023

Choose a reason for hiding this comment

pbalcer Feb 13, 2023

Choose a reason for hiding this comment

bfaccini Feb 14, 2023

Choose a reason for hiding this comment

pbalcer Feb 15, 2023

Choose a reason for hiding this comment

bfaccini Feb 15, 2023

Choose a reason for hiding this comment

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

bfaccini commented Mar 1, 2023

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

bfaccini commented Mar 1, 2023

grom72 commented Apr 4, 2023

bfaccini commented Apr 6, 2023

bfaccini commented Apr 7, 2023

bfaccini commented Apr 14, 2023

bfaccini commented Apr 19, 2023

codecov bot commented Apr 20, 2023

Codecov Report

pbalcer commented Apr 20, 2023

wlemkows commented Apr 20, 2023

bfaccini commented Apr 20, 2023

pbalcer commented Apr 21, 2023

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

bfaccini commented Feb 13, 2023 •

edited by grom72

Loading