Could this change to fn(u32) -> bool since that's basically what's being deduplicated here?

also could the name perhaps be something like is_readonly_for_win32 or something like that?

I have renamed it to user_can_write - I was staring at it and this jumped out at me; let me know if you would still prefer is_readonly_for_win32 : the function isn't actually win32 specific, its just only used on win32 today.

I don't want to change the signature though, because this cascades into the secure open function and it gets messy: that can't be a closure because there is already a closure borrowing self; as a function passing in windows specific parameters like 'readonly' will make the code hard to maintain as more os-specific cases occur, so I'd rather pass in only the actual data that is being operated on - e.g. the mode, and as mode is a monad, not a value, keeping the monad makes a lot of sense to me.

Let's please change this to u32 -> bool, unnecessarily having to wrap this in &Ok(...) in one callsite is pretty wonky.

I'll have a go at this, I started there and worked in to end up with the shape I got, but I'll see if I can make it not too ugly.

Actually now that I think about it, Unix also has OpenOptionsExt::mode which I think could be used here?

IIRC that interacts poorly with umask (compared to the semantics expected from e.g. bsd-tar/gnu-tar etc). I'm happy to look into that as a separate patch however.

It seems that the purpose of this patch is to reduce syscalls setting mode bits, but it only supports Windows, so it seems like a good place to me to add this for Unix? I don't really know how this would interact in a bad way with umask that isn't already exposed, and it seems like it would be simpler to basically remove the set_perms call below entirely.

I'm reasonably sure that chmod(2) which is being used today ignores umask, because its job is setting an explicit mode. But putting that aside.

https://linux.die.net/man/2/chmod suggests to me that we'll get bug reports doing this in creat on Unix (and may even need to tweak this further from doing it at createfile on windows to doing it as a windows specific call on the file handle (current setting readonly requires a stat-set cycle due to the stdlib abstractions).

The specific issue I see here is "On NFS file systems, restricting the permissions will immediately influence already open files, because the access control is done on the server, but open files are maintained by the client. Widening the permissions may be delayed for other clients if attribute caching is enabled on them. " - so untarring a file on NFS will result in the restricted mode being applied on each write and thus erroring.

I'm happy to rework this patch to use winapi to do the readonly setting on the open file without the stat syscalls and avoid this now-identified issue - I don't believe it will affect SMB shares, as SMB's server semantics are much more session oriented, but Windows does have NFS clients.

Oh, its also worth noting that linux's (and unix in general) decoupled model where IO is permitted to happen long after the client requesting it has gone admits much faster syscalls; fchmod() done late to the file as it is today should be supremely fast on linux and I see no need to change it. I can strace a rustup on linux and see if the std library is causing a lot of wasted syscalls there, and if so tune it as well - but because we don't have to do a metadata call, I don't think that that will be the case.

tl;dr: Unix already has less syscalls, and suffers an order of magnitude less from having them in the first place.

Hm ok, I'm not too particularly intersted in trying to dig into all the dark corners of Unix to see what's going on here. I would personally just like a consistent code path amongst all platforms, and both Unix/Windows support setting file modes on creation so it seems like we should use that. If someone runs into weird issues with weird filesystems later it can be handled, but before that I think internal consistency to ensure that this already very complicated wad of code is as simple as possible is better.

Ok, so this patch isn't going to meet your constraints; lets design a new one. The current code path is
create
write
set mtime on file handle
set mode on file handle
set xattr on file handle

setting the mode on the file handle is fine on unix - its a single syscall per file, and I'm pretty sure that as long as you have decent amounts of RAM available it won't block in any significant fashion.

On windows however, setting the mode looks like this:

                    let mut perm = f.metadata()?.permissions();
                    perm.set_readonly(mode & 0o200 != 0o200);
                    f.set_permissions(perm)

There are two performance considerations here.
One is that the set_permission syscall is actually the same one that setting the mtime uses: if we abstracted things a little differently we could make a single syscall to set the time and the readonly attribute. A small os-abstraction layer in tar-rs would let us present that cleanly I think; possibly filetime could grow the right interface, or we'd only use filetime for non-windows-family-os's.

Two is that f.metadata()? does two syscalls (I know its written as just GetFileInformationByHandle, but this is the strace:

11:33:59.0244734 AM	0.0001502	rustup.exe	25684	CreateFile	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	SUCCESS	Desired Access: Generic Write, Read Attributes, Disposition: Create, Options: Synchronous IO Non-Alert, Non-Directory File, Open Reparse Point, Attributes: n/a, ShareMode: Read, Write, Delete, AllocationSize: 0, OpenResult: Created
11:33:59.0246571 AM	0.0000810	rustup.exe	25684	WriteFile	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	SUCCESS	Offset: 0, Length: 8,192, Priority: Normal
11:33:59.0247826 AM	0.0000287	rustup.exe	25684	WriteFile	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	SUCCESS	Offset: 8,192, Length: 8,192, Priority: Normal
11:33:59.0248378 AM	0.0000088	rustup.exe	25684	WriteFile	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	SUCCESS	Offset: 16,384, Length: 8,192
11:33:59.0248782 AM	0.0000229	rustup.exe	25684	WriteFile	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	SUCCESS	Offset: 24,576, Length: 3,885, Priority: Normal
11:33:59.0249274 AM	0.0000036	rustup.exe	25684	QueryInformationVolume	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	SUCCESS	VolumeCreationTime: 4/01/2019 2:14:43 PM, VolumeSerialNumber: DE41-BA52, SupportsObjects: True, VolumeLabel: 
11:33:59.0249425 AM	0.0000440	rustup.exe	25684	QueryAllInformationFile	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	BUFFER OVERFLOW	CreationTime: 12/05/2019 11:33:59 AM, LastAccessTime: 12/05/2019 11:33:59 AM, LastWriteTime: 12/05/2019 11:33:59 AM, ChangeTime: 12/05/2019 11:33:59 AM, FileAttributes: A, AllocationSize: 49,152, EndOfFile: 28,461, NumberOfLinks: 1, DeletePending: False, Directory: False, IndexNumber: 0x2f00000007eeff, EaSize: 0, Access: Generic Write, Read Attributes, Position: 28,461, Mode: Synchronous IO Non-Alert, AlignmentRequirement: Long
11:33:59.0250234 AM	0.0000123	rustup.exe	25684	SetBasicInformationFile	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	SUCCESS	CreationTime: 1/01/1601 12:00:00 PM, LastAccessTime: 1/01/1601 12:00:00 PM, LastWriteTime: 1/01/1601 12:00:00 PM, ChangeTime: 1/01/1601 12:00:00 PM, FileAttributes: A
11:33:59.0250541 AM	0.0006331	rustup.exe	25684	CloseFile	C:\Users\robertc\.rustup\tmp\d__ktssiid9tznn8_dir\rust-docs\share\doc\rust\html\cargo\book.css	SUCCESS	

or about 5 hundred-thousands of a second; so for 20k files, thats about 1 second of total runtime, assuming no marshalling overhead etc. As with Unix, we just created the file, we know the attributes we want, so in principle we can set them directly without the intervening file_attr() call; again this could be hidden from the entry() flow to keep that clean and understandable, via a small os module.

But pragmatically; we don't need all of this for rustup. What we need for rustup is a lot simpler, and thats just to stop making the syscalls; as we don't have readonly files there, I'll put forward a SUPER minimal patch and see what you think :).

Mk so now that we've got the minimal for rustup in I'm actually wondering if it's perhaps best to leave as-is for now until it's an issue? I suspect that large tarballs of readonly files is probably relatively rare.

In your strace though, do you know what's up with the exponentially growing write sizes? Shouldn't rustup write everything in one go?

The write lengths are 8K each time, the offsets are 0, 8K, 16K, 24K. The durations are all over the place - 8,3,1,2 (*10^-6 each).

To write everything in one go, rustup needs to hook into the file output being done by tar-rs - currently thats not possible (at least as far as I understand rust), as tar-rs does the open directly, not through any trait object; and, there is no callback to permit rustup to add an additional BufWriter or similar object around the the File.

But I wouldn't rush to add that just yet; I've done some basic experiments, and at least for rustup, so many of the files are very small - its 20k files, 200M of content, average of 10k per file - that they are already a single write; the small number of quite large files like the js index help drive the common case below 8k. Adding a BufWriter actually slowed things down due to heap allocations.
I have experimented with patching the stdlib to permit using a larger io::copy buf size but I'm not happy with the outcomes there yet.

I'm happy closing this ticket for now. It's certainly not an itch I have any personal stake in scratching. I agree that very large tars of readonly files in performance-sensitive scenarios is probably quite the edge case.