Skip to content

Conversation

@ban-nobuhiro
Copy link
Contributor

@ban-nobuhiro ban-nobuhiro commented Nov 10, 2025

並行で delete を二つ実行した場合にタイミングによって masstree の root node が root bit を持たないという不整合状態になることがあり、それが起きてしまうと masstree の操作のほとんどができなくなるという不具合がありました。

案件: project-tsurugi/tsurugi-issues#1317

masstree の node は、操作対象の node 自身のロックによって保護されるメンバ変数のほかに、隣接 node (parent, prev) のロックによって保護されるメンバ変数があり、今回 parent node のロック (以下、「parent lock」) により保護されるメンバの見直しと、 parent が存在しない node の取り扱いを見直して、この問題を解消しました。

  • parent lock 保護対象として 従来は parent メンバ変数だけであったが root bit も parent lock 保護対象とする
  • parent が存在しない node、すなわち masstree root node に対する parent lock のため masstree root lock を導入

コミットの概略

  • 1aecd65: masstree root lock の導入
  • 51f6371: root bit の変更操作を parent/root lock 保護下へ移動
  • 28915a5: masstree root lock を lock_parent() 内で実施するように変更
  • その他は、デバッグ用関数display() の表示変更・コメントの変更・テストコードの修正 です

@ban-nobuhiro
Copy link
Contributor Author

28915a5 の補足

  • base_node::lock_parent() が masstree の tree_instence* を取り、関数内部で使うようにしたのでクラス構造を知る必要がある
  • → base_node は tree_instance クラスを知らないので、 include に追加した
  • すでに逆向きに tree_instance.h が base_node.h を include していて循環参照になる
  • → tree_instance は base_node をポインタとしてしか扱わず、クラス構造を知る必要はないので、 include をやめ前方宣言で済ますようにし、循環参照を解消した

void lock() { version_.lock(); }

/**
* @pre This function is called by split.
Copy link
Contributor

Choose a reason for hiding this comment

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

コードは問題ないのですが、関数のdoc commentに関数終了時に何がロックされた状態になるかという記述をしたほうがいいように思います。この関数以外でもそのあたりの記述が全体的に甘いですが、この関数の場合は特にnullptrを返すかどうかでロック対象が異なると思うのでそのあたりを注意喚起すべきと感じました。

Copy link
Contributor

Choose a reason for hiding this comment

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

ロックに関する状態を @pre@post に記述するようにするのはどうでしょうか。この関数の場合だと事前条件はなさそうなのでこんな感じ。

    /**
     * @post returned non-null parent node is locked.
     * If return value is nullptr, this node is root node and tree instance root lock is acquired.
     */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

改訂しました。 post と returns で同じ条件わけを書いて冗長になったので details 項を設けてそちらに移動しました。

Copy link
Contributor

Choose a reason for hiding this comment

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

ありがとうございます。よさげです。

Copy link
Contributor

@kuron99 kuron99 left a comment

Choose a reason for hiding this comment

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

コメントしたものはdocumentation絡みの話なのでapproveします。
doc comment関連の変更はまとめて別のissue/PRでもいいかもしれません。

@ban-nobuhiro ban-nobuhiro merged commit 5e0d639 into master Nov 19, 2025
4 checks passed
@ban-nobuhiro
Copy link
Contributor Author

レビューありがとうございました。マージしました。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants