본문으로 건너뛰기

Code Review: c7a90bfa — Reduce put_many lock count from 4N to 2N (재검토)

커밋: c7a90bfa0f7e7cf7b907498df9a780e31e75ca34 + working-tree 변경 제목: [Perf][RawBlock] Reduce put_many lock count from 4N to 2N 브랜치: perf/reduce-lock-put-many 작성자: Nayeon Kim nayeoni.kim@samsung.com 일자: 2026-06-02 리뷰 모드: /code-review --xhigh (9-angle finder × 1-vote verify × sweep) 선행 리뷰: cr-5a27732f-rawblock-put-many-lock-coalesce.md

변경 요약​

선행 리뷰 (5a27732f) 의 후속 조치가 working-tree 에 적용된 상태에서 재검토. 적용된 변경:

  • put_many 의 _last_io_ts = time.monotonic() 을 if success: 가드로 감쌈 (core.py:519-526).
  • inflight_io_count() docstring 을 ON window 에 맞춰 갱신 (core.py:398-406).
  • _CountingLock.__enter__ 가 self 를 반환하도록 정정 (tests/...:2300-2303).
  • 회귀 테스트 추가: test_put_many_does_not_stamp_last_io_ts_on_prep_failure (tests/...:2387-2421).

이번 재검토 범위는 선행 리뷰가 본 PR 내 보강을 권고했던 4 건이 정확히 적용되었는가 + 그 보강 자체가 새 결함을 만들지 않았는가 두 축.

정합성 (Correctness) — 부분 통과​

선행 리뷰의 F1/F3/F5 fix 와 신규 회귀 테스트는 의도대로 동작. 다만 if success: 가드의 docstring 정당화가 사실과 다르고, 선행 F4 가 유지된 read 측 비대칭이 working-tree 의 새 룰과 맞물려 더 명확한 결함이 됨, 그리고 신규 테스트 1건이 의도한 invariant 를 trivially 통과. 또한 public 시맨틱 변경이 외부 release-note 없이 진행 중.

Findings​

총 10건 (CONFIRMED 4, PLAUSIBLE 4 — 둘은 doc/altitude). 분류 표:

#finding파일:라인verdictseverity권장 액션
F1load_many_into 가 pread 실패에도 _last_io_ts 갱신 → put 측의 if success: 룰과 비대칭core.py:660CONFIRMEDmedium본 PR 내 수정 권고
F2_read_slot_header 가 read 실패에도 _last_io_ts 갱신 — F1 과 같은 결함core.py:1000CONFIRMEDmedium본 PR 내 수정 권고
F3if success: 가드의 코멘트가 "Match pre-L1 semantics" 라 거짓 주장. 실제로는 pwrite 실패 시 pre-L1 과 다름core.py:520-525CONFIRMEDmedium본 PR 내 코멘트 정정
F4test_checkpoint_idle_gate_blocks_during_put_many 가 dirty short-circuit 으로 trivially 통과 → idle gate invariant 미검증tests/...:2520CONFIRMEDmedium본 PR 내 테스트 보강
F5inflight_io_count() 시맨틱이 pwrite-only → allocate→commit 으로 넓어졌으나 외부 (report_status) 에 release-note 부재core.py:398, 773PLAUSIBLElow본 PR 내 release-note 한 줄
F6lock-protocol 계약이 3 곳 (put_many docstring, 인라인 코멘트, _write_one Note) 중복 → drift 위험core.py:446-452, 505-510, 936-943PLAUSIBLElow본 PR 내 통합 권고
F7_inflight_io_count accounting 이 3 호출자 (put_many, load_many_into, _read_slot_header) 에 inline 중복 → context-manager helper 화 권고core.py:511, 609, 992PLAUSIBLEinfo별도 cleanup PR (F1/F2 와 함께 처리 권장)
F8BlockingDevice 클래스 + monkeypatch.setitem 보일러플레이트 3건 중복tests/...:2436-2450, 2492-2506PLAUSIBLElow별도 cleanup PR
F9_CountingLock 가 post-init core._lock = ... swap 으로 install — meta_enable_periodic=False 가정에 의존tests/...:2342PLAUSIBLEinfo본 PR 내 코멘트 보강 또는 별도 cleanup
F10(보류) inflight is None 분기, _rawdev lazy init race, _closed TOCTOU — 모두 verifier 단계 REFUTED, drop—REFUTED—drop

severity 기준: 선행 리뷰와 동일 — high = 회귀, medium = 계약/표준 위반, low = 정리, info = 코멘트 수준.

즉시 조치 권고 (본 PR 내 보강)​

F1 / F2 — read 측 _last_io_ts 비대칭 정리 (medium) [CONFIRMED]​

  • 현상: put_many 는 working-tree 에서 if success: 가드로 prep/pwrite 실패 시 _last_io_ts 를 갱신하지 않도록 바뀌었음. 그러나
    • load_many_into (core.py:657-660) 는 finally 에서 _inflight_io_count -= 1 후 무조건 _last_io_ts = time.monotonic() 호출.
    • _read_slot_header (core.py:997-1000) 도 동일.
  • 근거 인용: 
    # core.py:657-660
    finally:
        with self._lock:
            self._inflight_io_count -= 1
            self._last_io_ts = time.monotonic()
    # core.py:997-1000
    finally:
        with self._lock:
            self._inflight_io_count -= 1
            self._last_io_ts = time.monotonic()
    반면 put_many 는: 
    # core.py:519-526
    if success:
        # Match pre-L1 semantics: only stamp `_last_io_ts` when
        # an actual device write completed.
        self._last_io_ts = time.monotonic()
  • 트리거: 잘못된 offset 으로 _read_slot_header 가 반복 호출되거나 (_validate_loaded_entries 같은 recovery 경로), load_many_into 의 pread 가 transient OSError 로 반복 실패하는 워크로드.
  • 결과: 매 실패마다 _last_io_ts 가 advance → checkpoint idle gate 가 meta_idle_quiet_ms 만큼씩 밀림. 선행 F1 가 put 측에서 막고자 했던 바로 그 시나리오가 read 측에 그대로 살아 있음.
  • 제안 픽스: load_many_into 의 success_any = any(results) (또는 한 건도 성공하면 stamp), _read_slot_header 는 header decode 가 정상이면 stamp 하는 식으로 put_many 와 일관된 룰 적용. 또는 F7 의 helper 로 통합 시 룰을 contract 안에 박아 넣음.
  • 이 PR 에 포함: F1, F2 와 F3 가 모두 같은 root (idle gate 룰의 부분 적용) 라 묶어서 처리 권장.

F3 — if success: 가드 코멘트 거짓 주장 정정 (medium) [CONFIRMED]​

  • 현상: working-tree 의 put_many finally 가드 옆 코멘트 (core.py:520-525):

    Match pre-L1 semantics: only stamp _last_io_ts when an actual device write completed.

  • 근거: pre-L1 _write_one 은 inner try/finally 안에서 _last_io_ts 를 갱신했음. 분기별 실제 동작:

    실패 시점pre-L1 stamp?post-L1 + if success: stamp?
    _prepare_write_payload raise (outer except 로 빠짐)NO (inner try 미진입)NO (success=False)
    pwrite raise (inner finally 가 stamp)YESNO (success=False)
    양쪽 모두 성공YESYES

    prep-실패에서는 일치하나, pwrite-실패에서는 새 동작이 pre-L1 보다 엄격함 (덜 자주 stamp). 코멘트의 "match" 주장은 사실과 다름.

  • 제안 픽스 (택1):

    • (A) 코멘트를 정확히 고쳐쓰기. 예:

      "Stamp only on full success. Pre-L1 stamped on pwrite failures too, but that path is rare and the stricter rule prevents transient pwrite errors from delaying checkpoint."

    • (B) pre-L1 일치를 진짜 원하면 가드를 if success or attempted_pwrite: 같은 형태로 분리 — 실용성 낮음, (A) 권장.

  • 이 PR 에 포함: 코멘트 수정만 — scope 작음.

F4 — test_checkpoint_idle_gate_blocks_during_put_many 보강 (medium) [CONFIRMED]​

  • 현상: 신규 테스트는 core._checkpoint_once(force=False) is False 로 idle gate 가 닫혀있음을 검증한다고 docstring 에 명시. 그러나 assertion 시점에 put 은 _write_one 의 pwrite 안에서 block 중 → allocate-lock 만 수행됨 → _meta_dirty_total == _meta_persisted == 0.
  • 근거 인용: 
    # core.py:1227-1238
    with self._lock:
        dirty = self._meta_dirty_total > self._meta_persisted
        idle_ok = self._inflight_io_count == 0 and (
            time.monotonic() - self._last_io_ts
        ) >= (self.meta_idle_quiet_ms / 1000.0)

    if not dirty:
        return False                       # ← 여기서 short-circuit
    if not force and not idle_ok:
        return False                       # ← 여기 도달 못 함
    put 의 _meta_dirty_total += 1 은 commit-lock 안 (core.py:532, core.py:539) 에서만 일어나는데, 테스트는 commit-lock 진입 전에 assert 를 실행. 따라서 _checkpoint_once 는 not dirty 분기로 빠지며 idle gate 자체는 평가도 되지 않음.
  • 결과: idle gate 가 broken 이라도 (예: idle_ok = True 로 잘못 됐어도) 테스트는 통과함. 의도된 invariant 미검증.
  • 제안 픽스 (택1):
    • (A) 테스트 시작 시 core.put_many(precursor_keys, precursor_objs) 을 한 번 성공시켜 _meta_dirty_total > _meta_persisted 만든 뒤, BlockingDevice 로 두 번째 put 을 막고 assertion.
    • (B) core._meta_dirty_total = core._meta_persisted + 1 로 직접 조작 (private state poke, 선행 F2 와 같은 SLF 위반 — 정당화 코멘트 필요).
    • → (A) 권장. 첫 put 의 commit 도 BlockingDevice 와 충돌하지 않게 하려면 BlockingDevice 의 started/release 를 N 번째 호출에만 적용하는 단순 카운터 추가.
  • 이 PR 에 포함: 테스트 분량 ~10 줄 증가, value 큼.

F5 — inflight_io_count() 외부 시맨틱 변경 명시 (low) [PLAUSIBLE]​

  • 현상: inflight_io_count() (core.py:398-406) docstring 은 ON window 명시로 갱신되었으나, 같은 값이 report_status() (core.py:773) 의 metadata_dirty_total 와 별개로 노출되는 publishing 면에는 변경 노트가 없음.
  • 제안 픽스: report_status() 의 dict key 가 inflight_io_count 라면 README 또는 변경 문서 §3.3 에 ON window 가 allocate→commit 으로 넓어진 점을 한 줄 추가. 외부 모니터링이 이 값을 "active raw I/O" 로 해석 중이라면 그 의미가 약간 느슨해진 점만 명시.
  • 이 PR 에 포함: README/CHANGELOG 한 줄 — scope 미미.

F6 — lock-protocol 계약 중복 통합 (low) [PLAUSIBLE]​

  • 현상: 동일 contract 가 세 곳에 서술됨 — put_many docstring (core.py:446-452), put_many 인라인 코멘트 (core.py:505-510), _write_one Note: 블록 (core.py:936-943).
  • 제안 픽스 (택1):
    • (A) put_many docstring 만 권위 (authoritative) 로 두고, _write_one Note: 는 한 줄 cross-ref 로 축약 (예: "See put_many for the lock-accounting contract that this method depends on.").
    • (B) docs/design/v1/storage_backend/raw_block/lock_protocol.md 신규 작성, 양쪽이 그쪽으로 link.
    • → (A) 권장. (B) 는 분량 크고 본 PR scope 외.
  • 이 PR 에 포함: 한 함수의 docstring 축약 — 1 줄 이내 패치.

후속 PR 후보 (본 PR scope 외)​

F7 — _inflight_io_count accounting helper 화 (info) [PLAUSIBLE]​

  • 현상: 동일 ritual (+= 1 → I/O → -= 1 + 조건부 _last_io_ts stamp) 이 put_many (라인 511/518), load_many_into (609/659), _read_slot_header (992/999) 세 곳에 inline 중복.
  • 권고: F1/F2 정리와 함께 _inflight_io_window(success: bool) 같은 context manager 로 통합. contract 가 helper 안에 박혀 신규 I/O 경로 추가 시 룰 누락 방지.
  • 이 PR 외 사유: F1/F2 정리만 본 PR 에서 끝내고, helper 화는 read 경로까지 손대는 큰 변경 — 선행 F4 후속 PR 과 묶기.

F8 — 테스트 device 보일러플레이트 통합 (low) [PLAUSIBLE]​

  • 현상: BlockingDevice 클래스 + monkeypatch.setitem(sys.modules, "lmcache_rust_raw_block_io", types.SimpleNamespace(...)) 가 신규 테스트 3건에 동일하게 복붙됨 (tests/...:2436-2450, tests/...:2492-2506 + _install_fake_raw_block_device).
  • 권고: _install_fake_raw_block_device 가 device factory 를 받도록 확장하고, BlockingDevice 와 FailingDevice 를 module-level fixture 로 공유. 약 20-30 줄 정리.
  • 이 PR 외 사유: 분량 작지만 본 PR 의 perf 변경과 무관 — 별도 cleanup PR.

F9 — _CountingLock install 패턴 (info) [PLAUSIBLE]​

  • 현상: core._lock = counting 의 post-init swap 은 __init__ 시점의 lock 획득을 모두 놓침. 현재는 _make_raw_block_core 가 meta_enable_periodic=False 라 안전하나, 향후 __init__ 에 lock 을 잡는 다른 경로가 추가되면 silent 하게 count 가 부풀어 오를 수 있음.
  • 권고 (택1):
    • (A) _CountingLock install 옆에 한 줄 코멘트 추가 — "depends on meta_enable_periodic=False 로 background lock acquirer 가 없음".
    • (B) RawBlockCore.__init__ 에 lock factory injection — 변경 표면 큼.
    • → (A) 가 작고 충분.
  • 이 PR 외 사유: F2 (선행) 와 같은 layer — 별도 정리 PR 또는 본 PR 에서 1 줄 코멘트만 추가.

위험 / 호환성 평가​

  • on-disk format: 미변경 (선행 리뷰와 동일).
  • public API: put_many / _write_one 시그니처 미변경. inflight_io_count() 반환 타입 미변경, 시맨틱은 약간 넓어짐 — F5 참조.
  • 스레드 안전성: F1/F2 의 비대칭은 안전성이 아닌 idle gate 정확도 문제. 카운터 ±1 lock 보호는 유지됨.
  • 롤백: working-tree 변경 5 줄 (가드 + 코멘트) 단독으로도 revert 가능.
  • 회귀 영향:
    • put 측 (working-tree 가드) → 선행 F1 픽스로 prep 실패 분기는 pre-L1 동작 복원. pwrite 실패 분기는 의도적으로 더 엄격함 (F3 코멘트만 정정).
    • read 측 (F1/F2) → 여전히 pre-L1 의 unconditional stamp 패턴 유지. 읽기 실패가 빈번한 워크로드에서 checkpoint 지연 가능.

리뷰 메타​

  • 도구: /code-review --xhigh (9-angle finder × 1-vote verify × sweep).
  • 단계 요약:
    • Phase 0: git diff @{upstream}...HEAD + git diff HEAD 로 committed + working-tree diff 모두 확보. 선행 리뷰 (cr-5a27732f-rawblock-put-many-lock-coalesce.md) 의 fix 적용 상태 그대로.
    • Phase 1: 9 angles 병렬 (A 라인스캔 / B 제거동작 / C 호출자 / D Python pitfall / E proxy / Reuse / Simplification / Efficiency / Altitude).
    • Phase 2: 6 verifier 병렬 (1 vote, CONFIRMED/PLAUSIBLE/REFUTED). REFUTED 3건 drop.
    • Phase 3: sweep finder — 추가 신규 결함 0건.
  • 입력 commit: c7a90bfa + working-tree
  • verdict 분포: CONFIRMED 4 / PLAUSIBLE 5 / REFUTED 3 (drop) / sweep 0.
  • 본 리뷰는 read-only — 코드 변경 없음.

REFUTED (참고)​

후보사유
_inflight.pop(...) is None 로 slot leak 가능성_inflight mutator 는 put_many.insert/pop 와 delete_many.set-canceled (pop 아님) 뿐 — 동일 invocation 내 insert→pop 보장. None 분기 unreachable.
_rawdev() lazy init race__init__ 의 _ensure_capacity_and_layout 가 _rawdev() 를 동기 호출 → 인스턴스 공유 시점에 self._raw 이미 non-None. 경쟁 윈도우 없음.
_closed TOCTOUpre-L1 에도 동일 패턴 (if self._closed: break lock 밖) 존재. 본 PR 도입 아님 + _write_one 의 except Exception 으로 graceful fail.