Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix few Filesystem tests on FreeBSD #65059

Merged
merged 3 commits into from
Feb 24, 2022
Merged

fix few Filesystem tests on FreeBSD #65059

merged 3 commits into from
Feb 24, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Feb 9, 2022

The MoveDirectory_FailToMoveLowerCaseDirectoryWhenUpperCaseDirectoryExists is technically wrong on Unix IMHO.
The case sensitivity is determined by given filesystem not by the OS itself.

To ignore that, macOS has case insensitive filesystem by default but default filesystem (zfs) on FreeBSD is case sensitive.
This seems like old oversight assuming (somewhat correctly) similarity between FreeBSD and macOS

Some other tests they fail with

     System.IO.Tests.File_Open_str_options.ZeroPreallocationSizeDoesNotAllocate(mode: Truncate, createFile: True) [STARTING]
  stat: illegal option -- c
  usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file|handle ...]
      System.IO.Tests.File_Open_str_options.ZeroPreallocationSizeDoesNotAllocate(mode: Truncate, createFile: True) [FAIL]

In that regard, FreeBSD is like macOS e.g. parameters for stat are the same. I assumed easy fix but I was wrong.
The behavior is somewhat different.

macOS

Shining:wfurt-runtime furt$ touch foo
Shining:wfurt-runtime furt$ stat -f "%b %k" foo
0 4096

FreeBSD

furt@d13:~ $ touch foo
furt@d13:~ $ stat -f "%b %k" foo
1 131072

The zfs shows one block used even if the file itself is empty.
To fix that I decided to use %z that simply returns allocated size on both macOS and FreeBSD

I did quick check and solaris seems to follow Linux e.g. "%b %B" and updated code should still work.
cc: @am11

cc: @Thefrank @rootwyrm @emaste

@wfurt wfurt added area-System.IO os-freebsd FreeBSD OS test-enhancement Improvements of test source code labels Feb 9, 2022
@wfurt wfurt self-assigned this Feb 9, 2022
@ghost
Copy link

ghost commented Feb 9, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

The MoveDirectory_FailToMoveLowerCaseDirectoryWhenUpperCaseDirectoryExists is technically wrong on Unix IMHO.
The case sensitivity is determined by given filesystem not by the OS itself.

To ignore that, macOS has case insensitive filesystem by default but default filesystem (zfs) on FreeBSD is case sensitive.
This seems like old oversight assuming (somewhat correctly) similarity between FreeBSD and macOS

Some other tests they fail with

     System.IO.Tests.File_Open_str_options.ZeroPreallocationSizeDoesNotAllocate(mode: Truncate, createFile: True) [STARTING]
  stat: illegal option -- c
  usage: stat [-FLnq] [-f format | -l | -r | -s | -x] [-t timefmt] [file|handle ...]
      System.IO.Tests.File_Open_str_options.ZeroPreallocationSizeDoesNotAllocate(mode: Truncate, createFile: True) [FAIL]

In that regard, FreeBSD is like macOS e.g. parameters for stat are the same. I assumed easy fix but I was wrong.
The behavior is somewhat different.

macOS

Shining:wfurt-runtime furt$ touch foo
Shining:wfurt-runtime furt$ stat -f "%b %k" foo
0 4096

FreeBSD

furt@d13:~ $ touch foo
furt@d13:~ $ stat -f "%b %k" foo
1 131072

The zfs shows one block used even if the file itself is empty.
To fix that I decided to use %z that simply returns allocated size on both macOS and FreeBSD

I did quick check and solaris seems to follow Linux e.g. "%b %B" and updated code should still work.
cc: @am11

cc: @Thefrank @rootwyrm @emaste

Author: wfurt
Assignees: wfurt
Labels:

area-System.IO, os-freebsd, test enhancement

Milestone: -

@am11
Copy link
Member

am11 commented Feb 9, 2022

@wfurt, I think it depends on filesystem and flavor of tool. e.g. on illumos / OpenIndiana, the primary file system is zFS (supports sparse files, with holes). Running under vagrant+virtualbox, I get this:

# create/touch empty files on zFS and vbox/vagrant filesystems
$ touch $HOME/foo
$ touch /vagrant/foo

# stat is preinstalled (from GNU coreutils)
$ stat -c "%b %B" $HOME/foo
1 512
$ stat -c "%b %B" /vagrant/foo
0 512

# but using the built-in du -A (apparent size)
$ du -A $HOME/foo
0       /export/home/vagrant/foo
$ du -A /vagrant/foo
0       /vagrant/foo

zFS is also available on FreeBSD and Linux.

@emaste
Copy link

emaste commented Feb 9, 2022

I'm curious what the test is actually trying to check here - perhaps it is trying to test some option for creation that is just not applicable here?

@am11
Copy link
Member

am11 commented Feb 9, 2022

@emaste, it is calculating file size using stat(1) (num of blocks x block size). Then it is using that information in assertions during the FileStream APIs tests, that support "preallocation" (fallocate / F_ALLOCATEALL) feature. There are tests like these, which shell out the command to known system utilities (stat(1) in this case). If there is no utility we can use, we resort to C implementation (currently there are very few FFI calls for test-only cases but it is possible). In this case, I think it makes sense to exec the known command line utility, because that is how user will typically compare the size and expects certain result which these tests ensure.

@rootwyrm
Copy link
Contributor

rootwyrm commented Feb 9, 2022

To clarify, the default filesystem on FreeBSD is not ZFS, it is UFS2. And within pipelines you should not have ZFS; it should be UFS2 + soft journalling.
The introduction of multiple filesystems is obviously an issue in and of itself, but the FreeBSD test should be written around UFS2 behavior by default, not ZFS behavior.

09:21 AM Wed Feb 09 [J:0 C:18 H:514] FreeBSD 13.0-RELEASE-p4
prj@mojache ~ $ touch foo
09:21 AM Wed Feb 09 [J:0 C:19 H:515] FreeBSD 13.0-RELEASE-p4
prj@mojache ~ $ stat -f "%b %k" foo
0 32768

Realistically we should break out this test into UFS, UFS2, and ZFS tests which pivot around fstyp or local equivalent checks. Solaris derivatives have UFS, FreeBSD from 3.0 has UFS2, Indiana has ZFS, and FreeBSD and Linux may have ZFS but do not use ZFS by default. And said ZFS may or may not be case sensitive and may or may not be doing sparse or zero. To be entirely clear, this does in fact mean that multiple OSes may have wildly different root file systems which are partially or fully supported despite being non-default.

Honestly, this test always kind of bugged me, because it doesn't really have a way to pivot around non-default filesystems or non-default filesystem configurations and non-standard handling (i.e. OS X treats exFAT as case sensitive, even though that's not how FAT works.)
I think the more appropriate fix here is instead of trying to be OS specific, to rewrite the entire test based on expected behaviors of a given file system and pivot around the file system type exclusively. That would resolve everything in a much more permanent and maintainable fashion.

@wfurt
Copy link
Member Author

wfurt commented Feb 9, 2022

I'm not sure how I end up with zfs on my machine. I look at my 12.x and does have ufs and the "%b %k" works as expected. I think it would be OK for now IMHO - I really did not want to get to business of rewriting or adding tests.

As far as the allocation: should we try to use posix_fallocate? (or somehow make it in pal to take the space)

@adamsitnik
Copy link
Member

As far as the allocation: should we try to use posix_fallocate? (or somehow make it in pal to take the space)

We have initially used posix_fallocate but then reverted the change. posix_fallocate pre-allocates the disk space and modifies EOF. So if a user asks us to pre-allocate a 1000 byte file, but writes only half of it, closes and re-opens the file it's length is going to be reported as 1000 (not 500).

@rootwyrm
Copy link
Contributor

Awareness / capability absolutely should still be in there looking at the pal cmake. But as @adamsitnik mentioned, using posix_fallocate() does have that behavior, as does native ftruncate. Both just lay down an explicit EOF regardless of contents. If preallocation was previously reverted, I'd stick with that decision.

@wfurt
Copy link
Member Author

wfurt commented Feb 16, 2022

I did more testing on UFS and the current stat flags are OK. So I decided to keep it to avoid debate on various file systems here. If we want improvements that can independently regardless of particular OS.

Many tests still fail here with

      Stack Trace:

          Child exception:
            System.IO.IOException: The process cannot access the file '/tmp/FileStream_LockUnlock_nm3knbvl.5ix/OverlappingRegionsFromOtherProcess_With_WriteLock_ThrowsException_237_603d100b' because it is being used by another process.
          /usr/home/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Unix.cs(24,0): at System.IO.Strategies.FileStreamHelpers.CheckFileCall(Int64 result, String path, Boolean ignoreNotSupported)
          /usr/home/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Unix.cs(64,0): at System.IO.Strategies.FileStreamHelpers.Lock(SafeFileHandle handle, Boolean canWrite, Int64 position, Int64 length)
          /usr/home/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/OSFileStreamStrategy.cs(182,0): at System.IO.Strategies.OSFileStreamStrategy.Lock(Int64 position, Int64 length)
          /usr/home/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs(982,0): at System.IO.Strategies.BufferedFileStreamStrategy.Lock(Int64 position, Int64 length)
          /usr/home/furt/github/wfurt-runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs(225,0): at System.IO.FileStream.Lock(Int64 position, Int64 length)
          /usr/home/furt/github/wfurt-runtime/src/libraries/System.IO.FileSystem/tests/FileStream/LockUnlock.cs(255,0): at System.IO.Tests.FileStream_LockUnlock.<>c.<OverlappingRegionsFromOtherProcess_With_WriteLock_ThrowsException>b__10_1(String path)

          Child process:
            System.IO.FileSystem.Tests, Version=7.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51 System.IO.Tests.FileStream_LockUnlock+<>c Void <OverlappingRegionsFromOtherProcess_With_WriteLock_ThrowsException>b__10_1(System.String)

          Child arguments:
            /tmp/FileStream_LockUnlock_nm3knbvl.5ix/OverlappingRegionsFromOtherProcess_With_WriteLock_ThrowsException_237_603d100b

      System.IO.Tests.FileStream_LockUnlock.OverlappingRegionsFromOtherProcess_With_WriteLock_ThrowsException(fileAccess: Write) [FAIL]

I'm inclined to open separate issue and disable failing tests agains it.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @wfurt !

@adamsitnik
Copy link
Member

Many tests still fail here with

I was hoping to fix it with #47994. From the information that you have provided it seems that unlocking does not work as expected?

The test is doing sth like this:

  1. Open the file and lock it - it works
  2. Ensure that locking it again from different process fails - it works
  3. Unlock the file - ???
  4. Try to lock it from different process - it fails

@wfurt is there any chance you could take a quick look and see what sys-calls are being performed for the following:

using FileStream fs1 = File.Open(path, FileMode.Open, fileAccess, FileShare.ReadWrite);
fs1.Lock(0, 100);
fs1.Unlock(0, 100);
fs1.Lock(0, 100);

@wfurt
Copy link
Member Author

wfurt commented Feb 23, 2022

I did what @adamsitnik asked for and I get:

16618: openat(AT_FDCWD,"/tmp/foo",O_RDWR|O_CLOEXEC,00) = 34 (0x22)
16618: flock(34,LOCK_SH|LOCK_NB)         = 0 (0x0)
16618: lseek(34,0x0,SEEK_CUR)            = 0 (0x0)
16618: lseek(34,0x0,SEEK_SET)            = 0 (0x0)
16618: fcntl(34,F_SETLK,0x7fffffffd928)      ERR#35 'Resource temporarily unavailable'

from man page:

     [EAGAIN]           The argument cmd is F_SETLK, the type of lock (l_type)
                        is a shared lock (F_RDLCK) or exclusive lock
                        (F_WRLCK), and the segment of a file to be locked is
                        already exclusive-locked by another process; or the
                        type is an exclusive lock and some portion of the
                        segment of a file to be locked is already shared-
                        locked or exclusive-locked by another process.

I did isolate the sequence as small repro:

#include <sys/file.h>
#include <stdio.h>
#include <string.h>
#include <errno.h>

int main(int argc, char** argv)
{
    struct flock fl;

    int fd= open("/tmp/foo", O_RDWR|O_CLOEXEC);
    int ret  = lseek(fd, 0, SEEK_SET);
    printf("Opened as %d\n", fd);
    if (argc > 1) {
        ret = flock(fd, LOCK_SH|LOCK_NB);
        printf("flock finished with %d\n", ret);
    }

    fl.l_start = 0;
    fl.l_len = 100;
    fl.l_type = F_WRLCK;
    fl.l_whence = SEEK_SET;

    ret = fcntl(fd, F_SETLK, &fl);
    printf("fcntl finished with %d and errno %d (%s)\n", ret, errno, strerror(errno));

    return 0;
}

if we skip the flock then the fcntl works as expect. e.g. it seems like we cannot use both at the same time.
On Linux, it works fine in both cases.

I noticed Lock_Unlock_Successful is skipped on macOS and man page for Lock (long position, long length); warns about platform availability. I'm wondering if we should do it as well for FreeBSD - at least for now.

@wfurt
Copy link
Member Author

wfurt commented Feb 24, 2022

I open #65831 for further discussion about Lock().

@wfurt wfurt merged commit 18d9496 into dotnet:main Feb 24, 2022
@wfurt wfurt deleted the fbsd_FileSystem branch February 24, 2022 07:19
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO os-freebsd FreeBSD OS test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants