Date: Wed, 10 Feb 2016 00:12:52 -0800 From: Maxim Sobolev <sobomax@FreeBSD.org> To: Thomas Quinot <thomas@freebsd.org>, kib@freebsd.org Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn: head/bin/dd Message-ID: <CAH7qZfs=1vebh1jJVF2Gz1VViAaMNTip0p_oKq7Sb4yAHefwJw@mail.gmail.com> In-Reply-To: <201405071933.s47JXUx0046697@svn.freebsd.org> References: <201405071933.s47JXUx0046697@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Something is seriously broken about 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
Please fix.
On Wed, May 7, 2014 at 12:33 PM, Thomas Quinot <thomas@freebsd.org> wrote:
> Author: thomas
> Date: Wed May 7 19:33:29 2014
> New Revision: 265593
> URL: http://svnweb.freebsd.org/changeset/base/265593
>
> Log:
> (dd_out): Fix handling of all-zeroes block at end of input with
> conv=sparse.
>
> This change fixes two separate issues observed when the last output
> block is all zeroes, and conv=sparse is in use. In this case, care
> must be taken to roll back the last seek and write the entire last zero
> block at the original offset where it should have occurred: when the
> destination file is a block device, it is not possible to roll back
> by just one character as the write would then not be properly aligned.
>
> Furthermore, the buffer used to write this last all-zeroes block
> needs to be properly zeroed-out. This was not the case previously,
> resulting in a junk data byte appearing instead of a zero in the
> output stream.
>
> PR: bin/189174
> PR: bin/189284
> Reviewed by: kib
> MFC after: 2 weeks
>
> Modified:
> head/bin/dd/dd.c
>
> Modified: head/bin/dd/dd.c
>
> ==============================================================================
> --- head/bin/dd/dd.c Wed May 7 19:30:28 2014 (r265592)
> +++ head/bin/dd/dd.c Wed May 7 19:33:29 2014 (r265593)
> @@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$");
> #include <sys/disklabel.h>
> #include <sys/filio.h>
>
> +#include <assert.h>
> #include <ctype.h>
> #include <err.h>
> #include <errno.h>
> @@ -77,6 +78,7 @@ STAT st; /* statistics */
> void (*cfunc)(void); /* conversion function */
> uintmax_t cpy_cnt; /* # of blocks to copy */
> static off_t pending = 0; /* pending seek if sparse */
> +static off_t last_sp = 0; /* size of last added sparse block */
> u_int ddflags = 0; /* conversion options */
> size_t cbsz; /* conversion block size */
> uintmax_t files_cnt = 1; /* # of files to copy */
> @@ -174,6 +176,8 @@ setup(void)
> } else if ((in.db = malloc(MAX(in.dbsz, cbsz) + cbsz)) == NULL ||
> (out.db = malloc(out.dbsz + cbsz)) == NULL)
> err(1, "output buffer");
> +
> + /* dbp is the first free position in each buffer. */
> in.dbp = in.db;
> out.dbp = out.db;
>
> @@ -436,8 +440,15 @@ dd_out(int force)
> * we play games with the buffer size, and it's usually a partial
> write.
> */
> outp = out.db;
> +
> + /*
> + * If force, first try to write all pending data, else try to write
> + * just one block. Subsequently always write data one full block at
> + * a time at most.
> + */
> for (n = force ? out.dbcnt : out.dbsz;; n = out.dbsz) {
> - for (cnt = n;; cnt -= nw) {
> + cnt = n;
> + do {
> sparse = 0;
> if (ddflags & C_SPARSE) {
> sparse = 1; /* Is buffer sparse? */
> @@ -449,18 +460,24 @@ dd_out(int force)
> }
> if (sparse && !force) {
> pending += cnt;
> + last_sp = cnt;
> nw = cnt;
> } else {
> if (pending != 0) {
> - if (force)
> - pending--;
> + /* If forced to write, and we have
> no
> + * data left, we need to write the
> last
> + * sparse block explicitly.
> + */
> + if (force && cnt == 0) {
> + pending -= last_sp;
> + assert(outp == out.db);
> + memset(outp, 0, cnt);
> + }
> if (lseek(out.fd, pending,
> SEEK_CUR) ==
> -1)
> err(2, "%s: seek error
> creating sparse file",
> out.name);
> - if (force)
> - write(out.fd, outp, 1);
> - pending = 0;
> + pending = last_sp = 0;
> }
> if (cnt)
> nw = write(out.fd, outp, cnt);
> @@ -475,27 +492,29 @@ dd_out(int force)
> err(1, "%s", out.name);
> nw = 0;
> }
> +
> outp += nw;
> st.bytes += nw;
> - if ((size_t)nw == n) {
> - if (n != out.dbsz)
> - ++st.out_part;
> - else
> - ++st.out_full;
> - break;
> - }
> - ++st.out_part;
> - if ((size_t)nw == cnt)
> - break;
> - if (out.flags & ISTAPE)
> - errx(1, "%s: short write on tape device",
> - out.name);
> - if (out.flags & ISCHR && !warned) {
> - warned = 1;
> - warnx("%s: short write on character
> device",
> - out.name);
> +
> + if ((size_t)nw == n && n == out.dbsz)
> + ++st.out_full;
> + else
> + ++st.out_part;
> +
> + if ((size_t) nw != cnt) {
> + if (out.flags & ISTAPE)
> + errx(1, "%s: short write on tape
> device",
> + out.name);
> + if (out.flags & ISCHR && !warned) {
> + warned = 1;
> + warnx("%s: short write on
> character device",
> + out.name);
> + }
> }
> - }
> +
> + cnt -= nw;
> + } while (cnt != 0);
> +
> if ((out.dbcnt -= n) < out.dbsz)
> break;
> }
>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAH7qZfs=1vebh1jJVF2Gz1VViAaMNTip0p_oKq7Sb4yAHefwJw>