checkpoint-restore / criu

Checkpoint/Restore tool
criu.org
Other
2.79k stars 565 forks source link

memfd: make memfd_open() skip fd attributes not needed for vma mapping #2244

Closed osctobe closed 11 months ago

osctobe commented 11 months ago

There is only one user of memfd_open() outside of memfd.c: open_filemap(). It is restoring a file-backed mapping and doesn't need to F_SETOWN nor update the fd's position.

An optimization / cleanup.

avagin commented 11 months ago

The inhfd tests have been broken.

2023-08-22T11:03:39.7424214Z + ./test//zdtm.py --set inhfd run --all --keep-going --report report -f h
2023-08-22T11:03:40.0318668Z userns is supported
2023-08-22T11:03:40.0717599Z userns is supported
2023-08-22T11:03:40.6251187Z userns is supported
2023-08-22T16:40:23.2394454Z ##[error]The operation was canceled.
2023-08-22T16:40:23.2485541Z Post job cleanup.
osctobe commented 11 months ago

The inhfd tests have been broken. 2023-08-22T11:03:39.7424214Z + ./test//zdtm.py --set inhfd run --all --keep-going --report report -f h

Ah, it seems they are not run by the default zdtm.py invocation. I'll take a look tomorrow.

avagin commented 11 months ago

It looks like a test bug. Before, we didn't restore file positions for inherited descriptors. In case of memfd, it doesn't look like a right behavior. I think this pr fixes this problem but we need to adjust the test too. I think the following patch should work:

diff --git a/test/zdtm.py b/test/zdtm.py
index c6e852dc1..87234a55a 100755
--- a/test/zdtm.py
+++ b/test/zdtm.py
@@ -772,6 +772,15 @@ class inhfd_test:

     def getropts(self):
         self.__files = self.__fdtyp.create_fds()
+        i = 0
+        for my_file, peer_file in self.__files:
+            msg = self.__get_message(i)
+            my_file.write(msg)
+            my_file.flush()
+            data = peer_file.read(len(msg))
+            if data != msg:
+                raise test_fail_exc("FDs screwup: %r %r" % (msg, data))
+            i += 1
         ropts = ["--restore-sibling"]
         for i in range(len(self.__files)):
             my_file, peer_file = self.__files[i]
avagin commented 11 months ago

lgtm

avagin commented 11 months ago

pls fix DCO.

codecov-commenter commented 11 months ago

Codecov Report

Patch coverage: 86.66% and project coverage change: -0.01% :warning:

Comparison is base (5fedcaa) 70.62% compared to head (d442a79) 70.61%.

:exclamation: Current head d442a79 differs from pull request most recent head 0a14072. Consider uploading reports for the commit 0a14072 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## criu-dev #2244 +/- ## ============================================ - Coverage 70.62% 70.61% -0.01% ============================================ Files 133 133 Lines 33322 33315 -7 ============================================ - Hits 23532 23525 -7 Misses 9790 9790 ``` | [Files Changed](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2244?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore) | Coverage Δ | | |---|---|---| | [criu/memfd.c](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2244?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore#diff-Y3JpdS9tZW1mZC5j) | `82.58% <84.61%> (+0.81%)` | :arrow_up: | | [criu/files-reg.c](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2244?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore#diff-Y3JpdS9maWxlcy1yZWcuYw==) | `75.38% <100.00%> (+0.02%)` | :arrow_up: | ... and [9 files with indirect coverage changes](https://app.codecov.io/gh/checkpoint-restore/criu/pull/2244/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=checkpoint-restore)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

osctobe commented 11 months ago

pls fix DCO.

I'm lost as to what it expects from me:

Commit sha: [0a14072](https://github.com/checkpoint-restore/criu/pull/2244/commits/0a1407270b1ad8f7af8cea87d72b24cadc1092eb),
Author: Michał Mirosław, Committer: Michał Mirosław;
Expected "Michał Mirosław [emmir@google.com](mailto:emmir@google.com)",
but got "Michał Mirosław [emmir@google.com](mailto:emmir@google.com)".
osctobe commented 11 months ago

git commit --amend --reset-author didn't help.

osctobe commented 11 months ago

Ok, it was a non-breaking space in my .gitconfig...