Bug 207092 - dd conv=sparse is broken since r265593
Summary: dd conv=sparse is broken since r265593
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 10.2-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: Thomas Quinot
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-10 21:25 UTC by Maxim Sobolev
Modified: 2018-12-04 16:37 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Maxim Sobolev freebsd_committer 2016-02-10 21:25:49 UTC
The dd(1) sparse conversion is seriously broken after this change. The last block is never written at least for the case of output being regular file:

$ dd if=/dev/zero of=/tmp/foo.bar bs=1m count=10
10+0 records in
10+0 records out
10485760 bytes transferred in 0.003244 secs (3232431656 bytes/sec)
$ ktrace dd if=/tmp/foo.bar of=/tmp/foo.bar1 bs=1m conv=sparse
10+0 records in
10+0 records out
$ ls -l /tmp/foo.bar /tmp/foo.bar1
-rw-r--r--  1 sobomax  wheel  10485760 Feb  9 23:59 /tmp/foo.bar
-rw-r--r--  1 sobomax  wheel         0 Feb  9 23:59 /tmp/foo.bar1
$ uname -a
FreeBSD abc.sippysoft.com 10.3-PRERELEASE FreeBSD 10.3-PRERELEASE #1 80de3e2(master)-dirty: Tue Feb  2 12:19:57 PST 2016     sobomax@abc.sippysoft.com:/usr/obj/usr/home/sobomax/projects/freebsd103/sys/VAN01  amd64

ktrace ends with:

  3150 dd       RET   read 1048576/0x100000
  3150 dd       CALL  read(0x3,0x801009000,0x100000)
  3150 dd       GIO   fd 3 read 0 bytes
       ""
  3150 dd       RET   read 0
  3150 dd       CALL  lseek(0x4,0x900000,SEEK_CUR)
  3150 dd       RET   lseek 9437184/0x900000
  3150 dd       CALL  close(0x4)
  3150 dd       RET   close 0
  3150 dd       CALL  write(0x2,0x7fffffffe2c0,0x21)
  3150 dd       GIO   fd 2 wrote 33 bytes
       "10+0 records in
        10+0 records out
       "
  3150 dd       RET   write 33/0x21
  3150 dd       CALL  write(0x2,0x7fffffffe2c0,0x43)
  3150 dd       GIO   fd 2 wrote 67 bytes
       "10485760 bytes transferred in 0.008217 secs (1276090675 bytes/sec)
       "
  3150 dd       RET   write 67/0x43
  3150 dd       CALL  sigprocmask(SIG_BLOCK,0x800822a38,0x7fffffffe780)
  3150 dd       RET   sigprocmask 0
  3150 dd       CALL  sigprocmask(SIG_SETMASK,0x800822a4c,0)
  3150 dd       RET   sigprocmask 0
  3150 dd       CALL  sigprocmask(SIG_BLOCK,0x800822a38,0x7fffffffe310)
  3150 dd       RET   sigprocmask 0
  3150 dd       CALL  sigprocmask(SIG_SETMASK,0x800822a4c,0)
  3150 dd       RET   sigprocmask 0
  3150 dd       CALL  sigprocmask(SIG_BLOCK,0x800822a38,0x7fffffffe310)
  3150 dd       RET   sigprocmask 0
  3150 dd       CALL  sigprocmask(SIG_SETMASK,0x800822a4c,0)
  3150 dd       RET   sigprocmask 0
  3150 dd       CALL  exit(0)


Looking at the code in question I don't see how could it have worked. Look at the following piece of code in your diff for example:

+                                       if (force && cnt == 0) {
+                                               pending -= last_sp;
+                                               assert(outp == out.db);
+                                               memset(outp, 0, cnt);
+                                       }

When the branch is taken, cnt is 0, so at the very least memset(x, y, 0) is NOP.  Later on, write(2) is conditional on cnt != 0, so that it's never taken. As a result, lseek is the last operation the file sees.

Also, for what it's worth, you can use ftruncate(2) instead of write() for regular sparse files to ensure correct size. That would write just as much data as needed to the end. I've made a quick and dirty patch, that seems to be working better than current code at least:

http://sobomax.sippysoft.com/dd.diff
Comment 1 Thomas Quinot freebsd_committer 2016-02-10 22:27:17 UTC
Maxim,
Thanks for your report.
A fix (slightly edited version of your patch) is available for review at https://reviews.freebsd.org/D5248. It also includes regression tests.
Could you please double check that it fixes the problem for you?
Thomas.
Comment 2 commit-hook freebsd_committer 2016-02-18 08:45:04 UTC
A commit references this bug:

Author: thomas
Date: Thu Feb 18 08:44:17 UTC 2016
New revision: 295749
URL: https://svnweb.freebsd.org/changeset/base/295749

Log:
  Reorganize the handling all-zeroes terminal block in sparse mode

  The intent of the previous code in that case was to force
  an explicit write, but the implementation was incorrect, and
  as a result the write was never performed. This new implementation
  instead uses ftruncate(2) to extend the file with a trailing hole.

  Also introduce regression tests for these cases.

  PR: 189284
  (original PR whose fix introduced this bug)

  PR: 207092

  Differential Revision:	D5248
  Reviewed by:	sobomax,kib
  MFC after:	2 weeks

Changes:
  head/bin/dd/Makefile
  head/bin/dd/dd.c
  head/bin/dd/dd.h
  head/bin/dd/gen.c
  head/bin/dd/ref.obs_zeroes
Comment 3 commit-hook freebsd_committer 2016-04-19 07:35:11 UTC
A commit references this bug:

Author: thomas
Date: Tue Apr 19 07:34:32 UTC 2016
New revision: 298258
URL: https://svnweb.freebsd.org/changeset/base/298258

Log:
  MFC r295749:

  Reorganize the handling all-zeroes terminal block in sparse mode

  PR: 189284
  (original PR whose fix introduced this bug)

  PR: 207092

Changes:
_U  stable/10/
  stable/10/bin/dd/Makefile
  stable/10/bin/dd/dd.c
  stable/10/bin/dd/dd.h
  stable/10/bin/dd/gen.c
  stable/10/bin/dd/ref.obs_zeroes