DAOS-14149 client: add compatible mode for libpil4dfs #13294

wiliamhuang · 2023-11-05T04:51:32Z

Regular mode of the interception library libpil4dfs uses fake file descriptors (fd) allocated in user space. In case of some libc functions are not intercepted, applications could get error even crash due to fake fd. Compatibility mode is introduced to alleviate such issues and increase compatibility. open, openat, and opendir etc rely on dfuse to get real fd from dfuse and return them to applications with better compatibility and degraded performance.
Environmental variable "D_IL_COMPATIBLE=1" turns on compatible mode. Regular mode is set as default if "D_IL_COMPATIBLE" is unset.

Required-githooks: true

Before requesting gatekeeper:

Two review approvals and any prior change requests have been resolved.
Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
Commit messages follows the guidelines outlined here.
Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

Test-tag: pil4dfs avoid using fake fd to get better compatibility with degraded performance in open and opendir etc. Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

github-actions · 2023-11-05T04:51:52Z

Bug-tracker data:
Ticket title is 'To implement a compatible mode in libpil4dfs for better compatibility'
Status is 'In Progress'
Labels: 'intercept_lib'
https://daosio.atlassian.net/browse/DAOS-14149

daosbuild1

LGTM. No errors found by checkpatch.

Test-tag: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

Features: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1

LGTM. No errors found by checkpatch.

Features: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Features: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1 · 2023-11-14T05:47:44Z

Test stage Unit Test bdev on EL 8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-13294/5/display/redirect

daosbuild1 · 2023-11-14T05:47:48Z

Test stage NLT on EL 8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-13294/5/display/redirect

daosbuild1

LGTM. No errors found by checkpatch.

Features: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

wiliamhuang · 2023-11-17T14:52:37Z

We may need a separate ticket and PR for more tests.

mchaarawi · 2023-11-17T14:22:14Z

src/client/dfuse/pil4dfs/int_dfs.c

+	if (rc != 0) {
+		rc = errno;
+		if (rc != ENOTTY)
+			DS_WARN(rc, "ioctl call on %d failed", fd);


most of those sounds like legitimate errors and not warnings?

Even fetch_dfs_cont_file_obj_with_fd() failed, open() still can return the fd allocated by kernel. If you think error is more appropriate here, I can change it.

This was more of a question to you..
Warning messages should be used when something that the user is expecting but another path was taken and the operation is still onging / succeeds.
Error messages are for when we get legitimate errors from DAOS that something went wrong like EIO, etc.
otherwise, like if user is opening a non-existent file, this is more of a debug message that can be logged at the DAOS log level.

ok. I will change this and several other warnings in this function to debug message. Thank you!

should probably print the errno (or string version of it here), no? (applies to subsequent messages).

DS_WARN does print errno and string version message.
fetch_dfs_cont_file_obj_with_fd() is called inside open. When it is called, we already get fd from dfuse without issue. Even fetch_dfs_cont_file_obj_with_fd fails, we still return fd allocated by dfuse. I think warning is appropriate here.

mchaarawi · 2023-11-17T14:25:05Z

src/client/dfuse/pil4dfs/int_dfs.c

@@ -850,6 +979,13 @@ retrieve_handles_from_fuse(int idx)
 			errno_saved, strerror(errno_saved));
 		goto err;
 	}
+	rc = dc_cont_hdl2uuid(dfs_list[idx].coh, NULL, &dfs_list[idx].uuid_cont);


ideally we don't want to use internal functions like these here. is this for the UNS support?

ok. I can remove this for now. It is not for UNS support. I just used the container uuid to double check the container uuid returned by ioctl(fd, DFUSE_IOCTL_IL, &il_reply).

mchaarawi · 2023-11-17T14:29:21Z

src/tests/ftest/dfuse/bash.py

@@ -212,3 +212,18 @@ def test_bashcmd_pil4dfs(self):
        :avocado: tags=Cmd,test_bashcmd_pil4dfs
        """
        self.run_bashcmd(il_lib="libpil4dfs.so")
+
+    def test_bashcmd_pil4dfs_compatible(self):


we need to add some more testing for the most common use cases that we know are not supported currently that will be with compatibility mode.

mchaarawi · 2023-11-17T17:06:17Z

src/client/dfuse/pil4dfs/int_dfs.c

+
+		rc = dfs_get_mode(dfs_obj_local, &mode_query);
+		if (rc)
+			goto out_compatible;


do we need to close dfs_obj_local?
error ignored?

You are right. "dfs_obj_local" should be closed. I will add a debug message when dfs_get_mode() fails. Thank you!

src/client/dfuse/pil4dfs/int_dfs.c

daosbuild1 · 2023-11-17T17:23:12Z

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13294/7/execution/node/1511/log

jolivier23

I was chatting with Johann last week and he had an interesting idea.

If we have a special file, let's say .fd_alloc, where dfuse does nothing but just allow the kernel to create a new file descriptor, then we can use ioctl to map that file descriptor to our real dfs file. Then we have a real file descriptor to return to the application. If we ever encounter a situation that is difficult to implement at least in the near term (e.g. mmap), we can simply disable interception and defer back to dfuse which now knows the mapping between the fd and the dfs file.

so open handling would be something like
Open the dfs file
If not successful
return

fd = real_open("/path/to/dfuse/.fd_alloc", ...);
ioctl(fd, DFUSE_MAP_FILE, ....);
save mapping from fd to dfs_obj
return fd;

jolivier23 · 2023-11-28T15:49:54Z

src/client/dfuse/pil4dfs/int_dfs.c

+	if (rc != 0) {
+		rc = errno;
+		if (rc != ENOTTY)
+			DS_WARN(rc, "ioctl call on %d failed", fd);


should probably print the errno (or string version of it here), no? (applies to subsequent messages).

jolivier23 · 2023-12-13T16:37:10Z

docs/user/filesystem.md

@@ -974,6 +974,10 @@ libpil4dfs intercepting summary for ops on DFS:
 [op_sum ]  5003
 ```

+### Turn on compatible mode in libpil4dfs
+
+"D_IL_COMPATIBLE=1" or "D_IL_COMPATIBLE=true" turns on compatible mode. Fake fd will not be returned to applications. This mode provides better compatibility with degraded performance in open, openat, and opendir, etc.


I would argue this should be the default setting and the performant version should be a use at your own risk option. Though I think we can probably make this better if we do something like opening the special fake file on the actual fuse mountpoint to avoid lookup calls and maybe acceptable enough that we don't even need this environment variable at all.

Anyway, my reasoning here is that people will not care about a small bump in performance if it means half of their applications don't run at all. I don't think open is as big of a bottleneck as many other metadata ops

I agree. I can set compatible mode as default in next commit. Thank you!

I'm not sure i agree actually. I do not think the performance effect is small but can be quite substantial in metadata heavy cases.
ideally these use cases should be found and eventually resolved. if we set this as the default, im afraid we are going to get not reports of new issues found, and more importantly have performance issues.

I think if we move to fake file method, perf difference will be minimal, IMO. The issue with the current version, is it will cause a bunch of lookups for items in path whereas a fake file will minimize that churn.

The main issue I have with not being compatible by default, is that it may take significant cycles for someone to discover this and their first experience will be that DAOS doesn't work.

I mentioned this to Ashley and he had an even better idea, IMO. If you allocate a bunch of these at startup, you can just keep a cache of such kernel allocated file descriptors and assign them to real files as needed.

Signed-off-by: Lei Huang <lei.huang@intel.com>

Test-provider-hw-medium: ofi+tcp;ofi_rxm Priority: 2 Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

wiliamhuang · 2024-04-25T14:15:04Z

A test case for a shell script that calls if [ -d <dir> ] and then calls mkdir as a subprocess and checks for the dir again would be good. For the case we're discussing I guess you'd have to create the dir, test for it using -d, then delete it and test for it again with -d, with caching on the second -d test should get the wrong result but with disabling the caching for bash processes then it should correctly detect the dir is missing.

Thank you! Since dir caching now is automatically disabled inside bash, I plan to add such test in unit test. src/tests/ftest/dfuse/utest/pil4dfs_dcache.c

Could you write one in bash so it's closer to the case you're testing for.

ok. I will test it in bash then. I also need to expose an env to enable/disable auto-disabling dir caching in bash.

daosbuild1 · 2024-04-25T16:03:10Z

Test stage Functional Hardware Medium Verbs Provider completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13294/43/testReport/

wiliamhuang · 2024-04-25T16:07:02Z

All CI tests passed except one failure. It was reported in ticket, https://daosio.atlassian.net/issues/DAOS-15662.
I am preparing the ftest Ashley suggested. I will update soon.

2. add a test to create dir and file, then remove them and recreate them. Priority: 2 Test-tag: test_bashdcache_pil4dfs Skip-unit-tests: true Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

mchaarawi · 2024-04-25T17:12:36Z

SConstruct

@@ -363,7 +363,8 @@ MINIMAL_ENV = ('HOME', 'TERM', 'SSH_AUTH_SOCK', 'http_proxy', 'https_proxy', 'PK

 # Environment variables that are also kept when LD_PRELOAD is set.
 PRELOAD_ENV = ('LD_PRELOAD', 'D_LOG_FILE', 'DAOS_AGENT_DRPC_DIR', 'D_LOG_MASK', 'DD_MASK',
-               'DD_SUBSYS', 'D_IL_MAX_EQ', 'D_IL_ENFORCE_EXEC_ENV', 'D_IL_COMPATIBLE')
+               'DD_SUBSYS', 'D_IL_MAX_EQ', 'D_IL_ENFORCE_EXEC_ENV', 'D_IL_COMPATIBLE',
+               'D_IL_NO_DCACHE_BASH')


im going to object on adding more env variables like this.. it's counter productive for users and hard to keep track of. if we know this has issues, then why we need a flag to enable it?

how about if compatible mode is on, dcache in bash is disabled.
otherwise it's enabled.

Another option would be to have a D_IL_NO_CACHE_EXE and have it be a comma separated list of process named with the default being sh,bash, that way the default would work and not need a hack in this file and there would be the option to enable/disable it for other procs as needed.

This shouldn't hold up the PR from landing however, the previous code is good enough to pass the test and land the PR IMO and we should improve this in a follow up.

ok. I can add an env "D_IL_NO_CACHE_EXE" to support user supplied app list in a follow up PR.
I think we should disable dcahe in bash in regular mode too. bash is not sensitive to performance. Disabling cache can avoid possible consistency issue.

wiliamhuang · 2024-04-25T17:53:14Z

@ashleypittman Shall I remove "'git -C {} checkout lei/DAOS-14149'.format(build_dir)," in src/tests/ftest/dfuse/daos_build.py now?

2. fix tags in test_bashdcache_pil4dfs Priority: 2 Test-tag: test_bashdcache_pil4dfs Skip-unit-tests: true Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

Features: pil4dfs Priority: 2 Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

wiliamhuang · 2024-04-25T18:17:15Z

The new ftest passed in previous build.
https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-13294/44/testReport/FTEST_dfuse/DFuseBashdcacheTest/

ashleypittman · 2024-04-25T18:24:52Z

src/tests/ftest/dfuse/bash_dcache.py

+
+
+class DFuseBashdcacheTest(DfuseTestBase):
+    """Base Bashdcache test class.


You need to resolve or silence these warnings.

Forgot to fix this in last commit. I will add "Bashdcache" into utils/cq/words.dict after current build completes. Thank you!

Added "#pylint: disable-next=wrong-spelling-in-docstring" to suppress the warning. Thank you!

src/tests/ftest/dfuse/bash_dcache.py

ashleypittman · 2024-04-25T18:33:41Z

src/tests/ftest/dfuse/bash_dcache.py

+        result = run_remote(self.log, self.hostlist_clients, env_str + cmd)
+        if not result.passed:
+            self.fail(f'"{cmd}" failed on {result.failed_hosts}')


Can you check the output?

Updated as you suggested. Thank you!

This checks the output but both files have the same contents so it's not checking as much as it could.

ok. I can write difference content for the second write. Will update it in another PR. Thank you!

src/tests/ftest/dfuse/bash_dcache.py

Features: pil4dfs Priority: 2 Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1 · 2024-04-26T01:01:05Z

Test stage Functional on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-13294/46/display/redirect

wiliamhuang · 2024-04-26T13:30:31Z

A few minor lint related changes will be needed after current build is finished.

wrong-spelling-in-docstring, Wrong spelling of a word 'Bashdcache' in a docstring:
wrong-spelling-in-docstring, Wrong spelling of a word 'conftest' in a docstring:
superfluous-parens, Unnecessary parens after 'if' keyword
wrong-spelling-in-comment, Wrong spelling of a word 'dcache' in a comment:

@ashleypittman Is there anything else we should change? Thank you!

wiliamhuang · 2024-04-26T19:18:23Z

@ashleypittman All tests finished in CI without issue in build 49 https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-13294/49/. Any more comments? I will push another commit to fix pylint related issue soon. Thank you!

Priority: 2 Test-tag: test_bash_dcache_pil4dfs Skip-unit-tests: true Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

wiliamhuang · 2024-04-29T11:43:57Z

@ashleypittman @mchaarawi Do you have more feedback? Thank you!

ashleypittman · 2024-04-29T14:48:01Z

src/tests/ftest/dfuse/bash_dcache.py

+        result = run_remote(self.log, self.hostlist_clients, env_str + cmd)
+        if not result.passed:
+            self.fail(f'"{cmd}" failed on {result.failed_hosts}')


This checks the output but both files have the same contents so it's not checking as much as it could.

src/tests/ftest/dfuse/bash_dcache.py

ashleypittman · 2024-04-29T14:51:26Z

src/tests/ftest/dfuse/daos_build.py

+        :avocado: tags=daosio,dfuse,il,dfs,pil4dfs
+        :avocado: tags=DaosBuild,test_dfuse_daos_build_wt_pil4dfs
+        """
+        self.run_build_test("nocache", il_lib='libpil4dfs.so', run_on_vms=True)


I assume this doesn't work with anything other than nocache as dfuse would cache negative dentries and then lots of things would fail.

I agree. That's what I found in local test.

Regular mode of the interception library (libpil4dfs) uses fake file descriptors (fd) allocated in user space. In case of some libc functions are not intercepted, applications could get errors or even crash due to the fake fd. Compatibility mode is introduced to alleviate such issues and increase compatibility. open, openat, and opendir etc rely on dfuse to get real fd from dfuse and return them to applications with better compatibility but with some degraded performance. Environmental variable "D_IL_COMPATIBLE=1" turns on compatible mode. Regular mode is set as default if "D_IL_COMPATIBLE" is unset. Signed-off-by: Lei Huang <lei.huang@intel.com>

DAOS-14149 client: add compatible mode for libpil4dfs

4d066d5

Test-tag: pil4dfs avoid using fake fd to get better compatibility with degraded performance in open and opendir etc. Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1 reviewed Nov 5, 2023

View reviewed changes

wiliamhuang added 3 commits November 6, 2023 15:56

fix format issue. also increase kernel_fa-fake_fd hash table size.

d77b2a3

Test-tag: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

resolving format related issues

cc9cd63

Test-tag: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

Merge branch 'master' into lei/DAOS-14149 and resolve conflict

b652dd8

Features: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1 reviewed Nov 14, 2023

View reviewed changes

add document about compatible mode

de19218

Features: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1 reviewed Nov 14, 2023

View reviewed changes

fix clang-format issues

a1682cb

Features: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1 reviewed Nov 14, 2023

View reviewed changes

Merge branch 'master' into lei/DAOS-14149 and resolve conflict

e1cf40e

Features: pil4dfs Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

daosbuild1 reviewed Nov 17, 2023

View reviewed changes

wiliamhuang marked this pull request as ready for review November 17, 2023 14:01

wiliamhuang requested review from a team as code owners November 17, 2023 14:01

wiliamhuang requested review from mchaarawi and jolivier23 November 17, 2023 14:01

mchaarawi reviewed Nov 17, 2023

View reviewed changes

jolivier23 reviewed Dec 12, 2023

View reviewed changes

jolivier23 reviewed Dec 13, 2023

View reviewed changes

wiliamhuang added 2 commits March 17, 2024 04:12

Merge branch 'master' into lei/DAOS-14149

be3a63c

Signed-off-by: Lei Huang <lei.huang@intel.com>

merge master and fix conflicts

62563ef

Test-provider-hw-medium: ofi+tcp;ofi_rxm Priority: 2 Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

wiliamhuang requested a review from a team as a code owner March 17, 2024 04:13

1. add env D_IL_NO_DCACHE_BASH to control whether disable dcache in bash

93d1b18

2. add a test to create dir and file, then remove them and recreate them. Priority: 2 Test-tag: test_bashdcache_pil4dfs Skip-unit-tests: true Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

mchaarawi reviewed Apr 25, 2024

View reviewed changes

wiliamhuang added 2 commits April 25, 2024 18:00

1. remove "D_IL_NO_DCACHE_BASH" in SConstruct

4577b10

2. fix tags in test_bashdcache_pil4dfs Priority: 2 Test-tag: test_bashdcache_pil4dfs Skip-unit-tests: true Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

use master branch in daos build test

80341e9

Features: pil4dfs Priority: 2 Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

mchaarawi previously approved these changes Apr 25, 2024

View reviewed changes

ashleypittman reviewed Apr 25, 2024

View reviewed changes

add comments and compare output

3e39ab7

Features: pil4dfs Priority: 2 Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

wiliamhuang dismissed mchaarawi’s stale review via 3e39ab7 April 26, 2024 00:59

wiliamhuang added 3 commits April 26, 2024 19:25

fix pylint related issues

71201a7

Priority: 2 Test-tag: test_bash_dcache_pil4dfs Skip-unit-tests: true Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

trying to fix pylint warnings for spelling

9dace27

Priority: 2 Test-tag: test_bash_dcache_pil4dfs Skip-unit-tests: true Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

trying to fix pylint warnings

b9c0a3e

Priority: 2 Test-tag: test_bash_dcache_pil4dfs Skip-unit-tests: true Required-githooks: true Signed-off-by: Lei Huang <lei.huang@intel.com>

wiliamhuang requested review from ashleypittman and mchaarawi April 26, 2024 19:45

mchaarawi approved these changes Apr 29, 2024

View reviewed changes

ashleypittman approved these changes Apr 29, 2024

View reviewed changes

wiliamhuang requested a review from a team April 29, 2024 15:47

mchaarawi merged commit 8511455 into master Apr 29, 2024
44 of 45 checks passed

mchaarawi deleted the lei/DAOS-14149 branch April 29, 2024 16:02

mjmac mentioned this pull request Apr 30, 2024

mjmac/DAOS 8331 no agent #14288

Closed



		class DFuseBashdcacheTest(DfuseTestBase):
		"""Base Bashdcache test class.

DAOS-14149 client: add compatible mode for libpil4dfs #13294

DAOS-14149 client: add compatible mode for libpil4dfs #13294

Conversation

wiliamhuang commented Nov 5, 2023 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Nov 5, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Nov 14, 2023

daosbuild1 commented Nov 14, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

wiliamhuang commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Nov 17, 2023

jolivier23 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolivier23 Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiliamhuang commented Apr 25, 2024

daosbuild1 commented Apr 25, 2024

wiliamhuang commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiliamhuang commented Apr 25, 2024

wiliamhuang commented Apr 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Apr 26, 2024

wiliamhuang commented Apr 26, 2024

wiliamhuang commented Apr 26, 2024 • edited Loading

wiliamhuang commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiliamhuang commented Nov 5, 2023 •

edited

Loading

jolivier23 left a comment •

edited

Loading

jolivier23 Dec 13, 2023 •

edited

Loading

wiliamhuang commented Apr 26, 2024 •

edited

Loading