DAOS-5758 vos: Add garbage collection at container level #4997

jolivier23 · 2021-03-13T02:08:27Z

Since DTX entries can still reference objects that are
being deleted by vos_obj_delete, we need to pass
a container handle when removing them so that any
DTX references can be removed upon deletion.

This change adds a container level garbage collection heap
so we can meet this requirement. When a container is
destroyed, the collection of that container will move
any outstanding items to the pool level heap.

Signed-off-by: Jeff Olivier jeffrey.v.olivier@intel.com

Since DTX entries can still reference objects that are being deletect by vos_obj_delete, we need to pass a container handle when removing them so that any DTX references can be removed upon deletion. This change adds a container level garbage collection heap so we can meet this requirement. When a container is destroyed, the collection of that container will move any outstanding items to the pool level heap. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 · 2021-03-13T02:10:26Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4997/1/execution/node/72/log

daosbuild1

LGTM. No errors found by checkpatch.

added also are added to container heaps Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2021-03-13T04:57:21Z

Test stage Unit Test with memcheck completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4997/3/execution/node/896/log

the bag Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

an existing container if there are outstanding items to be collected. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Also add a test for close and open container Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2021-03-13T09:15:57Z

Test stage Unit Test completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4997/7/execution/node/924/log

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

do not go to container lists I believe the PR that ensures no orphaned subtrees Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2021-03-13T19:31:19Z

Test stage Unit Test completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4997/9/execution/node/909/log

daosbuild1 · 2021-03-13T19:40:47Z

Test stage Unit Test with memcheck completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-4997/9/execution/node/910/log

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

jolivier23 · 2021-03-14T07:03:40Z

Failure is due to DAOS-7016

NiuYawei · 2021-03-15T14:49:33Z

src/vos/vos_gc.c

+	 * the list and give another container a turn next time.
+	 */
+	cont = d_list_pop_entry(&pool->vp_gc_cont, struct vos_container,
+				vc_gc_link);


What if a container is destroyed? The vos_container will be freed but still linked?

Instead of passing 'coh' to GC, Is there any way to make DTX to skip the entries belong to deleted objects?

We do d_list_del in cont_free (when last reference is dropped). In such case, the entries in the container can't be aggregated until the container is destroyed or reopened. When it's destroyed, the entries will be moved to the pool bins.

I see, thanks.

NiuYawei

The patch looks good to me in general, few minor questions.

Changing GC to solve the problem is ok to me, but looks not the most efficient way, since DTX will have to do commit for deleted object (before GC reclaiming them)? I think a better way is to have DTX taking proper actions on the entries belongs to deleted object. Let's see if @Nasf-Fan have any good idea.

NiuYawei · 2021-03-16T01:24:37Z

src/vos/tests/vts_aggregate.c

@@ -611,6 +618,8 @@ discard_1(void **state)
 			    ds.td_agg_epr.epr_lo, ds.td_iod_size);
 		aggregate_basic(arg, &ds, 0, NULL);
 	}
+
+	cleanup();


I don't see the purpose of this call for each discard/aggregation test case.

I wanted to assert that the aggregation entries get added to the pool heap. Running the garbage collector after each aggregation test allows me to test this.

I see, but most aggregation/discard test cases won't add anything to GC. Maybe only object/key punch involved tests adds things to GC.

NiuYawei · 2021-03-16T01:30:49Z

src/vos/vos_gc.c

+	 * the list and give another container a turn next time.
+	 */
+	cont = d_list_pop_entry(&pool->vp_gc_cont, struct vos_container,
+				vc_gc_link);


I see, thanks.

NiuYawei · 2021-03-16T01:48:52Z

src/vos/vos_obj.c

@@ -459,9 +459,6 @@ vos_obj_delete(daos_handle_t coh, daos_unit_oid_t oid)
 	rc = umem_tx_end(umm, rc);
 	if (rc)
 		goto out;
-
-	/* NB: noop for full-stack mode */
-	gc_wait();


Why it's removed?

For testing purposes mainly. It's only for unit tests and I wanted the gc tests to actually test garbage collection after the fact and not having that gives me more control over when it runs. It really should have no effect on anything but unit tests since only reintegration uses these APIs and it's a noop in that case.

jolivier23 · 2021-03-16T03:06:44Z

The patch looks good to me in general, few minor questions.

Changing GC to solve the problem is ok to me, but looks not the most efficient way, since DTX will have to do commit for deleted object (before GC reclaiming them)? I think a better way is to have DTX taking proper actions on the entries belongs to deleted object. Let's see if @Nasf-Fan have any good idea.

I think it could be tricky to do.

Adding it to the garbage collector doesn't impose any ordering requirements for commit. The reason for adding the container to the callback is so that the transaction entries get removed when the object/key/value gets removed by the garbage collector. If commit happens first, it will do a little extra work and save the garbage collector a little work. If the delete happens first, commit will have less stuff in its list of things to do.

Nasf-Fan

Only some leak issues in the tests.

src/vos/tests/vts_gc.c

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

gnailzenh · 2021-03-17T04:05:05Z

Sorry but we cannot pass container "handle" to GC functions, the GC bins & bags are stored in PMEM so GC can proceed even after server shutdown/crash. Container handle is an ephemeral address in DRAM, so it wouldn't work anymore after reboot. Can we just pass vos_cont_df into GC?

It also means that we don't need an extra parameter for:
gc_add_item(struct vos_pool *pool, enum vos_gc_type type, umem_off_t item_off, uint64_t args)

The whole intention of having the "args" is for this kind of requirement.

Nasf-Fan · 2021-03-17T04:57:07Z

Sorry but we cannot pass container "handle" to GC functions, the GC bins & bags are stored in PMEM so GC can proceed even after server shutdown/crash. Container handle is an ephemeral address in DRAM, so it wouldn't work anymore after reboot. Can we just pass vos_cont_df into GC?

It also means that we don't need an extra parameter for:
gc_add_item(struct vos_pool *pool, enum vos_gc_type type, umem_off_t item_off, uint64_t args)

The whole intention of having the "args" is for this kind of requirement.

The DTX entry index is in DRAM, please check lrua_lookupx(), we can find the DTX entry only when we have the container handle in DRAM. The vos_cont_df does NOT work for such purpose.

jolivier23 · 2021-03-17T13:21:22Z

Sorry but we cannot pass container "handle" to GC functions, the GC bins & bags are stored in PMEM so GC can proceed even after server shutdown/crash. Container handle is an ephemeral address in DRAM, so it wouldn't work anymore after reboot. Can we just pass vos_cont_df into GC?
It also means that we don't need an extra parameter for:
gc_add_item(struct vos_pool *pool, enum vos_gc_type type, umem_off_t item_off, uint64_t args)
The whole intention of having the "args" is for this kind of requirement.

The DTX entry index is in DRAM, please check lrua_lookupx(), we can find the DTX entry only when we have the container handle in DRAM. The vos_cont_df does NOT work for such purpose.

Right. I had a chat with Liang last night after he posted the comment. The container handle is only used to

indicate to gc that it should store the object/key in the container bin
Ensure the container handle is added to the list of open containers that have gc items

The container handle itself isn't stored anywhere and when a container is closed and reopened, the new handle will be tracked if there are outstanding objects to be collected.

Since DTX entries can still reference objects that are being deleted by vos_obj_delete, we need to pass a container handle when removing them so that any DTX references can be removed upon deletion. This change adds a container level garbage collection heap so we can meet this requirement. When a container is destroyed, the collection of that container will move any outstanding items to the pool level heap. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

jolivier23 requested review from gnailzenh, Nasf-Fan, NiuYawei and ryon-jensen March 13, 2021 02:08

jolivier23 force-pushed the jvolivie/gc_bin branch from d2328fa to 4130002 Compare March 13, 2021 02:10

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

Ok, have to make sure any keys or other entries that get

8996f13

added also are added to container heaps Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

Avoid memory leak by saving offset before potentially freeing

a1ffd6f

the bag Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

Anticipate some code review comments

7fef4f4

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

Ensure we kick of container garbage collection upon opening

27c61f9

an existing container if there are outstanding items to be collected. Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

Disable the wait in vos_obj_delete and key delete

5881ab9

Also add a test for close and open container Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

Need to clear the container bin so we can process it again

703205c

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

Add some fault injection to test that aggregation deletions

6b43698

do not go to container lists I believe the PR that ensures no orphaned subtrees Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

Reset fail_loc in gc post

6647f9c

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

minor style correction

7a6748a

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

daosbuild1 reviewed Mar 13, 2021

View reviewed changes

NiuYawei reviewed Mar 15, 2021

View reviewed changes

jolivier23 requested a review from NiuYawei March 15, 2021 15:00

NiuYawei reviewed Mar 16, 2021

View reviewed changes

jolivier23 requested a review from NiuYawei March 16, 2021 03:13

NiuYawei previously approved these changes Mar 16, 2021

View reviewed changes

Nasf-Fan requested changes Mar 16, 2021

View reviewed changes

src/vos/tests/vts_gc.c Outdated Show resolved Hide resolved

src/vos/tests/vts_gc.c Outdated Show resolved Hide resolved

jolivier23 added 2 commits March 16, 2021 07:33

Merge branch 'master' into jvolivie/gc_bin

eade381

Address review comments

fb42546

Signed-off-by: Jeff Olivier <jeffrey.v.olivier@intel.com>

jolivier23 dismissed NiuYawei’s stale review via fb42546 March 16, 2021 13:38

jolivier23 requested review from daosbuild1, Nasf-Fan and NiuYawei March 16, 2021 13:40

daosbuild1 reviewed Mar 16, 2021

View reviewed changes

NiuYawei approved these changes Mar 16, 2021

View reviewed changes

Nasf-Fan approved these changes Mar 16, 2021

View reviewed changes

ryon-jensen approved these changes Mar 17, 2021

View reviewed changes

jolivier23 merged commit 345bd0f into master Mar 17, 2021

jolivier23 deleted the jvolivie/gc_bin branch March 17, 2021 21:33

ashleypittman mentioned this pull request Apr 28, 2021

1.2 dfuse single #5582

Closed

ashleypittman mentioned this pull request May 20, 2021

1.2 proc #5753

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-5758 vos: Add garbage collection at container level #4997

DAOS-5758 vos: Add garbage collection at container level #4997

jolivier23 commented Mar 13, 2021 •

edited

Loading

daosbuild1 commented Mar 13, 2021

daosbuild1 left a comment

daosbuild1 left a comment

daosbuild1 commented Mar 13, 2021

daosbuild1 left a comment

daosbuild1 left a comment

daosbuild1 left a comment

daosbuild1 left a comment

daosbuild1 commented Mar 13, 2021

daosbuild1 left a comment

daosbuild1 left a comment

daosbuild1 commented Mar 13, 2021

daosbuild1 commented Mar 13, 2021

daosbuild1 left a comment

daosbuild1 left a comment

jolivier23 commented Mar 14, 2021

NiuYawei Mar 15, 2021

jolivier23 Mar 15, 2021

NiuYawei Mar 16, 2021

NiuYawei left a comment

NiuYawei Mar 16, 2021

jolivier23 Mar 16, 2021

NiuYawei Mar 16, 2021

NiuYawei Mar 16, 2021

NiuYawei Mar 16, 2021

jolivier23 Mar 16, 2021

jolivier23 commented Mar 16, 2021

Nasf-Fan left a comment

daosbuild1 left a comment

gnailzenh commented Mar 17, 2021

Nasf-Fan commented Mar 17, 2021 •

edited

Loading

jolivier23 commented Mar 17, 2021

DAOS-5758 vos: Add garbage collection at container level #4997

DAOS-5758 vos: Add garbage collection at container level #4997

Conversation

jolivier23 commented Mar 13, 2021 • edited Loading

daosbuild1 commented Mar 13, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Mar 13, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Mar 13, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Mar 13, 2021

daosbuild1 commented Mar 13, 2021

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

jolivier23 commented Mar 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiuYawei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolivier23 commented Mar 16, 2021

Nasf-Fan left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

gnailzenh commented Mar 17, 2021

Nasf-Fan commented Mar 17, 2021 • edited Loading

jolivier23 commented Mar 17, 2021

jolivier23 commented Mar 13, 2021 •

edited

Loading

Nasf-Fan commented Mar 17, 2021 •

edited

Loading