search

pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.49k stars 379 forks source link

Proposal: Standardized FileInfo Structure. RE: z/OS performance investigation #581

Closed dustin-ward closed 3 months ago

dustin-ward commented 3 months ago

Hello, Im with the Go on z/OS dev team at IBM. We got a request to look into some performance issues with this package and zos. (#578
& #579)

When reading a file located on a linux machine to a zos machine, we see the poor performance reported by our users. The issue seems to be that despite enabling concurrent reads, checking the filemode forces sequential reading.

The fs.Mode returned from the remote machine is consistent with the linux format, but the zos syscall constants are used to mask and compare the value in isRegular(). These constants do not match the linux format and cause incorrect behaviour in isRegular() (And possibly any other functions that rely on the stat constants being consistent between machines)

Bypassing the !isRegular() check (forcing concurrent reads) shows performance consistent with the systems default sftp.

Maybe converting the stat packet response to the standardized fs.FileInfo struct could allow for the use of FileMode.IsRegular()? I'd be happy to take a stab at making the change, but I'm just looking for some guidance or alternative suggestions first.

Thanks!

puellanivis commented 3 months ago

Hm… we’ve had to deal with this before, with windows, plan9 and js/wasm. https://github.com/pkg/sftp/blob/master/syscall_fixed.go#L4-L6

I’m afraid that “convert the stat packet response to the standardized fs.FileInfo” won’t be as easy as it is said. Considering various other syscall values are going to break just as much in the conversion: https://github.com/pkg/sftp/blob/master/stat_posix.go#L56-L71

Unfortunately, it looks like simply adding zos to the list of build tags for syscall_fixed.god won’t be enough, since while that will get us the “right” S_IFMT, the other syscall.S_IF* parameters are going to still be different.

It might just be necessary to implement a new stat_zos.go file and build-tag exclude zos from stat_posix.go. A lot of code might be copied, but it will allow you more control over ensuring that you’re using the proper “linux” format values (probably more so these are the openbsd values, which just happened to be the same as linux when openssh was released). It looks like they eventually realized just how incompatible type values of the permissions could be, later defining a whole new field to hold that type information in a formalized, codified, and consistent way.

😩 Unfortunately, this is some of the most painstaking work in the repo, ensuring compliance for all the various operating systems shoving values through the I-Assumed-The-POSIX-Standards-Set-Specific-Values-For-This standardized-by-implementation fields.

We’ll kind of need to audit all the syscall use for the large number of OS-dependent files (fortunately, we can ignore windows and plan9 specific stuff at least)… or maybe we accelerate our integration of https://github.com/pkg/sftp/blob/master/internal/encoding/ssh/filexfer/permissions.go which just blanket defines all the OpenBSD filemode values for the wire, and ignore whatever is in the syscall package.

P.S.: Also, thanks so much for looking into what’s wrong here. Narrowing it down the S_IFMT gave me enough lead to know exactly where things have gone wrong, considering we’ve had to address this stuff for plan9 as well.