Fix tests (mount_sshfs.bats) in debian testing #4242
Conversation
tests/integration/mounts_sshfs.bats
Outdated
| @@ -393,7 +393,8 @@ function fail_sshfs_bind_flags() { | |||
| pass_sshfs_bind_flags "nodiratime" "bind" | |||
| run -0 grep -wq nodiratime <<<"$mnt_flags" | |||
| # MS_DIRATIME implies MS_RELATIME by default. | |||
| run -0 grep -wq relatime <<<"$mnt_flags" | |||
| # XXX: rata. For some reason this can't work on my debian system. | |||
| # run -0 grep -wq relatime <<<"$mnt_flags" | |||
There was a problem hiding this comment.
I'd like people to weight in here, is it safe to remove this grep?
I guess some simple reason might be the cause (like when it is used for the $SOMETHING, then relatime is not shown, or is maybe implied but not shown) but I don't really know.
There was a problem hiding this comment.
No, this needs to be kept. diratime is not part of the strict-rel-no-atime enum and it's behaviour is quite specific. This test ensures that a user requesting nodiratime doesn't clear relatime.
Does this part not work on Debian? relatime should be shown in the output of mount... An alternative patch would be to verify that everything other than relatime is not set.
There was a problem hiding this comment.
No, it doesn't work here nor with a stable kernel nor testing kernel, using different versions of mount (2.40 is the latest I tried).
I've changed this to check that no other setting that clears relatime is set. Please have a look. Especially see if I should check for any other flag, as I'm not very familiar with that kernel detail.
rata
force-pushed
the
rata/fix-tests-debian-testing
branch
from
2cedff7
to
2b48734
Compare
rata
changed the title
rata
force-pushed
the
rata/fix-tests-debian-testing
branch
2 times, most recently
from
7436b40
to
6c535e0
Compare
rata
changed the title
rata
force-pushed
the
rata/fix-tests-debian-testing
branch
2 times, most recently
from
cbe4b60
to
ae24dd2
Compare
|
EDIT: Grrr, I wanted to post in the issue, not the PR. Moved the comment there |
rata
force-pushed
the
rata/fix-tests-debian-testing
branch
2 times, most recently
from
2a4fdcf
to
8d5f950
Compare
If we don't unmount, we try to run the following command:
mount --bind -o remount,diratime,strictatime "$DIR"
That fails in debian testing, when it is the second time we call this
function in the same bats test (i.e. when $DIR is defined already).
strace shows this line tries to run this:
mount_setattr(3, "", AT_EMPTY_PATH, {attr_set=MOUNT_ATTR_NOSUID|MOUNT_ATTR_NODEV|MOUNT_ATTR_NOEXEC|MOUNT_ATTR_NOATIME|MOUNT_ATTR_STRICTATIME, attr_clr=MOUNT_ATTR_RDONLY|MOUNT_ATTR_NOATIME|MOUNT_ATTR_STRICTATIME|MOUNT_ATTR_NODIRATIME|0x40, propagation=0 /* MS_??? */, userns_fd=0}, 32) = -1 EINVAL (Invalid argument)
Note it has `MOUNT_ATTR_NOATIME` and `MOUNT_ATTR_STRICTATIME` which
probably causes it to return EINVAL.
We can fix this by adding atime to the mount command, but it seems
better just to unmount and start from scratch. This way we don't share
state between different tests in the same bats test and do not depend on
any specific behavior. Also, perf-wise mounting is evey time is not an
issue.
This patch fixes most of the tests in debian testing.
Signed-off-by: Rodrigo Campos <[email protected]>
rata
changed the title
relatime is not shown on some debian systems. Let's check that no other setting that removes the relatime effect is set, as that should be enough too. For more info, see the issue linked in the comments. Signed-off-by: Rodrigo Campos <[email protected]>
rata
force-pushed
the
rata/fix-tests-debian-testing
branch
from
8d5f950
to
5e89196
Compare
|
Closing, this was fixed by the alternative I proposed. |
If I apply this PR, it all works fine in Debian testing.
While it seems adding
atimecan fix the issue on debian, I still think it's better to not share so much state between "different" tests. Starting from scratch seems better to not run into issues about what we share across mounts in "different" tests inside the same bats tests. Performance-wise it doesn't seem relevant and reducing shared state in tests is nice, IMHO :)I'd really like people to review the second commit in detail, where we check relatime is set or no other options that resets relatime is set. That is needed in my debian to work, but I'd like review to make sure I didn't miss to check any flag there.
Fixes #4093