fall back to slower stat filesize if optimized filesize fails

[IanButterworth]

Based on the triage suggestion #54203 (comment)

[giordano]

@ronisbr would you be able to test this?

[ronisbr]

@ronisbr would you be able to test this?

I will try this weekend! I am currently without access to a Raspberry Pi to test.

@JakeZw

Can you test this PR?

[JakeZw]

I have the RPI behind me. What is the best way to update my iostream.jl file to test it? Jake

[giordano]

Go to the build page, select the build aarch64-Linux-gnu job, open the Artifacts tab, and download the tarball. Open it and you'll have the Julia build on your disk. Maybe there's a way to access the build with juliaup, but I'm not up to date with that.

[KristofferC]

Or copy paste the new function in a @eval Base block.

[JakeZw]

I used giordano's method.

The command that has been causing problems in BaremetalPi is gpio_init(), in which the following commands are executed and then stored in a structure.

        using Mmap
        gpiomem_io  = open("/dev/gpiomem", "w+");
        gpiomem_map = Mmap.mmap(
            gpiomem_io,
            Vector{UInt32},
            (1024,),
            0,
            grow = false
        );

With Julia 1.10.0 it all works.

On the development version that I just downloaded
I get the following output

julia> gpiomem_map = Mmap.mmap(
           gpiomem_io,
           Vector{UInt32},
           (1024,),
           0,
           grow = false
       );
ERROR: ArgumentError: requested size 4096 larger than file size 0, but requested not to grow
Stacktrace:
 [1] mmap(io::IOStream, ::Type{Vector{UInt32}}, dims::Tuple{Int64}, offset::Int64; grow::Bool, shared::Bool)
   @ Mmap ~/Downloads/julia-984f884dfe/share/julia/stdlib/v1.12/Mmap/src/Mmap.jl:226
 [2] top-level scope
   @ REPL[22]:1

Awaiting instructions.

[JakeZw]

Or copy paste the new function in a @eval Base block.

This is a bit cryptic for me.

[IanButterworth]

Seems like you need to set grow = true?

[JakeZw]

Still not working

julia> gpiomem_map = Mmap.mmap(
           gpiomem_io,
           Vector{UInt32},
           (1024,),
           0,
           grow = true
       );
ERROR: SystemError: ftruncate: Invalid argument
Stacktrace:
 [1] systemerror(p::Symbol, errno::Int32; extrainfo::Nothing)
   @ Base ./error.jl:185
 [2] systemerror
   @ ./error.jl:184 [inlined]
 [3] grow!(io::IOStream, offset::Int64, len::Int64)
   @ Mmap ~/Downloads/julia-984f884dfe/share/julia/stdlib/v1.12/Mmap/src/Mmap.jl:91
 [4] mmap(io::IOStream, ::Type{Vector{UInt32}}, dims::Tuple{Int64}, offset::Int64; grow::Bool, shared::Bool)
   @ Mmap ~/Downloads/julia-984f884dfe/share/julia/stdlib/v1.12/Mmap/src/Mmap.jl:224
 [5] top-level scope
   @ REPL[24]:1

julia>

[JakeZw]

I don't think filesize() is the source of the BaremetalPi problem. I compared the working and not working versions of the function filesize and they don't correspond to when the problem occurs. See the comments below for what is working and what is not working. I also took at look Mmap.map and it has changed from 1.10.0 to 1.10.1 and is different again for 1.12.0. I will report back to the BaremetalPi issue tracker.

# version 1.10.0  BaremetalPi works
function filesize(s::IOStream)
sz = @_lock_ios s ccall(:ios_filesize, Int64, (Ptr{Cvoid},), s.ios)
if sz == -1
err = Libc.errno()
throw(IOError(string("filesize: ", Libc.strerror(err), " for ", s.name), err))
end
return sz
end

version 1.10.5 BaremetalPi does not work
function filesize(s::IOStream)
sz = @_lock_ios s ccall(:ios_filesize, Int64, (Ptr{Cvoid},), s.ios)
if sz == -1
err = Libc.errno()
throw(IOError(string("filesize: ", Libc.strerror(err), " for ", s.name), err))
end
return sz
end

# version 1.12.0 BaremetalPi does not work
function filesize(s::IOStream)
sz = @_lock_ios s ccall(:ios_filesize, Int64, (Ptr{Cvoid},), s.ios)
if sz == -1
# if `s` is not seekable `ios_filesize` can fail, so fall back to slower stat method
sz = filesize(stat(s))
end
return sz
end

[ronisbr]

The problem is the filesize function. There was a modification in Mmap.mmap and it now calls the function filesize (which was not being called before) that leads to the problem in raspberry pi due to the file /dev/gpiomem.

[JakeZw]

Filesize(io) returns 0 with this patch so it is working.

I see what you mean about Mmap,mmap having been changed from 1.10.0 to 1.10.1. This may have introduced another problem?

[vtjnash]

I think we just need a check for isfile(t) on the stat result before using the result of the filesize, if the result is 0, since that filters out non-regular files, since we know that will fail the ftruncate syscall, but also won't be useful to call:

  EINVAL fd does not reference a regular file or a POSIX shared memory object.

[vtjnash]

(in addition to this PR or after this PR)

[JakeZw]

It is failing on line 91 of grow! in the Mmap.jl file where the systemcall on line 90 returns failure = -1. Complete error message shown above.

[IanButterworth]

@vtjnash I gave that a go here. Could be separated into another PR, but for testing purposes here for now.

I couldn't see a tidy way to reuse the stat call within filesize

[JakeZw]

if I change your line to

filelen == 0 && !isfile(io) && return  # note the ! before isfile

I no longer get an error. I am in process of testing this with BaremetalPi.

It seems that @ronisbr prevented Mmap from using grow! by setting the option grow=false when mmap was called. With the current proposed fix I need to set grow=true, but then grow! returns before performing any function, which seems counterintuitive. I am wondering if the following logic in mmap (lines 220 to 231) should be reviewed and the case of !isfile(io) with grow=false included in the logic there?

    @static if Sys.isunix()
        prot, flags, iswrite = settings(file_desc, shared)
        if requestedSizeLarger
            if iswrite
                if grow
                    grow!(io, offset, len)
                else
                    throw(ArgumentError("requested size $szfile larger than file size $(filesize(io)), but requested not to grow"))
                end
            else
                throw(ArgumentError("unable to increase file size to $szfile due to read-only permissions"))
            end
        end

[ronisbr]

It seems that @ronisbr prevented Mmap from using grow! by setting the option grow=false when mmap was called. With the current proposed fix I need to set grow=true, but then grow! returns before performing any function, which seems counterintuitive. I am wondering if the following logic in mmap (lines 220 to 231) should be reviewed and the case of !isfile(io) included in the logic there?

Yes, I agree. However, I can change BaremetalPi.jl to use grow = true if required. It is just that technically you do not need to grow that file.

[JakeZw]

With the fix shown two posts up and a dev version of BaremetalPi with grow set to true, my application that uses BaremetalPi is working.

[oscardssmith]

by "needs a test", Ian means it needs a test in CI, but it's somewhat unclear how we can do that. IMO since this is a bugfix we should merge without a test if we don't have one.

[IanButterworth]

I was imagining that we could either use some address on a linux system to mimic the gpio output, or mock it.
If that isn't easy then I guess merge without tests, but open an issue to note that it's untested?

[JakeZw]

From https://github.com/ronisbr/BaremetalPi.jl/blob/master/src/gpio.jl

the commands that errored are:

  gpiomem_io  = open("/dev/gpiomem", "w+")
  gpiomem_map = Mmap.mmap(
            gpiomem_io,
            Vector{UInt32},
            (GPIOMEM_SIZE,),
            0,
            grow = false
        )

If we change grow = true the proposed changes work.

If we leave grow = false then we need the additional change on line 222 that I proposed above.

This code segment should work as a test.

[ronisbr]

The problem is that this scenario only works in a Raspberry Pi, which cannot be emulated by CI here in GitHub.

[IanButterworth]

Given there are ongoing questions here about what is needed to fix Mmap, I will reduce this to just the issue in the title so it can be merged, then another PR opened to fix the remaining issues.

[IanButterworth]

Also, seems we don't have a good idea for a test here, so I'll remove that needs label.

[ronisbr]

Hi @IanButterworth

Given there are ongoing questions here about what is needed to fix Mmap, I will reduce this to just the issue in the title so it can be merged, then another PR opened to fix the remaining issues.

Is it really necessary? I thought everything was fine with that suggestion. The current state does not fix #54203. Since this is an important regression (nobody is able to use Julia in the current state to access Raspberry Pi's GPIO), can't we try to implement the solution to #54203 in this PR to fix the problem faster?

[ronisbr]

By the way, the problem is that this PR:

https://github.com/JuliaLang/julia/pull/44354/files

now perform checks that were not performed before (like throwing an error in some conditions).

[IanButterworth]

The Mmap fixes have been opened here #55849

@JakeZw opening PRs isn't as complicated as the guide at #55641 (comment) makes out.

@ronisbr there's just been a lot of noise in this PR and the title fix should get in. See #55849 for the remainder. Please review & approve that if it is correct.

[ronisbr]

@ronisbr there's just been a lot of noise in this PR and the title fix should get in. See #55849 for the remainder. Please review & approve that if it is correct.

Perfect @IanButterworth ! You were very fast :) I just tested both modifications and, from my side, everything is working perfectly (Raspberry Pi Zero 2). However, I think it will be important if @JakeZw test it too.