Bug 229616
| Summary: | bin/dd: add conv=fsync, handle errors in close(2) | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Thomas Hurst <tom> | ||||||||
| Component: | bin | Assignee: | Matt Macy <mmacy> | ||||||||
| Status: | Closed FIXED | ||||||||||
| Severity: | Affects Many People | CC: | brooks, emaste, freqlabs, imp, kevans, swills | ||||||||
| Priority: | Normal | Keywords: | easy | ||||||||
| Version: | CURRENT | Flags: | koobs:
mfc-stable12?
koobs: mfc-stable11? |
||||||||
| Hardware: | Any | ||||||||||
| OS: | Any | ||||||||||
| Attachments: |
|
||||||||||
Given the potential for data loss, and the relative, apparently, simplicity of addressing it such as checking for close(2) succeeds or not, mark this as importance:high CC original committer of code block (base r249063) who may know the best course of action, or someone who might. Created attachment 204530 [details]
Patch against head/bin/dd/dd.c
So, did a bit of research to see what everyone else is doing. NetBSD: * Checks close() on output * fsync() on output if it's stdout (where close() may simply decrement a refcount) OpenBSD: * Does not close() at all * Supports conv=fsync Illumos: * Checks close() on output, and also fclose(stdout) * Supports conv=fsync and fdatasync * Supports oflag=sync and dsync for synchronous writes Coreutils: * Checks close() on both input and output * Checks ferror()/fclose() on stdout if arg parsing fails * Supports conv=fsync and fdatasync * Supports oflag=sync, dsync, and direct * EINTR-checking loops for close() and f*sync() So FreeBSD's bit of an outlier, being the only dd without any fsync support at all, and calling close() explicitly but not checking it succeeds. I'm thinking we could do a hybrid of NetBSD and OpenBSD's approaches: 1. Check close() 2. Also check fsync() if output is stdout 3. Add conv=fsync support Other options might be nice to have, but aren't needed to ensure data is on stable storage after dd is finished, which is the most important point. Created attachment 206684 [details]
Patch against head/bin/dd
This adds conv=fsync, implicit fsync when output is stdout, and checking of close(2) return code.
Created attachment 206685 [details]
Patch against releng/12/bin/dd
CC imp@ for dd(1) patch I agree with checking close() and conv=fsync/fdatasync but not implicit fsync(). Implicit fsync might cause unexpected poor performance. For conv=fsync see review https://reviews.freebsd.org/D21370 This patch looks better than the code review posted. A commit references this bug: Author: mmacy Date: Mon Sep 30 21:53:26 UTC 2019 New revision: 352921 URL: https://svnweb.freebsd.org/changeset/base/352921 Log: dd: Check result of close(2) for errors close(2) can return errors from previous operations which should not be ignored. PR: 229616 Submitted by: Thomas Hurst Reported by: Thomas Hurst Reviewed by: mmacy@ Obtained from: Ryan Moeller MFC after: 1 week Differential Revision: https://reviews.freebsd.org/D21376 Changes: head/bin/dd/dd.c If this (or any other issue, eg: the other recent dd changes) should be merged to releng/12.1, please add bug 240700 to this issues Blocks field Merges should also include this PR: <ID> in the commit log messages for tracking purposes |
In dd's main(): dd_close(); /* * Some devices such as cfi(4) may perform significant amounts * of work when a write descriptor is closed. Close the out * descriptor explicitly so that the summary handler (called * from an atexit() hook) includes this work. */ close(out.fd); exit(0); dd_close() comments *claim* it flushes output, but this appears to be a lie - it just finishes off any pending calls to write(), it doesn't ask they be flushed to disk. IO errors from previous writes that were later flushed to disk can end up reported in close(), for example as mentioned in close(2): [ENOSPC] The underlying object did not fit, cached data was lost Obviously a dd that can exit 0 while also losing data is.. unfortunate.