Re: [RFC PATCH v5] ntfs: serialize attribute-list replacement with lookups

[Cen Zhang]

Hi Hyunchul.

Thanks.

> Design A:
>   * Eliminates the raw pointer (ctx->al_entry) entirely from search_ctx.
>   * Instead, it tracks the cursor using 'al_cursor.off' (offset within
>     the buffer) and 'al_cursor.gen' (generation counter matching
>     base_ni->attr_list_gen).
>   * If the generation counter changes, the lookup detects this and
>     restarts safely using an exact search key (al_exact).
>   * Since we no longer carry raw pointers, attr_list_lock only needs to
>     be held briefly during in-memory list operations.
>   * To strictly maintain the lock order (runlist.lock -> attr_list_lock)
>     and prevent deadlocks, functions that previously held attr_list_lock
>     while calling runlist-locking operations must be refactored.
>   * Pros:
>     - Short-lived locks.
>     - Narrow lock scope makes it easy to maintain codes.
>   * Cons:
>     - More complex implementation.
>
> Design B (runlist.lock style):
>   * Retains ctx->al_entry (raw pointers), but manages attr_list_lock as
>     a top-level coarse-grained lock similar to runlist.lock.
>   * We must establish a strict global lock hierarchy: runlist.lock ->
>     attr_list_lock.
>   * Pros:
>     - Simpler implementation.
>   * Cons:
>     - Could reduce performance.
>     - Wide lock scope makes it difficult to maintain codes.
>     - May introduce a lot of '_nolock' helper functions to avoid
>       self-deadlock.
>
> Which approaches do you think would be better, or other approaches?


I think Design A is the better long-term direction.

Design B is simpler at first glance, but I think v5 already shows its main
problem.  As long as search_ctx keeps ctx->al_entry as a raw pointer into
base_ni->attr_list, the lock has to cover the whole lifetime of that pointer.
Then attr_list_lock starts to spread through many helper layers, and we need
more _nolock variants or nested-lock variants to avoid self-deadlocks and lock
ordering problems.  That makes the patch large and hard to maintain.

Design A looks more complex internally, but it avoids the root problem: the
search context would no longer carry a raw pointer into a replaceable buffer.
Using an offset plus generation counter, and restarting the lookup when the
generation changes, seems easier to reason about than keeping a wide
attr_list_lock scope across many NTFS paths.

> And please let me know if you would like me to implement it.

I have limited bandwidth at the moment, and you know this code much better
than I do.  So if you are willing to take the implementation side, I would
really appreciate it.  I will also help review and test it so we can move
this larger fix forward together. :)

Best regards,
Cen Zhang