Sync output to disk before removing source #151

sebastianas · 2024-11-17T16:01:40Z

Synchronize created output to disk before removing original input. This lowers the risk to loose source and destination if a crash happens shortly after.

Add a function which returns the directory part of the filename. This is either the name up the last deliminator like '/' or simple '.' if it is current directory. The function reuses the inner parts of has_dir_sep() for the deliminator. The inner parts are moved to get_dir_sep() in order to the pointer while has_dir_sep() still returns the bool. Signed-off-by: Sebastian Andrzej Siewior <[email protected]>

If the file is compressed (or decompressed) then the original file is deleted by default after the written file is closed. All this is fine. If the machine crashes then it is possible that the newly written file has not yet hit the disk while the previous delete operation might have. In that case neither the original file nor the written file is available. To avoid this scenario, sync the file, followed the directory (to ensure that the file is part of the directory). This may cause the `xz' command to take a bit longer because it now waits additionally until the data has been written to disk. Signed-off-by: Sebastian Andrzej Siewior <[email protected]>

Larhzu · 2024-11-25T19:12:47Z

The Debian bug 814089 has an example why this can matter. Sebastian and I have discussed this both recently and in 2016. There are three related xz-devel mailing list messages from 2016 as well.

I tested fsync at some point before XZ Utils 5.0.0, likely in 2009. Back then it was on rotating media. When compressing a large number of tiny files, the performance was horrible. Obviously no one cares about performance if it leads to data loss. However, some use cases are on files which can be easily re-created, and then the lack of fsync doesn't matter but performance does.

gzip 1.7 (2016) added the --synchronous which enables fsync usage. This way users can get the safer behavior when they need it. However, it might be that users don't learn about the option before an accident has already happened.

Adding gzip's --synchronous would be a conservative change in sense that it wouldn't make current use cases slower. Opinions are welcome. Once we know what to do, we can focus on making the code do that. The current PR is fine for testing the performance impact on GNU/Linux.

xz's default behavior is to delete the input file after successful compression or decompression (unless writing to standard output). If the system crashes soon after the deletion, it is possible that the newly written file has not yet hit the disk while the previous delete operation might have. In that case neither the original file nor the written file is available. The --synchronous option makes xz call fsync() on the file and possibly the directory where the file was created. A similar option was added to GNU gzip 1.7 in 2016. There some differences in behavior: - When writing to standard output and processing multiple input files, xz calls fsync() after every file while gzip does so only after all files have been processed. - This has no effect on "xz --list". xz doesn't sync standard output in --list mode but gzip does. Portability notes: - <libgen.h> and dirname() should be available on all POSIX systems, and aren't needed on non-POSIX systems. - fsync() is available on all POSIX systems. The directory syncing could be changed to fdatasync() although at least on ext4 it doesn't seem to make a performance difference in xz's usage. fdatasync() would need a build system check to support (old) special cases, for example, MINIX 3.3.0 doesn't have fdatasync() and Solaris 10 needs -lrt. - On native Windows, _commit() is used to replace fsync(). Directory syncing isn't done and shouldn't be needed. (In Cygwin, fsync() on directories is a no-op.) It is known that syncing will fail if writing to stdout and stdout isn't a regular file. - DJGPP has fsync() for files. ;-) Co-authored-by: Sebastian Andrzej Siewior <[email protected]> Link: https://bugs.debian.org/814089 Link: https://www.mail-archive.com/[email protected]/msg00282.html Closes: #151 Closes: #159

Larhzu · 2024-12-27T08:23:16Z

@sebastianas, I don't expect anyone to comment this here. I feel the number of complaints will be smaller if xz does what gzip did, that is, add --synchronous. It's possible to add it in XZ_DEFAULTS if one wants it always enabled.

I created #159 to add --synchronous. It would be nice if you could check if it seems OK. I'm aware that a few choices I made are against the advice I gave you via email.

sebastianas · 2025-01-03T22:40:23Z

On 2024-11-25 11:13:08 [-0800], Lasse Collin wrote: The [Debian bug 814089](https://bugs.debian.org/814089) has an example why this can matter. Sebastian and I have discussed this both recently and in 2016. There are three related [xz-devel mailing list messages from ***@***.***/msg00282.html) as well. I tested `fsync` at some point before XZ Utils 5.0.0, likely in 2009. Back then it was on rotating media. When compressing a large number of tiny files, the performance was horrible. Obviously no one cares about performance if it leads to data loss. However, some use cases are on files which can be easily re-created, and then the lack of `fsync` doesn't matter but performance does.

grml.

gzip 1.7 (2016) added the `--synchronous` which enables `fsync` usage. This way users can get the safer behavior when they need it. However, it might be that users don't learn about the option before an accident has already happened.

Yes. Also gzip is single threaded while xz happily takes all available cores if the file is big enough.

Adding gzip's `--synchronous` would be a conservative change in sense that it wouldn't make current use cases slower. Opinions are welcome. Once we know what to do, we can focus on making the code do that. The current PR is fine for testing the performance impact on GNU/Linux.

What about we make the `fsync` part default and if people indeed complain about performance we can either hide it behind `--synchronous` like gzip does or add an option to avoid `fsync` and let them stick it to `XZ_DEFAULT` (or so) or do nothing encourage the usage of `eatamydata`. Given the incident and the default removal I kind of feel to better safe (and slower) by default than sorry. Sebastian

Larhzu · 2025-01-04T08:51:46Z

Thanks for the comments! Part of me feels like you do, that is, the default behavior should be safe. So far I got one comment on IRC and that agreed with you. :-)

The critical case is using fsync before deleting the source file. If this is the default behavior and opening the containing directory fails, I wonder if the error from open should be ignored. A directory with write and search permission but no read permission is unusual but it's still a bit odd to fail (de)compression if the directory cannot be openend. This is only a problem because Linux lacks O_SEARCH that POSIX defines. Maybe such directories are unusual enough that it's OK to make xz fail when syncing is enabled. I know I said in an email that ignoring the error could make sense but I might be changing my mind to make this feature more robust.

It's also a question if --flush-timeout should sync by default. The feature was requested for some kind of log files where one wanted to be able to decompress everything received so far with a short delay. It depends on the specific use case if syncing after every flush is desired or not. It's such a special option that it perhaps is enough to document that one can enable syncing separately for it. Typically it's used when writing to stdout and then one needs to create the file and sync the directory before starting xz (like explained in the man page addition in my PR).

So, use fsync by default if not writing to stdout and --keep wasn't used (or implied). This is what your patch does. :-) An option is needed to disable the behavior because there are use cases where one compresses lots of small files which are easy to recreate so fsync isn't needed.

Should there be an option to enable fsync even when input file isn't deleted? If --flush-timeout doesn't sync by default, that alone sounds like a reason to add such an option. But does it make sense in any other situation?

If xz is writing to stdout which is a regular file, user can sync it after xz has finished. The --flush-timeout option is the only exception to this.
With xz --keep foo one can also sync after xz has finished.

Am I missing something? If not, perhaps --flush-timeout should sync by default. Then all we need is --no-sync instead of --synchronous=<auto|always|never>. I may try to update my code to something like that.

Getting feedback about this kind of topics is difficult so I appreciate all comments. Thanks!

xz's default behavior is to delete the input file after successful compression or decompression (unless writing to standard output). If the system crashes soon after the deletion, it is possible that the newly written file has not yet hit the disk while the previous delete operation might have. In that case neither the original file nor the written file is available. Call fsync() on the file. On POSIX systems, sync also the directory where the file was created. Add a new option --no-sync which disables fsync() usage. It can avoid a (possibly significant) performance penalty when processing many small files. It's fine to use --no-sync when one knows that the files are easy to recreate or restore after a system crash. Using fsync() after every flush initiated by --flush-timeout was considered. It wasn't implemented at least for now. - --flush-timeout is typically used when writing to stdout. If stdout is a file, xz cannot (portably) sync the directory of the file. One would need to create the output file first, sync the directory, and then run xz with fsync() enabled. - If xz --flush-timeout output goes to a file, it's possible to use a separate script to sync the file, for example, once per minute while telling xz to flush more frequently. - Not supporting syncing with --flush-timeout was simpler. Portability notes: - On systems that lack O_SEARCH (like Linux), "xz dir/file" will now fail if "dir" cannot be opened for reading. If "dir" still has write and search permissions (like d-wx------ in "ls -l"), previously xz would have been able to compress "dir/file" still. Now it only works if using --no-sync (or --keep or --stdout). - <libgen.h> and dirname() should be available on all POSIX systems, and aren't needed on non-POSIX systems. - fsync() is available on all POSIX systems. The directory syncing could be changed to fdatasync() although at least on ext4 it doesn't seem to make a performance difference in xz's usage. fdatasync() would need a build system check to support (old) special cases, for example, MINIX 3.3.0 doesn't have fdatasync() and Solaris 10 needs -lrt. - On native Windows, _commit() is used to replace fsync(). Directory syncing isn't done and shouldn't be needed. (In Cygwin, fsync() on directories is a no-op.) - DJGPP has fsync() for files. ;-) Using fsync() was considered somewhere around 2009 and again in 2016 but those times the idea was rejected. For comparison, GNU gzip 1.7 (2016) added the option --synchronous which enables fsync(). Co-authored-by: Sebastian Andrzej Siewior <[email protected]> Fixes: https://bugs.debian.org/814089 Link: https://www.mail-archive.com/[email protected]/msg00282.html Closes: #151

Larhzu · 2025-01-04T15:43:44Z

I created a new branch synchronous2. I tried adding syncing to --flush-timeout but the conditions become ugly and I'm not even sure if that use case needs syncing. So I will skip that for now and focus on sorting out the main issue.

Larhzu · 2025-01-04T15:46:27Z

I know that the position of the filename in the error messages might be archaic but if the order is changed, then all messages should be revised for consistency. I suppose it's not worth doing.

sebastianas · 2025-01-04T20:22:35Z

On 2025-01-04 00:52:08 [-0800], Lasse Collin wrote: Thanks for the comments! Part of me feels like you do, that is, the default behavior should be safe. So far I got one comment on IRC and that agreed with you. :-)

;)

The critical case is using `fsync` before deleting the source file. If this is the default behavior and opening the containing directory fails, I wonder if the error from `open` should be ignored. A directory with write and search permission but no read permission is unusual but it's still a bit odd to fail (de)compression if the directory cannot be openend. This is only a problem because Linux lacks `O_SEARCH` that POSIX defines. Maybe such directories are unusual enough that it's OK to make xz fail when syncing is enabled. I know I said in an email that ignoring the error could make sense but I might be changing my mind to make this feature more robust.

okay, sounds sane to fail if the open() and the following sync fails.

It's also a question if `--flush-timeout` should sync by default. The feature was requested for some kind of log files where one wanted to be able to decompress everything received so far with a short delay. It depends on the specific use case if syncing after every flush is desired or not. It's such a special option that it perhaps is enough to document that one can enable syncing separately for it. Typically it's used when writing to stdout and then one needs to create the file and sync the directory before starting xz (like explained in the man page addition in my PR).

I would keep it async and fsync it at the end/ while closing the file. The reason is that the filesystem will have the freedome to allocate extens once it feels like it/ writting larger blocks. A sync every X seconds could lead to more extends/ fragments. The flush at the end forces the write which the filesystem has to do anyway.

So, use `fsync` by default if not writing to stdout and `--keep` wasn't used (or implied). This is what your patch does. :-) An option is needed to disable the behavior because there are use cases where one compresses lots of small files which are easy to recreate so `fsync` isn't needed. Should there be an option to enable `fsync` even when input file isn't deleted? If `--flush-timeout` doesn't sync by default, that alone sounds like a reason to add such an option. But does it make sense in *any* other situation?

Well, the idea is to ensure that there is at least one valid copy in case of a crash. So if we don't delete the input file we could avoid the sync on the output. We could do it anyway to be consistent. I think doing it anyway wouldn't be bad.

* If xz is writing to stdout which is a regular file, user can sync it after xz has finished. The `--flush-timeout` option is the only exception to this. * With `xz --keep foo` one can also sync after xz has finished. Am I missing something? If not, perhaps `--flush-timeout` should sync by default. Then all we need is `--no-sync` instead of `--synchronous=<auto|always|never>`. I may try to update my code to something like that. Getting feedback about this kind of topics is difficult so I appreciate all comments. Thanks!

Thanks for considering it. Sebastian

Larhzu · 2025-01-05T08:50:00Z

I got another comment from IRC favoring syncing at least before deletion. :-)

> It's also a question if `--flush-timeout` should sync by default. I would keep it async and fsync it at the end/ while closing the file. The reason is that the filesystem will have the freedome to allocate extens once it feels like it/ writting larger blocks. A sync every X seconds could lead to more extends/ fragments. The flush at the end forces the write which the filesystem has to do anyway.

The --flush-timeout option is useful if the input comes slowly but one wants to be able to decompress all data with short enough delay. For example, some tool could be piping a log file to xz --flush-timeout. xz's output might be piped to another program, for example, to be sent over a slow network connection. Or the output might be directed to a local file, and if it is the only copy of the log, then calling fsync() now and then could be valuable. If flush timeout is a small value like 1-3 seconds to make data available to programs reading the file, syncing so often might indeed be too often. In this case one hacky solution could be to have a separate script that syncs the file, for example, once a minute. That is, if syncing is wanted to guard against data loss on crashes. So the need for intermediate syncing depends on why one is using --flush-timeout. The feature was added to fulfill a real use case but I suspect that there aren't many users of this option. Anyway, syncing *only* at the end of the file when using --flush-timeout isn't interesting because the "xz --flush-timeout" process might run for hours or days. If one needs only end of the file syncing, the script running xz could just use "sync filename" after xz has finished. And the directory of the file needs to be synced too.

Well, the idea is to ensure that there is at least one valid copy in case of a crash. So if we don't delete the input file we could avoid the sync on the output. We could do it anyway to be consistent. I think doing it anyway wouldn't be bad.

I don't have a strong opinion. When xz doesn't delete, it's kind of like "cp foo foo.xz" which doesn't sync. gzip --synchronous syncs also stdout (even for gzip --list). It's not possible to portably sync the directory of the file in this case though. (On Linux one could readlink("/proc/self/fd/NUMBER", ...) to figure out the directory.) I'm not sure if the consistency argument extends to stdout. With the --synchronous idea in the "synchronous" branch it did but I feel it was needed when syncing has been explicitly requested. Let's only sync before deletion at least for now because it's enough to fix the dangerous situation. This will be in 5.7.1alpha. It's just a development release so this can be changed to something else before 5.8.0 if we get such feedback.

> Getting feedback about this kind of topics is difficult so I > appreciate all comments. Thanks! Thanks for considering it.

Probably this should have been done in 2016 (or even in 2009) but perhaps I was too much against adding an option like --no-sync or too afraid that users would be unhappy about the performance without figuring out the reason and thus not finding the --no-sync option. I pushed a minor tweak to synchronous2. Can I merge it or is there something to tweak (including man page or comments or commit message)? Thanks!

…

-- Lasse Collin

sebastianas · 2025-01-05T17:09:47Z

On 2025-01-05 00:50:22 [-0800], Lasse Collin wrote: Let's only sync before deletion at least for now because it's enough to fix the dangerous situation. This will be in 5.7.1alpha. It's just a development release so this can be changed to something else before 5.8.0 if we get such feedback.

Okay.

> > Getting feedback about this kind of topics is difficult so I > > appreciate all comments. Thanks! > > Thanks for considering it. Probably this should have been done in 2016 (or even in 2009) but perhaps I was too much against adding an option like --no-sync or too afraid that users would be unhappy about the performance without figuring out the reason and thus not finding the --no-sync option.

I kind of also forgot all about it. I just remembered it while looking over the bug list.

I pushed a minor tweak to synchronous2. Can I merge it or is there something to tweak (including man page or comments or commit message)?

I've been looking over synchronous2 and feel free to merge it. I see you found dirname(), I've been looking for it with no success :/ Thanks.

Thanks!

Sebastian

Larhzu · 2025-01-05T19:11:28Z

I see you found dirname(), I've been looking for it with no success :/

I shortly mentioned in an email but I wasn't immediately sure if it was indeed fine to use, thus I didn't steer you towards it. Sorry. :-(

I've been looking over synchronous2 and feel free to merge it.

Thank you! Done. CI fails with musl. I hope to figure it out tomorrow.

Larhzu · 2025-01-06T09:33:45Z

Have you seen the discussion from 2016 how gzip got the --synchronous option instead of using fsync by default? In short, it was performance worries. https://debbugs.gnu.org/cgi/bugreport.cgi?bug=22768 gzip had default fsync in gzip.git for a few days: https://git.savannah.gnu.org/cgit/gzip.git/commit/?id=22aac8f8a616a72dbbe0e4119db8ddda0f076c04 https://git.savannah.gnu.org/cgit/gzip.git/commit/?id=5ef892a9248e02dac13840f0acefe0fe72605dfa https://git.savannah.gnu.org/cgit/gzip.git/commit/?id=e6516c537071ac28b3e8e12439fbaf54bfe4fbd0

sebastianas added 2 commits November 11, 2024 21:41

Larhzu closed this in 2a9e91d Jan 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync output to disk before removing source #151

Sync output to disk before removing source #151

sebastianas commented Nov 17, 2024

Larhzu commented Nov 25, 2024

Larhzu commented Dec 27, 2024

sebastianas commented Jan 3, 2025 via email

Larhzu commented Jan 4, 2025

Larhzu commented Jan 4, 2025

Larhzu commented Jan 4, 2025

sebastianas commented Jan 4, 2025 via email

Larhzu commented Jan 5, 2025 via email

sebastianas commented Jan 5, 2025 via email

Larhzu commented Jan 5, 2025 via email

Larhzu commented Jan 6, 2025 via email

Sync output to disk before removing source #151

Sync output to disk before removing source #151

Conversation

sebastianas commented Nov 17, 2024

Larhzu commented Nov 25, 2024

Larhzu commented Dec 27, 2024

sebastianas commented Jan 3, 2025 via email

Larhzu commented Jan 4, 2025

Larhzu commented Jan 4, 2025

Larhzu commented Jan 4, 2025

sebastianas commented Jan 4, 2025 via email

Larhzu commented Jan 5, 2025 via email

sebastianas commented Jan 5, 2025 via email

Larhzu commented Jan 5, 2025 via email

Larhzu commented Jan 6, 2025 via email