On Tue, Jan 17, 2023 at 10:13 AM Ilya Maximets <[email protected]> wrote: > > On 1/6/23 07:35, Han Zhou wrote: > > > > > > On Tue, Jan 3, 2023 at 8:22 AM Ilya Maximets <[email protected] <mailto:[email protected]>> wrote: > >> > >> The counter for the number of atoms has to be re-set to the number from > >> the new database, otherwise the value will be incorrect. For example, > >> this is causing the atom counter doubling after online conversion of > >> a clustered database. > >> > >> Miscounting may also lead to increased memory consumption by the > >> transaction history or otherwise too aggressive transaction history > >> sweep. > >> > >> Fixes: 317b1bfd7dd3 ("ovsdb: Don't let transaction history grow larger than the database.") > >> Signed-off-by: Ilya Maximets <[email protected] <mailto: [email protected]>> > >> --- > >> ovsdb/ovsdb.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c > >> index 11786f376..afec96264 100644 > >> --- a/ovsdb/ovsdb.c > >> +++ b/ovsdb/ovsdb.c > >> @@ -715,5 +715,8 @@ ovsdb_replace(struct ovsdb *dst, struct ovsdb *src) > >> > >> dst->rbac_role = ovsdb_get_table(dst, "RBAC_Role"); > >> > >> + /* Get statistics from the new database. */ > >> + dst->n_atoms = src->n_atoms; > >> + > >> ovsdb_destroy(src); > >> } > >> -- > >> 2.38.1 > >> > > > > Thanks Ilya. The fix looks good to me. It would be even better to update a related test case to verify both n_atoms and n_txn_history_atoms are correct after DB conversion. > > > > Acked-by: Han Zhou <[email protected] <mailto:[email protected]>> > > Makes sense. I can add the following change: > > diff --git a/tests/ovsdb-server.at b/tests/ovsdb-server.at > index 0828e6d04..bf539b6e5 100644 > --- a/tests/ovsdb-server.at > +++ b/tests/ovsdb-server.at > @@ -1308,6 +1308,24 @@ dnl After removing all the bridges, the number of atoms in the database > dnl should return to its initial value. > AT_CHECK([test $(get_memory_value atoms) -eq $initial_db_atoms]) > > +dnl Add a few more resources. > +for i in $(seq 1 10); do > + cmd=$(add_ports $i $(($i / 4 + 1))) > + AT_CHECK([ovs-vsctl --no-wait add-br br$i $cmd]) > +done > +check_atoms > + > +db_atoms_before_conversion=$(get_memory_value atoms) > + > +dnl Trigger online conversion. > +AT_CHECK([ovsdb-client convert $abs_top_srcdir/vswitchd/vswitch.ovsschema], > + [0], [ignore], [ignore]) > + > +dnl Check that conversion didn't change the number of atoms and the history > +dnl still has a reasonable size. > +check_atoms > +AT_CHECK([test $(get_memory_value atoms) -eq $db_atoms_before_conversion]) > + > OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > AT_CLEANUP > ^L > --- > > What do you think? > LGTM. Thanks Ilya. Han
> Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
