DAOS-15069 dfs: use memcpy instead of snprintf for readdir #13654

mchaarawi · 2024-01-23T17:33:29Z

memcpy is more efficient to avoid scanning the entire kds string.
remove an assert and replace with error when looking up "."

Features: dfs

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:

- strncpy is more efficient to avoid scanning the entire kds string. - remove an assert and replace with error when looking up "." Features: dfs Required-githooks: true Signed-off-by: Mohamad Chaarawi <[email protected]>

github-actions · 2024-01-23T17:33:49Z

Bug-tracker data:
Ticket title is 'dfs_readdir complexity is quadratic in number of dirents'
Status is 'Open'
Labels: 'triaged'
https://daosio.atlassian.net/browse/DAOS-15069

yuvale2

Thank you!

ashleypittman · 2024-01-25T09:32:30Z

src/client/dfs/dfs.c

@@ -568,7 +568,7 @@ fetch_entry(dfs_layout_ver_t ver, daos_handle_t oh, daos_handle_t th, const char

 	/** TODO - not supported yet */
 	if (strcmp(name, ".") == 0)
-		D_ASSERT(0);
+		return ENOTSUP;


This is progress but wasn't there a different ticket with a reprodyucer for this? Without seeing that ticket could dfs_lookup_rel() skip over entiries named "." when doing the path walk, then the rerroducer should pass rather than exit gracefully as presumably it does with this fix.

we can leave that case for the ticket. this is fine for now

ashleypittman · 2024-01-25T09:41:37Z

src/client/dfs/dfs.c

+			strncpy(dirs[key_nr].d_name, ptr, kds[i].kd_key_len);
+			dirs[key_nr].d_name[kds[i].kd_key_len] = '\0';


I thought I'd been through daos and remove most of the places where snprintf() was used with "%s" but I must have missed this one. I think here though as dfs isn't ever doing anything with the entry names then it could just treat them as a memory region and use memcpy(). strncpy() will do two extra steps, check the length of the input string and pad the output buffer with \0, neither of which are of benefit in this case.

The same fix should also be applied to the dfs_listxattr code I assume?

agree, memcpy is better in this case.

Features: dfs Required-githooks: true Signed-off-by: Mohamad Chaarawi <[email protected]>

yuvale2 · 2024-01-26T17:48:47Z

src/client/dfs/dfs.c

@@ -3790,7 +3790,7 @@ readdir_int(dfs_t *dfs, dfs_obj_t *obj, daos_anchor_t *anchor, uint32_t *nr,
 			D_GOTO(out, rc = daos_der2errno(rc));

 		for (ptr = enum_buf, i = 0; i < number; i++) {
-			strncpy(dirs[key_nr].d_name, ptr, kds[i].kd_key_len);
+			memcpy(dirs[key_nr].d_name, ptr, kds[i].kd_key_len);


Maybe check that kds[i].kd_key_len < sizeof(dirs[key_nr].d_name)? (I realize that this wasn't checked before, maybe it's guaranteed elsewhere)

If that condition would be false, then we have landed some data corruption in the container and this would be the least of the problems :-)

But if we don't detect it here, we'll have two data corruptions. :)

my point was we probably won't even get here if there is data corruption in the container.
but sure, i can add that check if needed to repush. otherwise can add it in another PR im working on.

Adding in another PR sounds good to me.

Alternatively, if the check is 100% redundant, a comment that explains this would help (it's not obvious unless you know the server side, I guess).

I think there is an issue here but adding in another PR seems the way to go.

daosbuild1 · 2024-01-26T22:26:55Z

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13654/2/execution/node/1553/log

- memcpy is more efficient to avoid scanning the entire kds string. - remove an assert and replace with error when looking up "." Signed-off-by: Mohamad Chaarawi <[email protected]>

- memcpy is more efficient to avoid scanning the entire kds string. - remove an assert and replace with error when looking up "." #Pragmas from previous commit message: Skip-checkpatch: true Skip-python-bandit: true Skip-build: true Quick-build: true Quick-Functional: true Allow-unstable-test: true #RPM-test-version: version[-release] #RPM-test-version: 2.5.100 # VM1-label: ci_vm1 # Ubuntu-VM9-label: ci_vm9 # Leap15-VM9-label: ci_vm9 # EL8-VM9-label: ci_vm9 # HW-medium-label: ci_nvme5 # HW-large-label: ci_nvme9 Signed-off-by: Mohamad Chaarawi <[email protected]>

…13697) - memcpy is more efficient to avoid scanning the entire kds string. - remove an assert and replace with error when looking up "." Signed-off-by: Mohamad Chaarawi <[email protected]>

DAOS-15069 dfs: use strncpy instead of snprintf for readdir

404118a

- strncpy is more efficient to avoid scanning the entire kds string. - remove an assert and replace with error when looking up "." Features: dfs Required-githooks: true Signed-off-by: Mohamad Chaarawi <[email protected]>

mchaarawi marked this pull request as ready for review January 23, 2024 21:22

mchaarawi requested a review from a team as a code owner January 23, 2024 21:22

mchaarawi requested review from jolivier23 and yuvale2 January 23, 2024 21:22

yuvale2 previously approved these changes Jan 23, 2024

View reviewed changes

jolivier23 previously approved these changes Jan 24, 2024

View reviewed changes

ashleypittman reviewed Jan 25, 2024

View reviewed changes

use memcpy instead of strncpy

f4c85e4

Features: dfs Required-githooks: true Signed-off-by: Mohamad Chaarawi <[email protected]>

mchaarawi dismissed stale reviews from jolivier23 and yuvale2 via f4c85e4 January 25, 2024 15:26

mchaarawi changed the title ~~DAOS-15069 dfs: use strncpy instead of snprintf for readdir~~ DAOS-15069 dfs: use memcpy instead of snprintf for readdir Jan 25, 2024

mchaarawi requested review from jolivier23, yuvale2 and ashleypittman January 26, 2024 14:18

yuvale2 reviewed Jan 26, 2024

View reviewed changes

yuvale2 approved these changes Jan 26, 2024

View reviewed changes

jolivier23 approved these changes Jan 26, 2024

View reviewed changes

mchaarawi merged commit 3732d42 into master Jan 30, 2024
50 checks passed

mchaarawi deleted the mschaara/dfs_readdir_cpy branch January 30, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DAOS-15069 dfs: use memcpy instead of snprintf for readdir #13654

DAOS-15069 dfs: use memcpy instead of snprintf for readdir #13654

mchaarawi commented Jan 23, 2024 •

edited

Loading

github-actions bot commented Jan 23, 2024

yuvale2 left a comment

ashleypittman Jan 25, 2024

mchaarawi Jan 25, 2024

ashleypittman Jan 25, 2024

mchaarawi Jan 25, 2024

yuvale2 Jan 26, 2024

mchaarawi Jan 26, 2024

yuvale2 Jan 26, 2024

mchaarawi Jan 26, 2024

yuvale2 Jan 26, 2024

yuvale2 Jan 26, 2024 •

edited

Loading

ashleypittman Jan 29, 2024

daosbuild1 commented Jan 26, 2024

		strncpy(dirs[key_nr].d_name, ptr, kds[i].kd_key_len);
		dirs[key_nr].d_name[kds[i].kd_key_len] = '\0';

DAOS-15069 dfs: use memcpy instead of snprintf for readdir #13654

DAOS-15069 dfs: use memcpy instead of snprintf for readdir #13654

Conversation

mchaarawi commented Jan 23, 2024 • edited Loading

Before requesting gatekeeper:

Gatekeeper:

github-actions bot commented Jan 23, 2024

yuvale2 left a 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

yuvale2 Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 commented Jan 26, 2024

mchaarawi commented Jan 23, 2024 •

edited

Loading

yuvale2 Jan 26, 2024 •

edited

Loading