Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Mark Millard]

On 2017-Nov-4, at 4:58 AM, Ed Maste  wrote:

> On 4 November 2017 at 07:41, Andriy Gapon  wrote:
>> On 04/11/2017 12:32, Mark Millard wrote:
>>>  if (int Err = ::posix_fallocate(FD, 0, Size)) {
>>>if (Err != EOPNOTSUPP)
>>>  return std::error_code(Err, std::generic_category());
>>>  }
>> 
>> The commit message that you didn't include into your reply contains some 
>> useful
>> information that authors / maintainers of this code should probably take into
>> account:
>> 
>>>  Please note that EINVAL is used to report that the underlying file system
>>>  does not support the operation (POSIX.1-2008).
>> 
>> Here is a link for that:
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html
> 
> I have no idea how they decided EINVAL was a reasonable errno for this case.

I think they viewed it as a bad fd argument: a reference
into a wrong file system, much like a wrong len (<0) or
offset (<0).

That there is no other means of run-time classifying the
file system(s)(?) was not viewed as sufficient reason
to give it a different classification.

But it is just a guess.

===
Mark Millard
markmi at dsl-only.net

___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"

Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Mark Millard]

[The patch allowed the amd64 -> aarch64 cross-buildworld
to complete instead of failing in lld.]

On 2017-Nov-4, at 10:13 AM, Mark Millard  wrote:

> On 2017-Nov-4, at 10:02 AM, Mark Millard  wrote:
> 
> 
>> On 2017-Nov-4, at 4:58 AM, Ed Maste  wrote:
>> 
>>> On 4 November 2017 at 07:41, Andriy Gapon  wrote:
 On 04/11/2017 12:32, Mark Millard wrote:
> if (int Err = ::posix_fallocate(FD, 0, Size)) {
>  if (Err != EOPNOTSUPP)
>return std::error_code(Err, std::generic_category());
> }
 
 The commit message that you didn't include into your reply contains some 
 useful
 information that authors / maintainers of this code should probably take 
 into
 account:
 
> Please note that EINVAL is used to report that the underlying file system
> does not support the operation (POSIX.1-2008).
 
 Here is a link for that:
 http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html
>>> 
>>> I have no idea how they decided EINVAL was a reasonable errno for this case.
>>> 
>>> Mark, can you give this patch a try:
>>> 
>>> diff --git a/contrib/llvm/lib/Support/Unix/Path.inc
>>> b/contrib/llvm/lib/Support/Unix/Path.inc
>>> index 45097eb918b7..67edb46f0025 100644
>>> --- a/contrib/llvm/lib/Support/Unix/Path.inc
>>> +++ b/contrib/llvm/lib/Support/Unix/Path.inc
>>> @@ -427,7 +427,7 @@ std::error_code resize_file(int FD, uint64_t Size) {
>>> // If we have posix_fallocate use it. Unlike ftruncate it always allocates
>>> // space, so we get an error if the disk is full.
>>> if (int Err = ::posix_fallocate(FD, 0, Size)) {
>>> -if (Err != EOPNOTSUPP)
>>> +if (Err != EINVAL && Err != EOPNOTSUPP)
>>> return std::error_code(Err, std::generic_category());

This change allowed the amd64 -> aarch64 cross-buildworld
(and buildkernel) to finish.

Thanks.


===
Mark Millard
markmi at dsl-only.net


___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"

Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Mark Millard]

On 2017-Nov-4, at 10:02 AM, Mark Millard  wrote:


> On 2017-Nov-4, at 4:58 AM, Ed Maste  wrote:
> 
>> On 4 November 2017 at 07:41, Andriy Gapon  wrote:
>>> On 04/11/2017 12:32, Mark Millard wrote:
 if (int Err = ::posix_fallocate(FD, 0, Size)) {
   if (Err != EOPNOTSUPP)
 return std::error_code(Err, std::generic_category());
 }
>>> 
>>> The commit message that you didn't include into your reply contains some 
>>> useful
>>> information that authors / maintainers of this code should probably take 
>>> into
>>> account:
>>> 
 Please note that EINVAL is used to report that the underlying file system
 does not support the operation (POSIX.1-2008).
>>> 
>>> Here is a link for that:
>>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html
>> 
>> I have no idea how they decided EINVAL was a reasonable errno for this case.
>> 
>> Mark, can you give this patch a try:
>> 
>> diff --git a/contrib/llvm/lib/Support/Unix/Path.inc
>> b/contrib/llvm/lib/Support/Unix/Path.inc
>> index 45097eb918b7..67edb46f0025 100644
>> --- a/contrib/llvm/lib/Support/Unix/Path.inc
>> +++ b/contrib/llvm/lib/Support/Unix/Path.inc
>> @@ -427,7 +427,7 @@ std::error_code resize_file(int FD, uint64_t Size) {
>>  // If we have posix_fallocate use it. Unlike ftruncate it always allocates
>>  // space, so we get an error if the disk is full.
>>  if (int Err = ::posix_fallocate(FD, 0, Size)) {
>> -if (Err != EOPNOTSUPP)
>> +if (Err != EINVAL && Err != EOPNOTSUPP)
>>  return std::error_code(Err, std::generic_category());
> 
> I've got a simple buildworld going but I expect that
> it will end up using lld in a form that runs into
> the problem. But I may luck out since I can link a
> trivial main to produce an a.out for amd64.

Actually I take that back: I no longer have
WITH_LLD_IS_LD= as part of my normal amd64
environment. (I did for a time.)

So I will not get the problem.

> It may be appropriate to have notes somewhere about
> what to do for folks that land in the range -r325320
> to whatever revision the updated
> contrib/llvm/lib/Support/Unix/Path.inc ends up at
> and that also have a zfs filesystem context involved.

Explicitly adding to that context-requirement for
having the problem for amd64: and that one has
WITH_LLD_IS_LD= in use.

Of course, for arm64.aarch64 WITH_LLD_IS_LD= is the
normal case and so would be more likely to catch
folks. So this too should be explicit.

> I'll let you know if the build completes vs. not. It
> takes a while since llvm materials are rebuilding.

It should complete since binutils's ld is in use:
I'm not building on aarch64 and reverted to normal
for amd64 some time ago relateive to WITH_LLD_IS_LD= .

===
Mark Millard
markmi at dsl-only.net

___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"

Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Mark Millard]

On 2017-Nov-4, at 4:58 AM, Ed Maste  wrote:

> On 4 November 2017 at 07:41, Andriy Gapon  wrote:
>> On 04/11/2017 12:32, Mark Millard wrote:
>>>  if (int Err = ::posix_fallocate(FD, 0, Size)) {
>>>if (Err != EOPNOTSUPP)
>>>  return std::error_code(Err, std::generic_category());
>>>  }
>> 
>> The commit message that you didn't include into your reply contains some 
>> useful
>> information that authors / maintainers of this code should probably take into
>> account:
>> 
>>>  Please note that EINVAL is used to report that the underlying file system
>>>  does not support the operation (POSIX.1-2008).
>> 
>> Here is a link for that:
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html
> 
> I have no idea how they decided EINVAL was a reasonable errno for this case.
> 
> Mark, can you give this patch a try:
> 
> diff --git a/contrib/llvm/lib/Support/Unix/Path.inc
> b/contrib/llvm/lib/Support/Unix/Path.inc
> index 45097eb918b7..67edb46f0025 100644
> --- a/contrib/llvm/lib/Support/Unix/Path.inc
> +++ b/contrib/llvm/lib/Support/Unix/Path.inc
> @@ -427,7 +427,7 @@ std::error_code resize_file(int FD, uint64_t Size) {
>   // If we have posix_fallocate use it. Unlike ftruncate it always allocates
>   // space, so we get an error if the disk is full.
>   if (int Err = ::posix_fallocate(FD, 0, Size)) {
> -if (Err != EOPNOTSUPP)
> +if (Err != EINVAL && Err != EOPNOTSUPP)
>   return std::error_code(Err, std::generic_category());

I've got a simple buildworld going but I expect that
it will end up using lld in a form that runs into
the problem. But I may luck out since I can link a
trivial main to produce an a.out for amd64.

It may be appropriate to have notes somewhere about
what to do for folks that land in the range -r325320
to whatever revision the updated
contrib/llvm/lib/Support/Unix/Path.inc ends up at
and that also have a zfs filesystem context involved.

I'll let you know if the build completes vs. not. It
takes a while since llvm materials are rebuilding.

===
Mark Millard
markmi at dsl-only.net

___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"

Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Andriy Gapon]

On 04/11/2017 13:58, Ed Maste wrote:
> I have no idea how they decided EINVAL was a reasonable errno for this case.

I completely agree.  That's a weird choice that I have not seen for any other 
API.

-- 
Andriy Gapon
___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"

Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Andriy Gapon]

On 04/11/2017 13:41, Andriy Gapon wrote:
> On 04/11/2017 12:32, Mark Millard wrote:
>>   if (int Err = ::posix_fallocate(FD, 0, Size)) {
>> if (Err != EOPNOTSUPP)
>>   return std::error_code(Err, std::generic_category());
>>   }
> 
> The commit message that you didn't include into your reply contains some 
> useful
> information that authors / maintainers of this code should probably take into
> account:
> 
>>   Please note that EINVAL is used to report that the underlying file system
>>   does not support the operation (POSIX.1-2008).
> 
> Here is a link for that:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html
> 

My response above is quite dry, so I want to add this.
Thank you very much for the deep analysis.
I am sorry for the trouble that my change caused, but I think that its root
cause lies elsewhere (lld, posix).

-- 
Andriy Gapon
___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"

Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Ed Maste]

On 4 November 2017 at 07:41, Andriy Gapon  wrote:
> On 04/11/2017 12:32, Mark Millard wrote:
>>   if (int Err = ::posix_fallocate(FD, 0, Size)) {
>> if (Err != EOPNOTSUPP)
>>   return std::error_code(Err, std::generic_category());
>>   }
>
> The commit message that you didn't include into your reply contains some 
> useful
> information that authors / maintainers of this code should probably take into
> account:
>
>>   Please note that EINVAL is used to report that the underlying file system
>>   does not support the operation (POSIX.1-2008).
>
> Here is a link for that:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html

I have no idea how they decided EINVAL was a reasonable errno for this case.

Mark, can you give this patch a try:

diff --git a/contrib/llvm/lib/Support/Unix/Path.inc
b/contrib/llvm/lib/Support/Unix/Path.inc
index 45097eb918b7..67edb46f0025 100644
--- a/contrib/llvm/lib/Support/Unix/Path.inc
+++ b/contrib/llvm/lib/Support/Unix/Path.inc
@@ -427,7 +427,7 @@ std::error_code resize_file(int FD, uint64_t Size) {
   // If we have posix_fallocate use it. Unlike ftruncate it always allocates
   // space, so we get an error if the disk is full.
   if (int Err = ::posix_fallocate(FD, 0, Size)) {
-if (Err != EOPNOTSUPP)
+if (Err != EINVAL && Err != EOPNOTSUPP)
   return std::error_code(Err, std::generic_category());
___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"

Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Andriy Gapon]

On 04/11/2017 12:32, Mark Millard wrote:
>   if (int Err = ::posix_fallocate(FD, 0, Size)) {
> if (Err != EOPNOTSUPP)
>   return std::error_code(Err, std::generic_category());
>   }

The commit message that you didn't include into your reply contains some useful
information that authors / maintainers of this code should probably take into
account:

>   Please note that EINVAL is used to report that the underlying file system
>   does not support the operation (POSIX.1-2008).

Here is a link for that:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_fallocate.html

-- 
Andriy Gapon
___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"

Re: svn commit: r325320 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs [breaks lld on zfs: lld uses fallocate]

[Mark Millard]

> Author: avg
> Date: Thu Nov  2 13:49:08 2017
> New Revision: 325320
> URL: 
> https://svnweb.freebsd.org/changeset/base/325320
> 
> 
> Log:
>   Disable posix_fallocate(2) for ZFS
> . . .

Turns out lld uses fallocate and so can fail
on zfs now.

The following is the lld for a amd64 -> aarch64
cross-buildworld.

/usr/obj/cortexA53_clang/arm64.aarch64/usr/src/arm64.aarch64/tmp/usr/bin/ld: 
error: cannot open output file a.out: Invalid argument

This resulted from:

Breakpoint 5, 0x00cf1cd1 in llvm::sys::fs::resize_file(int, unsigned 
long) ()
(gdb) disass
Dump of assembler code for function _ZN4llvm3sys2fs11resize_fileEim:
. . .
   0x00cf1ce5 <+21>:callq  0x1ad5880 
. . .

via the error status return value handling. It ends up with:

Breakpoint 3, 0x0041c6e4 in lld::elf::error(llvm::Twine const&) ()
(gdb) bt
#0  0x0041c6e4 in lld::elf::error(llvm::Twine const&) ()
#1  0x004113b1 in void 
lld::elf::LinkerDriver::link >(llvm::opt::InputArgList&) ()
#2  0x0040be3f in lld::elf::LinkerDriver::main(llvm::ArrayRef, bool) ()
#3  0x0040ae89 in lld::elf::link(llvm::ArrayRef, bool, 
llvm::raw_ostream&) ()
#4  0x0054cd61 in main ()


Progressing from posix_fallocate's call to its caller
and so on:

# grep -r "fallocate" /usr/src/contrib/llvm/ | more
/usr/src/contrib/llvm/lib/Support/Unix/Path.inc:  // If we have posix_fallocate 
use it. Unlike ftruncate it always allocates
/usr/src/contrib/llvm/lib/Support/Unix/Path.inc:  if (int Err = 
::posix_fallocate(FD, 0, Size)) {

Is called by:

std::error_code resize_file(int FD, uint64_t Size) {
#if defined(HAVE_POSIX_FALLOCATE)
  // If we have posix_fallocate use it. Unlike ftruncate it always allocates
  // space, so we get an error if the disk is full.
  if (int Err = ::posix_fallocate(FD, 0, Size)) {
if (Err != EOPNOTSUPP)
  return std::error_code(Err, std::generic_category());
  }
#endif
  // Use ftruncate as a fallback. It may or may not allocate space. At least on
  // OS X with HFS+ it does.
  if (::ftruncate(FD, Size) == -1)
return std::error_code(errno, std::generic_category());

  return std::error_code();
}

# grep -r "resize_file" /usr/src/contrib/llvm/ | more
/usr/src/contrib/llvm/lib/Support/FileOutputBuffer.cpp:  EC = 
sys::fs::resize_file(FD, Size);
/usr/src/contrib/llvm/lib/Support/Unix/Path.inc:std::error_code resize_file(int 
FD, uint64_t Size) {
/usr/src/contrib/llvm/lib/Support/Windows/Path.inc:std::error_code 
resize_file(int FD, uint64_t Size) {
/usr/src/contrib/llvm/include/llvm/Support/FileSystem.h:std::error_code 
resize_file(int FD, uint64_t Size);

Is called by:

ErrorOr
FileOutputBuffer::create(StringRef FilePath, size_t Size, unsigned Flags) {
  // Check file is not a regular file, in which case we cannot remove it.
. . .
#ifndef LLVM_ON_WIN32
  // . . .
  EC = sys::fs::resize_file(FD, Size);
  if (EC)
return EC;
#endif

Is called by:

std::error_code elf::tryCreateFile(StringRef Path) {
  if (Path.empty())
return std::error_code();
  return FileOutputBuffer::create(Path, 1).getError();
}

Is called by:

template  void LinkerDriver::link(opt::InputArgList ) {
  SymbolTable Symtab;
  elf::Symtab::X = 
  Target = getTarget();

  Config->MaxPageSize = getMaxPageSize(Args);
  Config->ImageBase = getImageBase(Args);

  // Default output filename is "a.out" by the Unix tradition.
  if (Config->OutputFile.empty())
Config->OutputFile = "a.out";

  // Fail early if the output file or map file is not writable. If a user has a
  // long link, e.g. due to a large LTO link, they do not wish to run it and
  // find that it failed because there was a mistake in their command-line.
  if (auto E = tryCreateFile(Config->OutputFile))
error("cannot open output file " + Config->OutputFile + ": " + E.message());


And the error call is then made once
tryCreateFile returns.

===
Mark Millard
markmi at dsl-only.net

___
freebsd-toolchain@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-toolchain
To unsubscribe, send any mail to "freebsd-toolchain-unsubscr...@freebsd.org"