From: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
To: Adarsh Das <adarshdas950@gmail.com>
Cc: <ntfs3@linux.de>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] fs/ntfs3: resolve compare function in public index APIs
Date: Tue, 24 Feb 2026 19:50:38 +0100 [thread overview]
Message-ID: <19dd94a6-e290-4575-9624-eeee3c64df57@paragon-software.com> (raw)
In-Reply-To: <20260208083904.63963-1-adarshdas950@gmail.com>
On 2/8/26 09:39, Adarsh Das wrote:
> Previously the comparator was stored in struct ntfs_index and
> used by low-level helpers such as hdr_find_e(). This creates
> a dependency on index state in private helpers.
>
> Resolve the compare function in the public index APIs and pass
> it explicitly to internal helpers.
>
> This should make the ownership of the comparator explicit and keeps
> low-level index code independent of index-root policy.
>
> This also resolves the TODO comment about dropping the stored
> comparator from struct ntfs_index.
>
> Signed-off-by: Adarsh Das <adarshdas950@gmail.com>
> ---
> fs/ntfs3/index.c | 76 ++++++++++++++++++++++++++++++----------------
> fs/ntfs3/ntfs_fs.h | 5 +--
> 2 files changed, 50 insertions(+), 31 deletions(-)
>
> diff --git a/fs/ntfs3/index.c b/fs/ntfs3/index.c
> index 7157cfd70fdc..f04a8e13f8b8 100644
> --- a/fs/ntfs3/index.c
> +++ b/fs/ntfs3/index.c
> @@ -716,10 +716,10 @@ static bool fnd_is_empty(struct ntfs_fnd *fnd)
> */
> static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
> const struct INDEX_HDR *hdr, const void *key,
> - size_t key_len, const void *ctx, int *diff)
> + size_t key_len, const void *ctx, int *diff,
> + NTFS_CMP_FUNC cmp)
> {
> struct NTFS_DE *e, *found = NULL;
> - NTFS_CMP_FUNC cmp = indx->cmp;
> int min_idx = 0, mid_idx, max_idx = 0;
> int diff2;
> int table_size = 8;
> @@ -729,9 +729,6 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
> u32 total = le32_to_cpu(hdr->total);
> u16 offs[128];
>
> - if (unlikely(!cmp))
> - return NULL;
> -
> fill_table:
> if (end > total)
> return NULL;
> @@ -802,7 +799,8 @@ static struct NTFS_DE *hdr_find_e(const struct ntfs_index *indx,
> static struct NTFS_DE *hdr_insert_de(const struct ntfs_index *indx,
> struct INDEX_HDR *hdr,
> const struct NTFS_DE *de,
> - struct NTFS_DE *before, const void *ctx)
> + struct NTFS_DE *before, const void *ctx,
> + NTFS_CMP_FUNC cmp)
> {
> int diff;
> size_t off = PtrOffset(hdr, before);
> @@ -825,7 +823,7 @@ static struct NTFS_DE *hdr_insert_de(const struct ntfs_index *indx,
> }
> /* No insert point is applied. Get it manually. */
> before = hdr_find_e(indx, hdr, de + 1, le16_to_cpu(de->key_size), ctx,
> - &diff);
> + &diff, cmp);
> if (!before)
> return NULL;
> off = PtrOffset(hdr, before);
> @@ -917,10 +915,6 @@ int indx_init(struct ntfs_index *indx, struct ntfs_sb_info *sbi,
>
> init_rwsem(&indx->run_lock);
>
> - indx->cmp = get_cmp_func(root);
> - if (!indx->cmp)
> - goto out;
> -
> return 0;
>
> out:
> @@ -1142,6 +1136,7 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
> int err;
> struct NTFS_DE *e;
> struct indx_node *node;
> + NTFS_CMP_FUNC cmp;
>
> if (!root)
> root = indx_get_root(&ni->dir, ni, NULL, NULL);
> @@ -1151,10 +1146,16 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
> return -EINVAL;
> }
>
> + cmp = get_cmp_func(root);
> + if (unlikely(!cmp)) {
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
> +
> /* Check cache. */
> e = fnd->level ? fnd->de[fnd->level - 1] : fnd->root_de;
> if (e && !de_is_last(e) &&
> - !(*indx->cmp)(key, key_len, e + 1, le16_to_cpu(e->key_size), ctx)) {
> + !(*cmp)(key, key_len, e + 1, le16_to_cpu(e->key_size), ctx)) {
> *entry = e;
> *diff = 0;
> return 0;
> @@ -1164,7 +1165,7 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
> fnd_clear(fnd);
>
> /* Lookup entry that is <= to the search value. */
> - e = hdr_find_e(indx, &root->ihdr, key, key_len, ctx, diff);
> + e = hdr_find_e(indx, &root->ihdr, key, key_len, ctx, diff, cmp);
> if (!e)
> return -EINVAL;
>
> @@ -1184,7 +1185,7 @@ int indx_find(struct ntfs_index *indx, struct ntfs_inode *ni,
>
> /* Lookup entry that is <= to the search value. */
> e = hdr_find_e(indx, &node->index->ihdr, key, key_len, ctx,
> - diff);
> + diff, cmp);
> if (!e) {
> put_indx_node(node);
> return -EINVAL;
> @@ -1583,7 +1584,7 @@ static int indx_add_allocate(struct ntfs_index *indx, struct ntfs_inode *ni,
> static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> const struct NTFS_DE *new_de,
> struct NTFS_DE *root_de, const void *ctx,
> - struct ntfs_fnd *fnd, bool undo)
> + struct ntfs_fnd *fnd, bool undo, NTFS_CMP_FUNC cmp)
> {
> int err = 0;
> struct NTFS_DE *e, *e0, *re;
> @@ -1624,7 +1625,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> if ((undo || asize + ds_root < sbi->max_bytes_per_attr) &&
> mi_resize_attr(mi, attr, ds_root)) {
> hdr->total = cpu_to_le32(hdr_total + ds_root);
> - e = hdr_insert_de(indx, hdr, new_de, root_de, ctx);
> + e = hdr_insert_de(indx, hdr, new_de, root_de, ctx, cmp);
> WARN_ON(!e);
> fnd_clear(fnd);
> fnd->root_de = e;
> @@ -1765,7 +1766,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> * Now root is a parent for new index buffer.
> * Insert NewEntry a new buffer.
> */
> - e = hdr_insert_de(indx, hdr, new_de, NULL, ctx);
> + e = hdr_insert_de(indx, hdr, new_de, NULL, ctx, cmp);
> if (!e) {
> err = -EINVAL;
> goto out_put_n;
> @@ -1795,7 +1796,7 @@ static int indx_insert_into_root(struct ntfs_index *indx, struct ntfs_inode *ni,
> static int
> indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni,
> struct INDEX_ROOT *root, const struct NTFS_DE *new_de,
> - const void *ctx, int level, struct ntfs_fnd *fnd)
> + const void *ctx, int level, struct ntfs_fnd *fnd, NTFS_CMP_FUNC cmp)
> {
> int err;
> const struct NTFS_DE *sp;
> @@ -1812,7 +1813,7 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni,
>
> /* Try the most easy case. */
> e = fnd->level - 1 == level ? fnd->de[level] : NULL;
> - e = hdr_insert_de(indx, hdr1, new_de, e, ctx);
> + e = hdr_insert_de(indx, hdr1, new_de, e, ctx, cmp);
> fnd->de[level] = e;
> if (e) {
> /* Just write updated index into disk. */
> @@ -1889,12 +1890,12 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni,
> * (depending on sp <=> new_de).
> */
> hdr_insert_de(indx,
> - (*indx->cmp)(new_de + 1, le16_to_cpu(new_de->key_size),
> + (*cmp)(new_de + 1, le16_to_cpu(new_de->key_size),
> up_e + 1, le16_to_cpu(up_e->key_size),
> ctx) < 0 ?
> hdr2 :
> hdr1,
> - new_de, NULL, ctx);
> + new_de, NULL, ctx, cmp);
>
> indx_mark_used(indx, ni, new_vbn >> indx->idx2vbn_bits);
>
> @@ -1909,14 +1910,14 @@ indx_insert_into_buffer(struct ntfs_index *indx, struct ntfs_inode *ni,
> */
> if (!level) {
> /* Insert in root. */
> - err = indx_insert_into_root(indx, ni, up_e, NULL, ctx, fnd, 0);
> + err = indx_insert_into_root(indx, ni, up_e, NULL, ctx, fnd, 0, cmp);
> } else {
> /*
> * The target buffer's parent is another index buffer.
> * TODO: Remove recursion.
> */
> err = indx_insert_into_buffer(indx, ni, root, up_e, ctx,
> - level - 1, fnd);
> + level - 1, fnd, cmp);
> }
>
> if (err) {
> @@ -1950,6 +1951,7 @@ int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni,
> struct NTFS_DE *e;
> struct ntfs_fnd *fnd_a = NULL;
> struct INDEX_ROOT *root;
> + NTFS_CMP_FUNC cmp;
>
> if (!fnd) {
> fnd_a = fnd_get();
> @@ -1966,6 +1968,12 @@ int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni,
> goto out;
> }
>
> + cmp = get_cmp_func(root);
> + if (unlikely(!cmp)) {
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
> +
> if (fnd_is_empty(fnd)) {
> /*
> * Find the spot the tree where we want to
> @@ -1989,13 +1997,13 @@ int indx_insert_entry(struct ntfs_index *indx, struct ntfs_inode *ni,
> * new entry into it.
> */
> err = indx_insert_into_root(indx, ni, new_de, fnd->root_de, ctx,
> - fnd, undo);
> + fnd, undo, cmp);
> } else {
> /*
> * Found a leaf buffer, so we'll insert the new entry into it.
> */
> err = indx_insert_into_buffer(indx, ni, root, new_de, ctx,
> - fnd->level - 1, fnd);
> + fnd->level - 1, fnd, cmp);
> }
>
> out:
> @@ -2288,6 +2296,7 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni,
> u32 e_size, root_size, new_root_size;
> size_t trim_bit;
> const struct INDEX_NAMES *in;
> + NTFS_CMP_FUNC cmp;
>
> fnd = fnd_get();
> if (!fnd) {
> @@ -2307,6 +2316,12 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni,
> goto out;
> }
>
> + cmp = get_cmp_func(root);
> + if (unlikely(!cmp)) {
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
> +
> /* Locate the entry to remove. */
> err = indx_find(indx, ni, root, key, key_len, ctx, &diff, &e, fnd);
> if (err)
> @@ -2372,9 +2387,9 @@ int indx_delete_entry(struct ntfs_index *indx, struct ntfs_inode *ni,
> err = level ? indx_insert_into_buffer(indx, ni, root,
> re, ctx,
> fnd->level - 1,
> - fnd) :
> + fnd, cmp) :
> indx_insert_into_root(indx, ni, re, e,
> - ctx, fnd, 0);
> + ctx, fnd, 0, cmp);
> kfree(re);
>
> if (err)
> @@ -2668,6 +2683,7 @@ int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi,
> struct INDEX_ROOT *root;
> struct mft_inode *mi;
> struct ntfs_index *indx = &ni->dir;
> + NTFS_CMP_FUNC cmp;
>
> fnd = fnd_get();
> if (!fnd)
> @@ -2679,6 +2695,12 @@ int indx_update_dup(struct ntfs_inode *ni, struct ntfs_sb_info *sbi,
> goto out;
> }
>
> + cmp = get_cmp_func(root);
> + if (unlikely(!cmp)) {
> + WARN_ON_ONCE(1);
> + return -EINVAL;
> + }
> +
> /* Find entry in directory. */
> err = indx_find(indx, ni, root, fname, fname_full_size(fname), sbi,
> &diff, &e, fnd);
> diff --git a/fs/ntfs3/ntfs_fs.h b/fs/ntfs3/ntfs_fs.h
> index a4559c9f64e6..63d3b76e7d06 100644
> --- a/fs/ntfs3/ntfs_fs.h
> +++ b/fs/ntfs3/ntfs_fs.h
> @@ -192,9 +192,6 @@ struct ntfs_index {
> /* read/write access to 'bitmap_run'/'alloc_run' while ntfs_readdir */
> struct rw_semaphore run_lock;
>
> - /*TODO: Remove 'cmp'. */
> - NTFS_CMP_FUNC cmp;
> -
> u8 index_bits; // log2(root->index_block_size)
> u8 idx2vbn_bits; // log2(root->index_block_clst)
> u8 vbn2vbo_bits; // index_block_size < cluster? 9 : cluster_bits
> @@ -378,7 +375,7 @@ struct ntfs_inode {
> */
> u8 mi_loaded;
>
> - /*
> + /*
> * Use this field to avoid any write(s).
> * If inode is bad during initialization - use make_bad_inode
> * If inode is bad during operations - use this field
Hello,
Queued for the next merge window, thanks.
Regards,
Konstantin
prev parent reply other threads:[~2026-02-24 18:50 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-08 8:39 Adarsh Das
2026-02-24 18:50 ` Konstantin Komarov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=19dd94a6-e290-4575-9624-eeee3c64df57@paragon-software.com \
--to=almaz.alexandrovich@paragon-software.com \
--cc=adarshdas950@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ntfs3@linux.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®