From 68f2c04f12e22706bdffebb751d5ca7c1b279c24 Mon Sep 17 00:00:00 2001 From: Fan Yong Date: Wed, 12 Aug 2026 16:11:18 +0800 Subject: [PATCH] DAOS-19284 vos: check conflict among distributed transactions - b28 For a distributed transaction, its epoch is generated by the first accessed server. So two distributed transactions may use the same epoch (if their first accessed servers are different) to operate the same set of object(s) by race. In theory, two read distributed transactions do not conflict with each other, but read maybe just part of the transaction, and there maybe update/punch in the same distributed transaction subsequently. Since we can keep only one (sponsor) timestamp for one TS entry, then have to request another one to restart. That may cause some fake conflict, but correctness is more important and such race is rare. Signed-off-by: Fan Yong --- src/include/daos/dtx.h | 4 +- src/vos/tests/vts_mvcc.c | 12 ++--- src/vos/tests/vts_pm.c | 9 ++-- src/vos/vos_io.c | 9 ++-- src/vos/vos_iterator.c | 7 +-- src/vos/vos_obj.c | 3 +- src/vos/vos_query.c | 20 ++++++--- src/vos/vos_ts.c | 6 +-- src/vos/vos_ts.h | 96 +++++++++++++++++++++++++++++----------- 9 files changed, 112 insertions(+), 54 deletions(-) diff --git a/src/include/daos/dtx.h b/src/include/daos/dtx.h index b91e5a1084c..acdc15bc961 100644 --- a/src/include/daos/dtx.h +++ b/src/include/daos/dtx.h @@ -1,6 +1,6 @@ /** * (C) Copyright 2019-2023 Intel Corporation. - * (C) Copyright 2025 Hewlett Packard Enterprise Development LP + * (C) Copyright 2025-2026 Hewlett Packard Enterprise Development LP * * SPDX-License-Identifier: BSD-2-Clause-Patent */ @@ -256,7 +256,7 @@ daos_is_zero_dti(const struct dtx_id *dti) } static inline bool -daos_dti_equal(struct dtx_id *dti0, struct dtx_id *dti1) +daos_dti_equal(const struct dtx_id *dti0, const struct dtx_id *dti1) { return memcmp(dti0, dti1, sizeof(*dti0)) == 0; } diff --git a/src/vos/tests/vts_mvcc.c b/src/vos/tests/vts_mvcc.c index f6f1e40dc5d..84d05743c7d 100644 --- a/src/vos/tests/vts_mvcc.c +++ b/src/vos/tests/vts_mvcc.c @@ -1,6 +1,6 @@ /* * (C) Copyright 2020-2023 Intel Corporation. - * (C) Copyright 2025 Hewlett Packard Enterprise Development LP + * (C) Copyright 2025-2026 Hewlett Packard Enterprise Development LP * * SPDX-License-Identifier: BSD-2-Clause-Patent */ @@ -1273,11 +1273,13 @@ conflicting_rw_exec_one(struct io_test_args *arg, int i, int j, bool empty, expect_inprogress = true; } } - if (w->o_rtype == R_E && !e) - expected_wrc = -DER_EXIST; - else if (w->o_rtype == R_NE && e) - expected_wrc = -DER_NONEXIST; + if (re != we || same_tx) { + if (w->o_rtype == R_E && !e) + expected_wrc = -DER_EXIST; + else if (w->o_rtype == R_NE && e) + expected_wrc = -DER_NONEXIST; + } } print_message(" %s(%s, "DF_X64") (expect %s): ", w->o_name, wp, we, expect_inprogress ? "DER_INPROGRESS" : diff --git a/src/vos/tests/vts_pm.c b/src/vos/tests/vts_pm.c index 97049b9fc19..f29d4430e57 100644 --- a/src/vos/tests/vts_pm.c +++ b/src/vos/tests/vts_pm.c @@ -1,6 +1,6 @@ /** * (C) Copyright 2019-2022 Intel Corporation. - * (C) Copyright 2025 Hewlett Packard Enterprise Development LP + * (C) Copyright 2025-2026 Hewlett Packard Enterprise Development LP * * SPDX-License-Identifier: BSD-2-Clause-Patent */ @@ -1310,12 +1310,11 @@ cond_test(void **state) cond_fetch_op(state, arg->ctx.tc_co_hdl, oid, epoch++, true, "a", "b", 0, 0, sgl, "xxxx", 'x'); /** Conditional update of non-existent key should fail */ - cond_update_op(state, arg->ctx.tc_co_hdl, oid, epoch - 1, "a", "b", - VOS_OF_COND_DKEY_UPDATE, + cond_update_op(state, arg->ctx.tc_co_hdl, oid, epoch++, "a", "b", VOS_OF_COND_DKEY_UPDATE, -DER_NONEXIST, sgl, "foo"); /** Conditional punch of non-existent akey should fail */ - cond_akey_punch_op(state, arg->ctx.tc_co_hdl, oid, epoch, "a", "b", - VOS_OF_COND_PUNCH, -DER_NONEXIST); + cond_akey_punch_op(state, arg->ctx.tc_co_hdl, oid, epoch++, "a", "b", VOS_OF_COND_PUNCH, + -DER_NONEXIST); /** Key doesn't exist still, that supersedes read conflict */ cond_dkey_punch_op(state, arg->ctx.tc_co_hdl, oid, epoch++, "a", VOS_OF_COND_PUNCH, -DER_NONEXIST); diff --git a/src/vos/vos_io.c b/src/vos/vos_io.c index e599677e3c5..6a7a5bc578e 100644 --- a/src/vos/vos_io.c +++ b/src/vos/vos_io.c @@ -1721,7 +1721,8 @@ vos_fetch_begin(daos_handle_t coh, daos_unit_oid_t oid, daos_epoch_t epoch, if (rc == -DER_NONEXIST || rc == 0) { vos_fetch_add_missing(ioc->ic_ts_set, dkey, iod_nr, iods); - vos_ts_set_update(ioc->ic_ts_set, ioc->ic_epr.epr_hi); + if (!vos_ts_set_update(ioc->ic_ts_set, ioc->ic_epr.epr_hi)) + rc = -DER_TX_RESTART; } if (rc != 0) { @@ -2663,8 +2664,10 @@ vos_update_end(daos_handle_t ioh, uint32_t pm_ver, daos_key_t *dkey, int err, if (err == 0) vos_ts_set_upgrade(ioc->ic_ts_set); - if (err == -DER_NONEXIST || err == -DER_EXIST || err == 0) - vos_ts_set_update(ioc->ic_ts_set, ioc->ic_epr.epr_hi); + if (err == -DER_NONEXIST || err == -DER_EXIST || err == 0) { + if (!vos_ts_set_update(ioc->ic_ts_set, ioc->ic_epr.epr_hi)) + err = -DER_TX_RESTART; + } if (err == 0) vos_ts_set_wupdate(ioc->ic_ts_set, ioc->ic_epr.epr_hi); diff --git a/src/vos/vos_iterator.c b/src/vos/vos_iterator.c index 71cc4a1e4d9..1b5a68f0f82 100644 --- a/src/vos/vos_iterator.c +++ b/src/vos/vos_iterator.c @@ -269,8 +269,8 @@ vos_iter_prepare(vos_iter_type_t type, vos_iter_param_t *param, if (rc == -DER_NONEXIST && dtx_is_valid_handle(dth)) { if (vos_ts_wcheck(ts_set, dth->dth_epoch, dth->dth_epoch_bound)) rc = -DER_TX_RESTART; - else - vos_ts_set_update(ts_set, dth->dth_epoch); + else if (!vos_ts_set_update(ts_set, dth->dth_epoch)) + rc = -DER_TX_RESTART; } if (rc != 0) vos_ts_set_free(ts_set); @@ -306,7 +306,8 @@ vos_iter_ts_set_update(daos_handle_t ih, daos_epoch_t read_time, int rc) if (vos_ts_wcheck(iter->it_ts_set, read_time, iter->it_bound)) return -DER_TX_RESTART; - vos_ts_set_update(iter->it_ts_set, read_time); + if (!vos_ts_set_update(iter->it_ts_set, read_time)) + return -DER_TX_RESTART; return rc; } diff --git a/src/vos/vos_obj.c b/src/vos/vos_obj.c index 4d376d79551..70f13c85b90 100644 --- a/src/vos/vos_obj.c +++ b/src/vos/vos_obj.c @@ -578,7 +578,8 @@ vos_obj_punch(daos_handle_t coh, daos_unit_oid_t oid, daos_epoch_t epoch, if (rc == -DER_NONEXIST || rc == 0) { vos_punch_add_missing(ts_set, dkey, akey_nr, akeys); - vos_ts_set_update(ts_set, epr.epr_hi); + if (!vos_ts_set_update(ts_set, epr.epr_hi)) + rc = -DER_TX_RESTART; } if (rc == 0) { diff --git a/src/vos/vos_query.c b/src/vos/vos_query.c index b4d414012e5..371c88bf203 100644 --- a/src/vos/vos_query.c +++ b/src/vos/vos_query.c @@ -1,5 +1,6 @@ /** * (C) Copyright 2019-2024 Intel Corporation. + * (C) Copyright 2026 Hewlett Packard Enterprise Development LP * * SPDX-License-Identifier: BSD-2-Clause-Patent */ @@ -764,8 +765,11 @@ vos_obj_query_key(daos_handle_t coh, daos_unit_oid_t oid, uint32_t flags, /** Go ahead and save timestamps for * things we read */ - vos_ts_set_update(query->qt_ts_set, - obj_epr.epr_hi); + if (!vos_ts_set_update(query->qt_ts_set, obj_epr.epr_hi)) { + rc = -DER_TX_RESTART; + break; + } + vos_ts_set_restore(query->qt_ts_set, &akey_save); continue; @@ -776,7 +780,11 @@ vos_obj_query_key(daos_handle_t coh, daos_unit_oid_t oid, uint32_t flags, if (rc == -DER_NONEXIST && query->qt_flags & VOS_GET_DKEY) { /** Go ahead and save timestamps for things we read */ - vos_ts_set_update(query->qt_ts_set, obj_epr.epr_hi); + if (!vos_ts_set_update(query->qt_ts_set, obj_epr.epr_hi)) { + rc = -DER_TX_RESTART; + break; + } + vos_ts_set_restore(query->qt_ts_set, &dkey_save); continue; } @@ -801,8 +809,10 @@ vos_obj_query_key(daos_handle_t coh, daos_unit_oid_t oid, uint32_t flags, rc = -DER_TX_RESTART; } - if (rc == 0 || rc == -DER_NONEXIST) - vos_ts_set_update(query->qt_ts_set, obj_epr.epr_hi); + if (rc == 0 || rc == -DER_NONEXIST) { + if (!vos_ts_set_update(query->qt_ts_set, obj_epr.epr_hi)) + rc = -DER_TX_RESTART; + } vos_ts_set_free(query->qt_ts_set); free_query: diff --git a/src/vos/vos_ts.c b/src/vos/vos_ts.c index cc8c214a72a..3ed2883a374 100644 --- a/src/vos/vos_ts.c +++ b/src/vos/vos_ts.c @@ -1,5 +1,6 @@ /** * (C) Copyright 2020-2024 Intel Corporation. + * (C) Copyright 2026 Hewlett Packard Enterprise Development LP * * SPDX-License-Identifier: BSD-2-Clause-Patent */ @@ -375,10 +376,7 @@ vos_ts_check_conflict(daos_epoch_t read_time, const struct dtx_id *read_id, if (write_time != read_time) return true; - if (read_id->dti_hlc != write_id->dti_hlc) - return true; - - return uuid_compare(read_id->dti_uuid, write_id->dti_uuid) != 0; + return !daos_dti_equal(write_id, read_id); } bool diff --git a/src/vos/vos_ts.h b/src/vos/vos_ts.h index 379f59f819c..7239cc15cc4 100644 --- a/src/vos/vos_ts.h +++ b/src/vos/vos_ts.h @@ -1,5 +1,6 @@ /** * (C) Copyright 2020-2023 Intel Corporation. + * (C) Copyright 2026 Hewlett Packard Enterprise Development LP * * SPDX-License-Identifier: BSD-2-Clause-Patent */ @@ -682,27 +683,67 @@ vos_ts_copy(daos_epoch_t *dest_epc, struct dtx_id *dest_id, } /** Internal API to update low read timestamp and tx id */ -static inline void -vos_ts_rl_update(struct vos_ts_entry *entry, daos_epoch_t read_time, - const struct dtx_id *tx_id) +static inline int +vos_ts_rl_update(struct vos_ts_entry *entry, daos_epoch_t read_time, const struct dtx_id *tx_id) { if (entry == NULL || read_time < entry->te_ts.tp_ts_rl) - return; + return 0; + + /* + * Different servers may generate the same timestamp that may be used as + * the epoch for different distributed transactions. So do NOT update if + * "read_time == entry->te_ts.tp_ts_rl" to avoid lost read timestamp for + * others by race. In theory, two read transactions do not conflict with + * each other, but read maybe just part of the transaction. Since we can + * keep only one on such entry, then have to request another to restart. + * That may cause some fake conflict, but correctness is more important. + */ + if (read_time == entry->te_ts.tp_ts_rl) { + if (likely(daos_dti_equal(tx_id, &entry->te_ts.tp_tx_rl))) + return 0; + + /* It is very rare, so warning message will not be much trouble. */ + D_WARN("Refuse low read-TS update with the same epoch " DF_X64 " from multiple " + "sponsors: " DF_DTI " vs " DF_DTI "\n", + read_time, DP_DTI(tx_id), DP_DTI(&entry->te_ts.tp_tx_rl)); + return -DER_TX_RESTART; + } vos_ts_copy(&entry->te_ts.tp_ts_rl, &entry->te_ts.tp_tx_rl, read_time, tx_id); + return 0; } /** Internal API to update high read timestamp and tx id */ -static inline void -vos_ts_rh_update(struct vos_ts_entry *entry, daos_epoch_t read_time, - const struct dtx_id *tx_id) +static inline int +vos_ts_rh_update(struct vos_ts_entry *entry, daos_epoch_t read_time, const struct dtx_id *tx_id) { if (entry == NULL || read_time < entry->te_ts.tp_ts_rh) - return; + return 0; + + /* + * Different servers may generate the same timestamp that may be used as + * the epoch for different distributed transactions. So do NOT update if + * "read_time == entry->te_ts.tp_ts_rh" to avoid lost read timestamp for + * others by race. In theory, two read transactions do not conflict with + * each other, but read maybe just part of the transaction. Since we can + * keep only one on such entry, then have to request another to restart. + * That may cause some fake conflict, but correctness is more important. + */ + if (read_time == entry->te_ts.tp_ts_rh) { + if (likely(daos_dti_equal(tx_id, &entry->te_ts.tp_tx_rh))) + return 0; + + /* It is very rare, so warning message will not be much trouble. */ + D_WARN("Refuse high read-TS update with the same epoch " DF_X64 " from multiple " + "sponsors: " DF_DTI " vs " DF_DTI "\n", + read_time, DP_DTI(tx_id), DP_DTI(&entry->te_ts.tp_tx_rh)); + return -DER_TX_RESTART; + } vos_ts_copy(&entry->te_ts.tp_ts_rh, &entry->te_ts.tp_tx_rh, read_time, tx_id); + return 0; } /** Internal API to check read conflict of a given entry */ @@ -784,24 +825,27 @@ vos_ts_set_append_cflags(struct vos_ts_set *ts_set, uint16_t flags) /** Update the read timestamps for the set after a successful operation * - * \param[in] ts_set The timestamp set - * \param[in] read_time The new read timestamp + * \param[in] ts_set The timestamp set + * \param[in] read_time The new read timestamp + * + * \return true on success, false if need to restart. */ -static inline void +static inline bool vos_ts_set_update(struct vos_ts_set *ts_set, daos_epoch_t read_time) { - struct vos_ts_set_entry *se; - int i; - uint16_t read_level; + struct vos_ts_set_entry *se; + uint16_t read_level; + int i; + int rc; if (!vos_ts_in_tx(ts_set)) - return; + return true; if (DAOS_FAIL_CHECK(DAOS_DTX_NO_READ_TS)) - return; + return true; if ((ts_set->ts_cflags & VOS_TS_READ_MASK) == 0) - return; + return true; if (ts_set->ts_max_type < ts_set->ts_rd_level) read_level = ts_set->ts_max_type; @@ -812,16 +856,16 @@ vos_ts_set_update(struct vos_ts_set *ts_set, daos_epoch_t read_time) se = &ts_set->ts_entries[i]; if (se->se_etype > read_level) - continue; /** We would have updated the high - * timestamp at a higher level - */ - - if (se->se_etype == read_level) - vos_ts_rl_update(se->se_entry, read_time, - &ts_set->ts_tx_id); - vos_ts_rh_update(se->se_entry, read_time, - &ts_set->ts_tx_id); + continue; + + rc = vos_ts_rh_update(se->se_entry, read_time, &ts_set->ts_tx_id); + if (rc == 0 && se->se_etype == read_level) + rc = vos_ts_rl_update(se->se_entry, read_time, &ts_set->ts_tx_id); + if (rc != 0) + return false; } + + return true; } static inline void