DAOS-12751 control: Add a daos filesystem evict command. #12331

ashleypittman · 2023-06-09T09:55:48Z

Add a command to tell dfuse and the kernel to evict a
path in dfuse. This is implemented via a ioctl that
sets a flag on the file handle, when the file is then
closed the kernel is then told the dentry for the file
is no longer valid which should cause the kernel to
forget the entry and any contents if the entry is
a directory.

Required-githooks: true

Signed-off-by: Ashley Pittman ashley.m.pittman@intel.com

github-actions · 2023-06-09T09:56:11Z

Bug-tracker data:
Ticket title is 'dfuse needs a mechanism to evict cached pool and container handles'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-12751

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-06-09T10:33:16Z

Test stage NLT on EL 8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12331/1/testReport/(root)/

daosbuild1

Style warning(s) for job https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-12331/2/
Please review https://wiki.hpdd.intel.com/display/DC/Coding+Rules

src/client/dfuse/ops/open.c

daosbuild1 · 2023-06-09T10:41:41Z

Test stage checkpatch completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12331/2/execution/node/146/log

Add a command to tell dfuse and the kernel to evict a path in dfuse. This is implemented via a ioctl that sets a flag on the file handle, when the file is then closed the kernel is then told the dentry for the file is no longer valid which should cause the kernel to forget the entry and any contents if the entry is a directory. Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

Updated patch

daosbuild1

LGTM. No errors found by checkpatch.

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

ashleypittman · 2023-06-09T15:00:48Z

src/client/dfuse/ops/open.c

+	if (oh->doh_evict_on_close) {
+		rc = fuse_lowlevel_notify_inval_entry(fs_handle->dpi_info->di_session,
+						      oh->doh_ie->ie_parent, oh->doh_ie->ie_name,
+						      strnlen(oh->doh_ie->ie_name, NAME_MAX));
+
+		if (rc != 0)
+			DFUSE_TRA_ERROR(oh->doh_ie, "inval_entry() returned: %d (%s)", rc,
+					strerror(-rc));


I think this is actually unsafe, once DFUSE_REPLY_ENTRY is called there's no longer a reference on the inode so I will take copies of this before landing.

daltonbohning

Maybe I misunderstand the ioctl stuff, but how would this work in the case of evicting a pool or container handle? For example, to pick up ACL changes

daltonbohning · 2023-06-09T15:06:47Z

src/control/cmd/daos/filesystem.go

@@ -37,6 +37,7 @@ type fsCmd struct {
 	ResetAttr      fsResetAttrCmd      `command:"reset-attr" description:"reset fs attributes"`
 	ResetChunkSize fsResetChunkSizeCmd `command:"reset-chunk-size" description:"reset fs chunk size"`
 	ResetObjClass  fsResetOclassCmd    `command:"reset-oclass" description:"reset fs obj class"`
+	DfuseEvict     fsDfuseEvictCmd     `command:"evict" description:"Evict object from dfuse"`


If this is ONLY for dfuse - not DFS - maybe evict -> dfuse-evict?
Or maybe it's okay if it only makes sense for dfuse anyway

I think the only thing al these commands have in common is they're for POSIX containers and therefore DFS at some level. evict doesn't mean anything in dfs directly that I'm aware of.

one thing i was thinking about here is that the daos fs options are on the container (server side), but these dfuse commands apply only on the client side (just the node they are running on). so they are just local operations and only executed on the node where this runs.
i don't know if that counts as grounds that we need to split this off into 2 tools or just have an extension to the dfuse command, or just keep it as it is. what do you think?

We discussed this extensively in the working group calls, this was your suggestion.

I'd be fine with making a new subcommand for the daos command other than daos filesystem but I don't want to do it in this PR.

yes i know, and i was just rethinking this. im fine to keeping it that way for now.

we should just add a comment that this is a local client operation

I agree though, there's a clear distinction between commands that have an effect on the container itself vs commands that change local in-memory state.

Something else to keep in mind is that we currently don't guarantee backwards compatibility for the command line, but I wonder if that will/should be a requirement in the future, since changing the command line interface later can be disruptive to deployment, job setup/cleanup, etc.

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-07-03T07:38:21Z

Test stage NLT on EL 8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12331/5/testReport/

daosbuild1 · 2023-07-03T07:51:47Z

Test stage NLT on EL 8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12331/6/testReport/

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-07-04T12:31:36Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12331/12/execution/node/1276/log

docs/user/filesystem.md

mjmac · 2023-07-05T16:49:38Z

src/control/cmd/daos/filesystem.go

+	if cmd.Ino != 0 {
+		if ap.dfuse_mem.found {
+			cmd.Infof(" Inode %#lx known", cmd.Ino)
+		} else {
+			cmd.Infof(" Inode %#lx not known", cmd.Ino)
+		}
+	}


Hmm. It seems like it would be better UX for the command to show an error for an unknown/invalid inode rather than making the user parse the output to figure it out.

This is not an error either way, it's in effect polling for the operation to have completed. Maybe the text in this output is badly worded but neither of these is an error. See the changes to filesystem.md in this PR for a full explanation.

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daosbuild1 · 2023-07-06T21:05:06Z

Test stage Functional Hardware Medium completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12331/13/execution/node/1286/log

Test-tag: dfuse

daosbuild1

LGTM. No errors found by checkpatch.

docs/user/filesystem.md

daltonbohning · 2023-07-11T14:44:56Z

docs/user/filesystem.md

+Paths can be requested for eviction from dfuse using the `daos filesystem evict` command, this does
+not change any data that is stored in DAOS in any way but rather releases local resources.  This
+command will return the inode number of the path as well as key dfuse metrics.


Suggested change

Paths can be requested for eviction from dfuse using the `daos filesystem evict` command, this does

not change any data that is stored in DAOS in any way but rather releases local resources. This

command will return the inode number of the path as well as key dfuse metrics.

Paths can be requested for eviction from dfuse using the `daos filesystem evict` command. This does

not change any data that is stored in DAOS in any way but rather releases local resources. This

command will return the inode number of the path as well as key dfuse metrics.

docs/user/filesystem.md

src/utils/daos_hdlr.c

utils/node_local_test.py

Test-tag: dfuse Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1

LGTM. No errors found by checkpatch.

daltonbohning · 2023-07-12T15:30:51Z

src/utils/daos_hdlr.c

+			rc = errno;
+			goto out;


In general, shouldn't these use D_GOTO?

wiliamhuang · 2023-07-12T17:21:08Z

Minor issues,
https://github.com/daos-stack/daos/blob/amd/dfuse-evict/src/client/dfuse/ops/ioctl.c#L329
"struct dfuse_mem_query query = {};" can be removed.

https://github.com/daos-stack/daos/blob/amd/dfuse-evict/src/client/dfuse/ops/ioctl.c#L333
"if (in_bufsz != sizeof(query))" --> "if (in_bufsz != sizeof(struct dfuse_mem_query))"

ashleypittman · 2023-07-12T18:08:14Z

Minor issues, https://github.com/daos-stack/daos/blob/amd/dfuse-evict/src/client/dfuse/ops/ioctl.c#L329 "struct dfuse_mem_query query = {};" can be removed.

https://github.com/daos-stack/daos/blob/amd/dfuse-evict/src/client/dfuse/ops/ioctl.c#L333 "if (in_bufsz != sizeof(query))" --> "if (in_bufsz != sizeof(struct dfuse_mem_query))"

Yes, you're right. I've got more PRs to follow in this series, I can fix this up later.

Multiple cherry-picks to add daos fs query feature for fuse statistics. Helpful for understanding and tuning DAOS performance when dfuse is used. DAOS-13625 dfuse: Merge the info and projection_info structs. (#11881) DAOS-13658 dfuse: Add filesystem query command. (#12367) DAOS-12751 control: Add a daos filesystem evict command. (#12331) DAOS-12751 dfuse: Improve evict command. (#12633) DAOS-13625 dfuse: Remove dfuse_projection_info entirely. (#12796) DAOS-13625 dfuse: Replace fs_handle with dfuse_info. (#12894) DAOS-13625 dfuse: Add core inode_lookup() and inode_decref() functions. (#12573) DAOS-14411 dfuse: Add per-container statistics. (#12819) Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

Multiple cherry-picks to add daos fs query feature for fuse statistics. Helpful for understanding and tuning DAOS performance when dfuse is used. DAOS-13625 dfuse: Merge the info and projection_info structs. (#11881) DAOS-13658 dfuse: Add filesystem query command. (#12367) DAOS-12751 control: Add a daos filesystem evict command. (#12331) DAOS-12751 dfuse: Improve evict command. (#12633) DAOS-13625 dfuse: Remove dfuse_projection_info entirely. (#12796) DAOS-13625 dfuse: Replace fs_handle with dfuse_info. (#12894) DAOS-13625 dfuse: Add core inode_lookup() and inode_decref() functions. (#12573) DAOS-14411 dfuse: Add per-container statistics. (#12819) Features: dfuse Required-githooks: true Change-Id: I8ae3cc743697c2434ae0d54b382ee6c585a3b033 Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com> Signed-off-by: Jeff Olivier <jeffolivier@google.com>

Multiple cherry-picks to add daos fs query feature for fuse statistics. Helpful for understanding and tuning DAOS performance when dfuse is used. DAOS-13625 dfuse: Merge the info and projection_info structs. (#11881) DAOS-13658 dfuse: Add filesystem query command. (#12367) DAOS-12751 control: Add a daos filesystem evict command. (#12331) DAOS-12751 dfuse: Improve evict command. (#12633) DAOS-13625 dfuse: Remove dfuse_projection_info entirely. (#12796) DAOS-13625 dfuse: Replace fs_handle with dfuse_info. (#12894) DAOS-13625 dfuse: Add core inode_lookup() and inode_decref() functions. (#12573) DAOS-14411 dfuse: Add per-container statistics. (#12819) DAOS-14411 control: Expose dfuse statistics as yaml. (#13876) Features: dfuse Required-githooks: true Change-Id: I8ae3cc743697c2434ae0d54b382ee6c585a3b033 Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com> Signed-off-by: Jeff Olivier <jeffolivier@google.com>

Multiple cherry-picks to add daos fs query feature for fuse statistics. Helpful for understanding and tuning DAOS performance when dfuse is used. DAOS-13625 dfuse: Merge the info and projection_info structs. (#11881) DAOS-13658 dfuse: Add filesystem query command. (#12367) DAOS-12751 control: Add a daos filesystem evict command. (#12331) DAOS-12751 dfuse: Improve evict command. (#12633) DAOS-13625 dfuse: Remove dfuse_projection_info entirely. (#12796) DAOS-13625 dfuse: Replace fs_handle with dfuse_info. (#12894) DAOS-13625 dfuse: Add core inode_lookup() and inode_decref() functions. (#12573) DAOS-14411 dfuse: Add per-container statistics. (#12819) DAOS-14411 control: Expose dfuse statistics as yaml. (#13876) Changed base branch to google/2.4 for daos_build test Change-Id: I8ae3cc743697c2434ae0d54b382ee6c585a3b033 Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com> Signed-off-by: Jeff Olivier <jeffolivier@google.com>

daosbuild1 reviewed Jun 9, 2023

View reviewed changes

ashleypittman changed the title ~~DAOS-12751 tool: Add a daos filesystem evict command.~~ DAOS-12751 control: Add a daos filesystem evict command. Jun 9, 2023

ashleypittman force-pushed the amd/dfuse-evict branch from 80cf5bd to 58af10b Compare June 9, 2023 10:39

daosbuild1 previously requested changes Jun 9, 2023

View reviewed changes

src/client/dfuse/ops/open.c Outdated Show resolved Hide resolved

ashleypittman force-pushed the amd/dfuse-evict branch from 58af10b to 5fdc6a7 Compare June 9, 2023 10:42

daosbuild1 reviewed Jun 9, 2023

View reviewed changes

Revise test.

f051fe0

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1 reviewed Jun 9, 2023

View reviewed changes

ashleypittman requested a review from daltonbohning June 9, 2023 14:59

ashleypittman commented Jun 9, 2023

View reviewed changes

daltonbohning reviewed Jun 9, 2023

View reviewed changes

ashleypittman added 2 commits July 3, 2023 07:04

Merge branch 'master' into amd/dfuse-evict

5273d95

Fix test.

400c9f0

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1 reviewed Jul 3, 2023

View reviewed changes

Remove dead code.

5a5dfe1

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1 reviewed Jul 3, 2023

View reviewed changes

Add a test and make it work.

ad83f4b

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1 reviewed Jul 3, 2023

View reviewed changes

Add documentation.

e4a64b5

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1 reviewed Jul 3, 2023

View reviewed changes

Remove debugging.

dc37309

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1 reviewed Jul 3, 2023

View reviewed changes

Use macro.

4bb9c76

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

Merge branch 'master' into amd/dfuse-evict

6530951

daosbuild1 reviewed Jul 4, 2023

View reviewed changes

mjmac reviewed Jul 5, 2023

View reviewed changes

Fix typo and improve output.

75db207

Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

daosbuild1 reviewed Jul 6, 2023

View reviewed changes

Merge branch 'master' into amd/dfuse-evict

d385d0f

Test-tag: dfuse

daosbuild1 reviewed Jul 10, 2023

View reviewed changes

ashleypittman requested review from daltonbohning, mchaarawi and wiliamhuang July 11, 2023 12:40

daltonbohning reviewed Jul 11, 2023

View reviewed changes

ashleypittman added 2 commits July 11, 2023 15:08

Take on review feedback.

03a6c97

Test-tag: dfuse Required-githooks: true Signed-off-by: Ashley Pittman <ashley.m.pittman@intel.com>

Merge branch 'master' into amd/dfuse-evict

e8aa287

daosbuild1 reviewed Jul 11, 2023

View reviewed changes

ashleypittman requested a review from daltonbohning July 12, 2023 09:11

daltonbohning approved these changes Jul 12, 2023

View reviewed changes

src/utils/daos_hdlr.c

Comment on lines +2507 to +2508

rc = errno;

goto out;

Copy link

Contributor

daltonbohning Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, shouldn't these use D_GOTO?

wiliamhuang approved these changes Jul 12, 2023

View reviewed changes

ashleypittman requested a review from a team July 12, 2023 18:26

ashleypittman merged commit 20eee6d into master Jul 13, 2023

ashleypittman deleted the amd/dfuse-evict branch July 13, 2023 07:40

jolivier23 mentioned this pull request Apr 26, 2024

DAOS-14411 dfuse: Add daos fs query #14261

Merged

18 tasks

DAOS-12751 control: Add a daos filesystem evict command. #12331

DAOS-12751 control: Add a daos filesystem evict command. #12331

Conversation

ashleypittman commented Jun 9, 2023

github-actions bot commented Jun 9, 2023 • edited Loading

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jun 9, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jun 9, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daltonbohning 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

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jul 3, 2023

daosbuild1 commented Jul 3, 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 Jul 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

daosbuild1 commented Jul 6, 2023

daosbuild1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daosbuild1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wiliamhuang commented Jul 12, 2023

ashleypittman commented Jul 12, 2023

github-actions bot commented Jun 9, 2023 •

edited

Loading