Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAOS-16876 vos: set cont parameter when deregister modification from DTX #15657

Merged
merged 1 commit into from
Dec 27, 2024
+26 −26
Merged

DAOS-16876 vos: set cont parameter when deregister modification from DTX #15657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions src/vos/tests/vts_aggregate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@

static bool slow_test;

static void
static inline void
cleanup(void)
{
daos_fail_loc_set(DAOS_VOS_GC_CONT_NULL | DAOS_FAIL_ALWAYS);
gc_wait();
}

Expand Down Expand Up @@ -1314,24 +1313,19 @@ agg_punches_test(void **state, int record_type, bool discard)
}
}
}
/** cleanup() sets the flag to assert if there are items in container garbage collection
* heap which will always be the case for these punch tests. So let's run garbage
* collection before cleanup in this case.
*/
gc_wait();

cleanup();
}
static void
discard_14(void **state)
{
agg_punches_test(state, DAOS_IOD_SINGLE, true);
cleanup();
}

static void
discard_15(void **state)
{
agg_punches_test(state, DAOS_IOD_ARRAY, true);
cleanup();
}

static void
Expand Down
18 changes: 8 additions & 10 deletions src/vos/vos_dtx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1584,7 +1584,6 @@ vos_dtx_deregister_record(struct umem_instance *umm, daos_handle_t coh,
uint32_t entry, daos_epoch_t epoch, umem_off_t record)
{
struct dtx_handle *dth = vos_dth_get(false);
struct vos_container *cont;
struct vos_dtx_act_ent *dae;
struct vos_dtx_act_ent_df *dae_df;
umem_off_t *rec_df;
Expand All @@ -1593,20 +1592,19 @@ vos_dtx_deregister_record(struct umem_instance *umm, daos_handle_t coh,
int rc;
int i;

/*
* If @coh is empty handle, then we are destroying the container. Under such case,
* both the in-DRAM and on-dish DTX entry have already been released or destroyed.
*/
if (daos_handle_is_inval(coh))
return 0;

if (!vos_dtx_is_normal_entry(entry))
return 0;

D_ASSERT(entry >= DTX_LID_RESERVED);

cont = vos_hdl2cont(coh);
/* If "cont" is NULL, then we are destroying the container.
* Under such case, the DTX entry in DRAM has been removed,
* The on-disk entry will be destroyed soon.
*/
if (cont == NULL)
return 0;

found = lrua_lookupx(cont->vc_dtx_array, entry - DTX_LID_RESERVED,
found = lrua_lookupx(vos_hdl2cont(coh)->vc_dtx_array, entry - DTX_LID_RESERVED,
epoch, &dae);
if (!found) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe risky, for some case, such as the DTX entry has been removed for error cleanup but related DTX sponsor may still cache some stable information. Then deregister the DTX record may not find the entry. On the other hand, non-exist DTX record when deregister is not fatal. Consider current patch, we destroy the DTX table firstly during drain the container, it may also cause non-exist case.

D_WARN("Could not find active DTX record for lid=%d, epoch="
Expand Down
15 changes: 10 additions & 5 deletions src/vos/vos_gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,15 @@ gc_drain_cont(struct vos_gc *gc, struct vos_pool *pool, daos_handle_t coh,
int i;
int rc;

/*
* When we prepaer to drain the container, we do not need DTX entry any long.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo s/prepaer/prepare

* Then destroy DTX table firstly to avoid dangling DXT records during drain
* the container (that may yield).
*/
rc = vos_dtx_table_destroy(&pool->vp_umm, cont);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why it's moved here, then it'll called on draining every object? The original place looks more proper to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in gc_drain_cont(), there is assertion:
D_ASSERT(daos_handle_is_inval(coh)); return gc_drain_btr(gc, pool, coh, item, &cont->cd_obj_root, credits, empty);
Means that the coh parameter for dtx_deregister will always be zero for draining the container. That will cause related DTX entry to be left as dangling. To avoid such case, since it will be drained, the DTX table is useless any longer, we can destroyed the DTX table firstly. On the other hand, even if the coh is not zero, deregistering DTX records one by one is also inefficient for drain case, destroying the table will be more efficient.

if (rc != 0)
return rc;

/** Move any leftover bags to the pool gc */
for (i = GC_AKEY; i < GC_CONT; i++) {
src_bin = &cont->cd_gc_bins[i];
Expand Down Expand Up @@ -331,11 +340,7 @@ gc_free_cont(struct vos_gc *gc, struct vos_pool *pool, daos_handle_t coh, struct
}
}

rc = vos_dtx_table_destroy(&pool->vp_umm, cd);
if (rc == 0)
rc = umem_free(&pool->vp_umm, item->it_addr);

return rc;
return umem_free(&pool->vp_umm, item->it_addr);
}

static struct vos_gc gc_table[] = {
Expand Down
2 changes: 1 addition & 1 deletion src/vos/vos_obj.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2599,7 +2599,7 @@ vos_obj_iter_aggregate(daos_handle_t ih, bool range_discard)
* be aborted. Then it will be added and handled via GC when ktr_rec_free().
*/

rc = dbtree_iter_delete(oiter->it_hdl, NULL);
rc = dbtree_iter_delete(oiter->it_hdl, obj->obj_cont);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like the main bug here

D_ASSERT(rc != -DER_NONEXIST);
} else if (rc == -DER_NONEXIST) {
/* Key no longer exists at epoch but isn't empty */
Expand Down
5 changes: 4 additions & 1 deletion src/vos/vos_obj_index.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -895,6 +895,7 @@ oi_iter_aggregate(daos_handle_t ih, bool range_discard)
struct vos_oi_iter *oiter = iter2oiter(iter);
struct vos_container *cont = oiter->oit_cont;
struct vos_obj_df *obj;
struct oi_delete_arg del_arg;
daos_unit_oid_t oid;
d_iov_t rec_iov;
bool delete = false, invisible = false;
Expand Down Expand Up @@ -945,7 +946,9 @@ oi_iter_aggregate(daos_handle_t ih, bool range_discard)
if (rc != 0)
D_ERROR("Could not evict object "DF_UOID" "DF_RC"\n",
DP_UOID(oid), DP_RC(rc));
rc = dbtree_iter_delete(oiter->oit_hdl, NULL);
del_arg.cont = oiter->oit_cont;
del_arg.only_delete_entry = 0;
rc = dbtree_iter_delete(oiter->oit_hdl, &del_arg);
D_ASSERT(rc != -DER_NONEXIST);
} else if (rc == -DER_NONEXIST) {
/** ilog isn't visible in range but still has some entries */
Expand Down
Loading