DAOS-18906 dfs: GIT (Global Inode Table) entries and associated funct… by sherintg · Pull Request #18211 · daos-stack/daos

sherintg · 2026-05-11T08:19:51Z

DAOS-18906 dfs: GIT (Global Inode Table) entries and associated functions

The Global Inode Table (GIT) stores per-inode metadata using the same layout as a directory entry (dentry) with one additional field: link_cnt. Dentry layout is unchanged at 12 fields (END_IDX); GIT entries extend it to 13 (END_GIT_IDX). INODE_AKEYS is bumped from 12 to 13 only to size the shared sg_iovs[] array large enough for GIT entries.

dfs_internal.h: add LINK_CNT_IDX / END_GIT_IDX layout constants; add link_cnt field to struct dfs_entry; bump INODE_AKEYS 12→13; declare git_{fetch,insert}_entry(), git_update_link_cnt(), git_copy_xattr().
common.c: refactor fetch_entry() / insert_entry() behind static helpers fetch_entry_common() / insert_entry_common(), parameterised by include_link_cnt, so both dentry and GIT paths share the same I/O logic. When include_link_cnt=true the recx extends to END_GIT_IDX and link_cnt is included in the iov set; symlink handling is skipped for GIT. fetch_entry() sets link_cnt=1 for normal dentries. Implement git_{fetch,insert}_entry(), git_update_link_cnt(), and git_copy_xattr() on top of these helpers.
daos_mw_fi.c: sync local INODE_AKEYS copy to 13.
dfs_unit_test.c: add DFS_UNIT_TEST29–32 covering GIT insert/fetch round-trip, insert-once (EEXIST), link_cnt inc/dec/underflow, and xattr copy.

Allow-unstable-test: true

Steps for the author:

Commit message follows the guidelines.
Appropriate Features or Test-tag pragmas were used.
Appropriate Functional Test Stages were run.
At least two positive code reviews including at least one code owner from each category referenced in the PR.
Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

Gatekeeper requested (daos-gatekeeper added as a reviewer).

…ions The Global Inode Table (GIT) stores per-inode metadata using the same layout as a directory entry (dentry) with one additional field: link_cnt. Dentry layout is unchanged at 12 fields (END_IDX); GIT entries extend it to 13 (END_GIT_IDX). INODE_AKEYS is bumped from 12 to 13 only to size the shared sg_iovs[] array large enough for GIT entries. - dfs_internal.h: add LINK_CNT_IDX / END_GIT_IDX layout constants; add link_cnt field to struct dfs_entry; bump INODE_AKEYS 12→13; declare git_{fetch,insert}_entry(), git_update_link_cnt(), git_copy_xattr(). - common.c: refactor fetch_entry() / insert_entry() behind static helpers fetch_entry_common() / insert_entry_common(), parameterised by include_link_cnt, so both dentry and GIT paths share the same I/O logic. When include_link_cnt=true the recx extends to END_GIT_IDX and link_cnt is included in the iov set; symlink handling is skipped for GIT. fetch_entry() sets link_cnt=1 for normal dentries. Implement git_{fetch,insert}_entry(), git_update_link_cnt(), and git_copy_xattr() on top of these helpers. - daos_mw_fi.c: sync local INODE_AKEYS copy to 13. - dfs_unit_test.c: add DFS_UNIT_TEST29–32 covering GIT insert/fetch round-trip, insert-once (EEXIST), link_cnt inc/dec/underflow, and xattr copy. Allow-unstable-test: true Signed-off-by: Sherin T George <sherin-t.george@hpe.com>

github-actions · 2026-05-11T08:20:43Z

Ticket title is 'GIT (Global Inode Table) entries and associated functions'
Status is 'Open'
https://daosio.atlassian.net/browse/DAOS-18906

daosbuild3 · 2026-05-11T18:38:47Z

Test stage Functional on EL 9 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18211/1/testReport/

knard38

Some small remarks not blocking the PR.
However, if I am correct, the remarks on the git_update_link_cnt() should probably be fixed before landing.

knard38 · 2026-05-12T07:32:37Z

 		D_GOTO(out, rc = 0);
 	} else if (rc) {
-		D_ERROR("Failed to fetch entry %s " DF_RC "\n", name, DP_RC(rc));
+		D_ERROR("Failed to fetch entry " DF_RC "\n", DP_RC(rc));


Nit, the refactoring drops the entry name from the error message, which was useful for log triage.

Would you consider re-logging the name in the fetch_entry() and git_fetch_entry() wrappers on failure, or passing an optional const char *label into the common helpers (NULL for GIT paths)?

No issue to keep as-is if you prefer a minimal helper signature.

I have moved this error message print to the fetch_entry() and git_fetch_entry() function respectively. In get_fetch_entry(), I print the oid instead of name in the error message.

knard38 · 2026-05-12T08:55:40Z

+}
+
+int
+git_update_link_cnt(daos_handle_t git_oh, daos_handle_t th, struct dfs_entry *entry, int delta)


From my understanding, git_update_link_cnt() derives the GIT dkey from entry->oid while git_fetch_entry() and git_insert_entry() both take an explicit daos_obj_id_t *oid.
Would you consider making the signature consistent by adding an explicit oid parameter here too?

int git_update_link_cnt(daos_handle_t git_oh, daos_handle_t th, daos_obj_id_t *oid, struct dfs_entry *entry, int delta);

This makes the API uniform and removes the implicit dependency on entry->oid being valid before the call.

I have changed the function. Instead of accepting delta, it accepts the value to be set and uses oid as the third parameter.

knard38 · 2026-05-12T08:57:30Z

 #define HLC_IDX            (SIZE_IDX + sizeof(daos_size_t))
 #define END_IDX            (HLC_IDX + sizeof(uint64_t))

+/** GIT (Global Index Table) inode layout extends the base inode with link_cnt */


Suggested change

/** GIT (Global Index Table) inode layout extends the base inode with link_cnt */

/** GIT (Global Inode Table) inode layout extends the base inode with link_cnt */

fixed in all locations

knard38 · 2026-05-12T09:01:39Z

 		/** don't log error if conditional failed */
 		if (rc != -DER_EXIST && rc != -DER_NO_PERM)
-			D_ERROR("Failed to insert entry '%s', " DF_RC "\n", name, DP_RC(rc));
+			D_ERROR("Failed to insert entry, " DF_RC "\n", DP_RC(rc));


Nit, same concerns as for fetch_entry_common()

I have moved this error message print to the insert_entry() and git_insert_entry() function respectively. In get_insert_entry(), I print the oid instead of name in the error message.

knard38 · 2026-05-12T09:24:25Z

-	/** add the symlink as a separate akey */
-	if (S_ISLNK(entry->mode)) {
+	/** add the symlink as a separate akey (dentry only, never in GIT) */
+	if (!include_link_cnt && S_ISLNK(entry->mode)) {


From my understanding, the !include_link_cnt guard in the symlink condition is redundant since git_insert_entry() already asserts !S_ISLNK(entry->mode) before reaching this point, making the include_link_cnt=true + symlink combination impossible.

Would you consider replacing it with an explicit assert to make the invariant visible and catch any future regression?

D_ASSERT(!include_link_cnt || !S_ISLNK(entry->mode)); /** add the symlink as a separate akey (dentry only, never in GIT) */ if (S_ISLNK(entry->mode)) {

If you prefer to keep the combined if, a brief comment explaining the redundancy would help future readers.

No issue to keep as-is if you disagree.

As per the review comments from @mchaarawi , I have converted the assert into return EIO.

mchaarawi · 2026-05-12T13:25:21Z

+/** GIT (Global Index Table) inode layout extends the base inode with link_cnt */
+#define LINK_CNT_IDX           END_IDX
+#define END_GIT_IDX            (LINK_CNT_IDX + sizeof(uint64_t))


given that this is going to be applied for all new layout containers, it would be better to just update END_IDX to this new value, and replace the old one with END_L3_IDX.

mchaarawi · 2026-05-12T13:26:18Z

 int
 remove_entry(dfs_t *dfs, daos_handle_t th, daos_handle_t parent_oh, const char *name, size_t len,
 	     struct dfs_entry entry);
+/** GIT (Global Index Table) entry operations */


Index -> inode

fixed in all locations.

mchaarawi · 2026-05-12T13:28:42Z

+	if (rc)
+		return rc;
+	if (!exists) {
+		D_ERROR("Entry not found in GIT\n");


you should not log errors for ENOENT. this should be debug

Fixed. Removed the error message. The caller will act on ENOENT accordingly.

mchaarawi · 2026-05-12T13:36:37Z

+
+	if (oid == NULL || entry == NULL)
+		return EINVAL;
+	D_ASSERT(!S_ISLNK(entry->mode));


should not assert here and just return EIO

mchaarawi · 2026-05-12T13:41:49Z

+static int
+insert_entry_common(daos_handle_t oh, daos_handle_t th, daos_key_t *dkey, bool include_link_cnt,
+		    uint64_t flags, struct dfs_entry *entry)


just a nit to the function input of include_link_cnt
maybe instead of include_link_cnt, we call this insert_in_git since that implies the link cnt to be added?

Changed the bool variable name to is_git_entry

mchaarawi · 2026-05-12T13:50:51Z

+int
+fetch_entry(dfs_layout_ver_t ver, daos_handle_t oh, daos_handle_t th, const char *name, size_t len,
+	    bool fetch_sym, bool *exists, struct dfs_entry *entry, int xnr, char *xnames[],
+	    void *xvals[], daos_size_t *xsizes)
+{
+	daos_iod_t  iod;
+	d_sg_list_t sgl;
+	daos_key_t  dkey;
+	int         rc;


i would rather to have one fetch common function rather than this which does the first part of the entry fetch in the commpn and then uses another set of stack variables (iod, sgl) for the follow on symlink fetch.

mchaarawi · 2026-05-12T13:54:55Z

+	recxs[0].rx_idx = LINK_CNT_IDX;
+	recxs[0].rx_nr  = sizeof(uint64_t);
+	recxs[1].rx_idx = CTIME_IDX;
+	recxs[1].rx_nr  = sizeof(uint64_t);
+	recxs[2].rx_idx = CTIME_NSEC_IDX;
+	recxs[2].rx_nr  = sizeof(uint64_t);


for optimization purposes, make the recx in order

mchaarawi · 2026-05-12T13:56:41Z

+		return EINVAL;
+	}
+
+	entry->link_cnt += delta;


question about how this will be used.
where is the entry coming from? a separate fetch?
I assume the fetch and this git update will be using the same transaction when this is used?

Yes, During link creation and link removal, the entry will be first fetched from GIT and then either link_cnt is updated of the GIT entry is punched based on the link count value. I have changed the definition of this function now. Now it takes a value to be set as link_cnt.

…ions Addressed Review comments. Allow-unstable-test: true Signed-off-by: Sherin T George <sherin-t.george@hpe.com>

sherintg requested review from johannlombardi, knard38 and mchaarawi May 11, 2026 10:21

sherintg marked this pull request as ready for review May 11, 2026 10:21

sherintg requested review from a team as code owners May 11, 2026 10:21

knard38 reviewed May 12, 2026

View reviewed changes

mchaarawi requested changes May 12, 2026

View reviewed changes

DAOS-18906 dfs: GIT (Global Inode Table) entries and associated funct…

9a1bada

…ions Addressed Review comments. Allow-unstable-test: true Signed-off-by: Sherin T George <sherin-t.george@hpe.com>

	/** GIT (Global Index Table) inode layout extends the base inode with link_cnt */
	/** GIT (Global Inode Table) inode layout extends the base inode with link_cnt */

Conversation

sherintg commented May 11, 2026

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions Bot commented May 11, 2026

Uh oh!

daosbuild3 commented May 11, 2026

Uh oh!

knard38 left a comment

Choose a reason for hiding this comment

Uh oh!

knard38 May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherintg May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

knard38 May 12, 2026 •

edited

Loading

sherintg May 13, 2026 •

edited

Loading