archive/tar: re-add sparse file support #22735
Comments
Automatic creation of sparse tar archives should actually be a non-goal. That is, we should not generate sparse archives with zero changes to user code. Instead, creation of sparse archives should occur with only 1 simple change to their code if they want sparse archives. In other words, make sparse support an easy opt-in, but not automatic. The two most common implementations, GNU tar and BSD tar, both understand sparse headers. The problem lies with a long-tail of other tar implementations that do not understand sparse headers. For example,
For the reason I stated earlier I'm opposed to Also, I feel uncomfortable that
Automatic (attempt at) creation of sparse files on the filesystem sounds fine, since OS or FS that don't support them still write a valid file (except for NaCl; see #21728), but I do have hesitation about calling OS specific methods in
I support having all of the OS-specific sparse logic in package
I like how easy it is to convert between the two semantics, but I would feel more comfortable if there was some bit of type safety. We can discuss further when discussing the change to
invertSparseEntries is an implementation details. It has the useful property that it normalizes the offsets, so we would still need something like it. That is, if there are two holes adjacent to each other, invertSparseEntries combines them. |
|
Change https://golang.org/cl/78030 mentions this issue: |
|
Thanks for the CL. I see your point about the Writer not doing it automatically. I'm OK with that. Reader.WriteTo doesn't seem like it really needs any OS-specific stuff at all. Assuming a new SetSparse method, it just has to sniff for SetSparse+Truncate+Seek. And actually Truncate is basically optional, it's just SetSparse and Seek. |
|
I still think of Are you going to propose changes to |
This CL removes the following APIs:
type SparseEntry struct{ ... }
type Header struct{ SparseHoles []SparseEntry; ... }
func (*Header) DetectSparseHoles(f *os.File) error
func (*Header) PunchSparseHoles(f *os.File) error
func (*Reader) WriteTo(io.Writer) (int, error)
func (*Writer) ReadFrom(io.Reader) (int, error)
This API was added during the Go1.10 dev cycle, and are safe to remove.
The rationale for reverting is because Header.DetectSparseHoles and
Header.PunchSparseHoles are functionality that probably better belongs in
the os package itself.
The other API like Header.SparseHoles, Reader.WriteTo, and Writer.ReadFrom
perform no OS specific logic and only perform the actual business logic of
reading and writing sparse archives. Since we do know know what the API added to
package os may look like, we preemptively revert these non-OS specific changes
as well by simply commenting them out.
Updates #13548
Updates #22735
Change-Id: I77842acd39a43de63e5c754bfa1c26cc24687b70
Reviewed-on: https://go-review.googlesource.com/78030
Reviewed-by: Russ Cox <rsc@golang.org>
dsnet
changed the title
AFAICT, on Windows, you can't create sparse zero areas by seeking, as the MSDN documentation clearly states: https://msdn.microsoft.com/it-it/library/windows/desktop/aa365566%28v=vs.85%29.aspx The blog post hints that you can set the file sparse and create immediately a full-size sparse span, so that later writers+seeks would basically fragment it but leaving sparse areas under the seeks. I have no clue if there is an impact on performance with this approach, and also it doesn't really belong to a Note that this was discussed at length in #13548, where also your proposal of lazy header writing was analyzed and discarded. |
bradfitz
added
NeedsFix
Support for sparse files does not exist in the implementation of golang tar. This is sad and tracked in [0]. We need this so we should look at supporting it again. This commit implements a naive version of hole skipping by just seeking over the "\0" parts of the file if the type of the file is `tar.TypeGNUSparse`. This copies parts of the io.go::copyBuffer() code (unfortunately). [0] golang/go#22735
Support for sparse files does not exist in the implementation of golang tar. This is sad and tracked in [0]. We need this so we should look at supporting it again. This commit implements a naive version of hole skipping by just seeking over the "\0" parts of the file if the type of the file is `tar.TypeGNUSparse`. This copies parts of the io.go::copyBuffer() code (unfortunately). [0] golang/go#22735
This comment was marked as outdated.
This comment was marked as outdated.
|
Actually thinking about this, maybe we could even make it easier. It seems we have the ingredients on Linux already in the tar module, maybe we could just export the |
Hi @dsnet. Thanks for all the Go 1.10 archive/tar work. It's really an amazing amount of cleanup, and it's very well done.
The one change I'm uncomfortable with from an API point of view is the sparse hole support.
First, I worry that it's too complex to use. I get lost trying to read Example_sparseAutomatic - 99% of it seems to have nothing to do with sparse files - and I have a hard time believing that we expect clients to write all this code. Despite the name, nothing about the example strikes me as “automatic.”
Second, I worry that much of the functionality here does not belong in archive/tar. Tar files are not the only time that a client might care about where the holes are in a file or about creating a new file with holes, and yet somehow this functionality is expressed in terms of tar.Header and a new tar.SparseHole structure instead of tar-independent operations. Tar should especially not be importing and using such subtle bits of syscall as it is in sparse_windows.go.
It's too late to redesign this for Go 1.10, so I suggest we pull out this new API and revisit for Go 1.11.
For Go 1.11, I would suggest to investigate (1) what an appropriate API in package os would be, and (2) how to make archive/tar take advantage of that more automatically.
For example, perhaps it would make sense for package os to add
That would avoid archive/tar's current DetectParseHoles and SparseEntry, and the tar.Header only need to add a new field
Regions []int64. (Regions is not a great name; better names are welcome.) Note that using a simple slice of offsets avoids the need for a special invertSparseEntries function entirely: you just change whether you read pairs starting at offset 0 or 1.As for "punching holes", it suffices on Unix (as you know) to simply truncate the file (which Create does anyway) and then not write to the holes. On Windows it appears to be necessary to set the file type to sparse, but I don't see why the rest of sparsePunchWindows is needed. It seems crazy to me that it could possibly be necessary to pre-declare every hole location in a fresh file. The FSCTL_SET_ZERO_DATA looks like it is for making a hole in an existing file, not a new file. It seems like it should suffice to truncate the target file, mark it as sparse, set the file size, and then write the data. What's left should be automatically inferred as holes. If we were to add a new method SetSparse(bool) to os.File, then I would expect it to work on all systems to do something like:
Finally, it seems like handling this should not be the responsibility of every client of archive/tar. It seems like it would be better for this to just work automatically.
On the tar.Reader side, WriteTo already takes care of not writing to holes. It could also call SetSparse and use Truncate if present as an alternative to writing the last byte of the file.
On the tar.Writer side, I think ReadFrom could also take care of this. It would require making WriteHeader compute the header to be written to the file but delay the actual writing until the Write or ReadFrom call. (And that in turn might make Flush worth keeping around not-deprecated.) Then when ReadFrom is called to read from a file with holes, it could find the holes and add that information to the header before writing out the header. Both of those combined would make this actually automatic.
At the very least, it seems clear that the current API steps beyond what tar should be responsible for. I can easily see developers who need to deal with sparse files but have no need for tar files constructing fake tar headers just to use DetectSparseHoles and PunchSparseHoles. That's a strong signal that this functionality does not belong in tar as the primary implementation. (A weaker but still important signal is that to date the tar.Header fields and methods have not mentioned os.File explicitly, and it should probably stay that way.)
Let's remove this from Go 1.10 and revisit in Go 1.11. Concretely, let's remove tar.SparseEntry, tar.Header.SparseHoles, tar.Header.DetectSparseHoles, tar.Header.PunchSparseHoles, and the deprecation notice for tar.Writer.Flush.
Thanks again.
Russ
The text was updated successfully, but these errors were encountered: