DAOS-18989 dfs: add hardlink link/unlink support and mask internal mo… by sherintg · Pull Request #18356 · daos-stack/daos

sherintg · 2026-05-26T05:21:40Z

…de bit

Implement dfs_link for regular files and maintain hardlink metadata through GIT entries. Handle hardlink-aware unlink path, including xattr cleanup/migration behavior. Ensure stat/mode APIs expose external mode only, and add DFS unit coverage for link/remove workflows.

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).

github-actions · 2026-05-26T05:21:55Z

Ticket title is 'Add hardlink link/unlink support APIs and mask internal mode bit'
Status is 'In Progress'
https://daosio.atlassian.net/browse/DAOS-18989

…de bit Implement dfs_link for regular files and maintain hardlink metadata through GIT entries. Handle hardlink-aware unlink path, including xattr cleanup/migration behavior. Ensure stat/mode APIs expose external mode only, and add DFS unit coverage for link/remove workflows. Allow-unstable-test: true Signed-off-by: Sherin T George <sherin-t.george@hpe.com>

mchaarawi · 2026-05-26T10:58:42Z

+	}
+
+restart:
+	D_ASSERT(DFS_IS_HARDLINK(entry.mode));


entry is fetched from storage. if there is a data corruption issue, we hit this assertion, no?
assert in that case is unacceptable and an error (EIO) should be returned.

The assert is making sure that the caller calls this routine only if the entry points to a hardlink.

im still not clear why this should be an assert. correct me if wrong, but you are not setting entry.mode yourself. this is coming from fetch, no?

The assert here is to make sure that caller calls this routine only when entry.mode has hardlink bit set like the code below.

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)
{
...
if (DFS_IS_HARDLINK(entry.mode))
return remove_hardlink(dfs, th, parent_oh, name, len, entry);
/ Default remove entry code */

Also, I have now moved the assert to the very beginning of the function so that the assert is called only once.

I share @mchaarawi's concern here. From my understanding, entry.mode is read from storage via fetch_entry(), so a corrupted mode bit is a realistic scenario — not just a programming error. In that case a D_ASSERT would abort the daemon, which seems disproportionate.

Would it make sense to replace the assert with an explicit runtime guard returning EIO? Something like:

if (!DFS_IS_HARDLINK(entry.mode)) { D_ERROR("remove_hardlink called on non-hardlink entry (mode=0x%x)\n", entry.mode); return EIO; }

This would survive data-corruption scenarios gracefully and make the precondition explicit to future readers without aborting the process. No issue to keep the assert if you consider this a programming invariant that can only be violated by a bug in the caller — but a short comment explaining that assumption would help clarify the intent.

The expectation here is that the caller here should ensure that entry.mode has hardlink bit set before calling remove_hardlink(). I see similar logic used in other functions. For example open_dir() expects parent to be not null when ocreate flag is passed.

mchaarawi · 2026-05-26T11:05:50Z

 	args->stbuf->st_nlink = 1;
-	args->stbuf->st_mode  = op_args->entry.mode;
+	args->stbuf->st_mode  = DFS_EXTERNAL_MODE(op_args->entry.mode);


im confused here.. how can this be a hardlink but st_nlink still hardcoded to 1?
the entire stat and statx paths is not in limbo. i assume you will finish this in a follow on PR?

I have restricted this PR only to dfs_link(), dfs_remove() and internal mode bit. I will be creating subsequent new PRs for fetching the metadata with hardlinks, updating metadata with hardlinks, move/rename.

mchaarawi · 2026-05-26T11:16:23Z

+			D_ERROR("Missing GIT entry for file %s", obj->name);
+			D_GOTO(out, EIO);


so what if someone removed the entry between the first fetch and this fetch?
this is not an error anymore, and the transaction restart behavior would detect that i hope.
logging an EIO error here sounds wrong, but correct me if im wrong please.

i do not think believe this function should log any error on the transaction commit/restart path.

Thanks, you are right. Modified to return rc (ENOENT) in this case.

mchaarawi · 2026-05-26T11:18:32Z

+		link_entry = &src_entry;
+	}
+
+	rc = insert_entry(dfs->layout_v, parent->oh, th, name, len, 0, link_entry);


should this not use a conditional insert for insertion into the new parent?
can't you overwrite an existing entry there in this case?

Thanks. I had thought about this and had made the change in the PoC code, but missed this time. I will add negative test case too.

mchaarawi · 2026-05-26T11:21:12Z

+int
+dfs_link(dfs_t *dfs, dfs_obj_t *obj, dfs_obj_t *parent, const char *name, dfs_obj_t **new_obj,
+	 struct stat *stbuf)


this function could use a few code comments at every stage to explain the steps.

mchaarawi · 2026-05-26T11:24:59Z

+ * \param[in]	parent	Opened parent directory object where the new link is created.
+ *			If NULL, use root obj.
+ * \param[in]	name	Link name of the new hard link.


for clarity, please rename these to new_*

mchaarawi · 2026-05-26T11:29:02Z

+/*
+ * Delete all extended attributes stored under a directory entry.
+ * Enumerates every akey whose name starts with "x:" under the dkey
+ * identified by @name in directory object @oh, and punches each one.
+ */
+int
+xattr_delete_entry(daos_handle_t oh, const char *name, daos_handle_t th)


there seems to be no tests added for this path.
perhaps xattr support should be left to a follow on PR?

Added a test to validate delete of xattr from directory entry has happened. Rest of the xattr tests will be added in the next PR related to xattr set and get operations.

mchaarawi · 2026-05-26T11:31:31Z

+	uint64_t         new_link_cnt;
+	bool             local_tx = false;
+
+	if (!daos_handle_is_valid(th)) {


i thought tx will always be used in hardlinks case per the design

Since dfs_remove() is in performance path, I have restricted the use of tx only for hardlinks.

yes, but here we are in the hardlinks path here, so this should always be true?

th will be TX_NONE in remove_entry if use_dtx is not set. The above check ensure that dtx is opened if use_dtx is not set and the file getting removed is a hardlink.

mchaarawi · 2026-05-26T11:38:02Z

+/**
+ * Test dfs_link and dfs_remove covering the following steps:
+ *


some missing tests that i can think of:

linking into an existing file, directory, symlink (all cases must be tested and fail with EEXIST)

link with same directory under a different name

xattr testing, but sounds like you want to leave this to a different PR (I agree), but you added some xattr code in the implementation in this PR

error paths testing for dfs_link()

for later:

hardlink + rename interactions

chmod/chown with hardlinks are visible to all paths

mode masking is properly covered in all cases including readdirplus (not tested here)

I have added additional tests for negative scenarios and hardlinks in same directory.
The xattr changes to dfs_link()/remove were made from completeness perspective. Is it ok, I retain it for now? I will create the next PR for adding support for hardlinks to dfs_*xattr() and dfs_lookupx() routines and add tests validating correctness xattr migration and deletion.

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

daosbuild3 · 2026-05-27T12:04:31Z

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-18356/4/testReport/

knard38

First round of comments.

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

daosbuild3 · 2026-06-01T07:06:02Z

Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18356/5/testReport/

daosbuild3 · 2026-06-01T15:48:41Z

Test stage Functional on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18356/5/execution/node/1043/log

…de bit Addressed review comments regarding ctime not updated when hard link is first created for the file. Allow-unstable-test: true Signed-off-by: Sherin T George <sherin-t.george@hpe.com>

daosbuild3 · 2026-06-02T06:15:48Z

Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18356/6/testReport/

sherintg force-pushed the sherintg/dfs_hardlinks/DAOS-18989 branch from 6e65b37 to c9ce944 Compare May 26, 2026 05:25

sherintg force-pushed the sherintg/dfs_hardlinks/DAOS-18989 branch from c9ce944 to bfcaca8 Compare May 26, 2026 05:29

sherintg requested review from knard38 and mchaarawi May 26, 2026 05:34

sherintg marked this pull request as ready for review May 26, 2026 05:56

sherintg requested review from a team as code owners May 26, 2026 05:56

mchaarawi requested changes May 26, 2026

View reviewed changes

DAOS-18989 dfs: add hardlink link/unlink support and mask internal mo…

217e3a4

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

knard38 reviewed May 29, 2026

View reviewed changes

Comment thread src/client/dfs/common.c Outdated

Comment thread src/client/dfs/common.c Outdated

Comment thread src/client/dfs/common.c Outdated

Comment thread src/client/dfs/common.c

DAOS-18989 dfs: add hardlink link/unlink support and mask internal mo…

4846aa9

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

sherintg requested review from knard38 and mchaarawi June 1, 2026 06:11

knard38 requested changes Jun 1, 2026

View reviewed changes

Comment thread src/client/dfs/obj.c

DAOS-18989 dfs: add hardlink link/unlink support and mask internal mo…

e18ccdc

…de bit Addressed review comments regarding ctime not updated when hard link is first created for the file. Allow-unstable-test: true Signed-off-by: Sherin T George <sherin-t.george@hpe.com>

knard38 self-requested a review June 2, 2026 13:43

		D_ERROR("Missing GIT entry for file %s", obj->name);
		D_GOTO(out, EIO);

Conversation

sherintg commented May 26, 2026

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions Bot commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Jun 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daosbuild3 commented May 27, 2026

Uh oh!

knard38 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daosbuild3 commented Jun 1, 2026

Uh oh!

Uh oh!

daosbuild3 commented Jun 1, 2026

Uh oh!

daosbuild3 commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

github-actions Bot commented May 26, 2026 •

edited

Loading

sherintg Jun 3, 2026 •

edited

Loading