Page MenuHomeFreeBSD

Add new "trim" conversion for dd(1)
Needs ReviewPublic

Authored by sobomax on Thu, Nov 29, 6:59 PM.

Details

Reviewers
eugen_grosbein.net
imp
sbruno
Group Reviewers
manpages
Summary

This patch implements new erase modifier for the conv=xx option, which acts similarly to "sparse", but uses ioctl(DIOCGDELETE) to erase the block in question rather than seeking over it.

Test Plan

Make memory disk, fill with random data, run dd if=/dev/zero conv=erase on it.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 21284

Event Timeline

sobomax created this revision.Thu, Nov 29, 6:59 PM
sobomax edited the summary of this revision. (Show Details)Thu, Nov 29, 7:01 PM
imp added a comment.Thu, Nov 29, 7:26 PM

I like this, independent of a standalone utility.

sbruno accepted this revision.Fri, Nov 30, 2:35 AM
This revision is now accepted and ready to land.Fri, Nov 30, 2:35 AM
danfe added a subscriber: danfe.Fri, Nov 30, 3:07 AM
danfe added inline comments.
bin/dd/dd.1
313

Typo.

bin/dd/dd.c
480

Why explicit cast to off_t is needed for zero?

537

Here sparse is checked as boolean (no != 0)...

559

... yet here, it is checked correctly (as int). Also, since it is ||, the order does not matter, and sparse can also be checked before ddflags like in the expression above?

trasz added a subscriber: trasz.Fri, Nov 30, 11:14 AM
trasz added inline comments.
bin/dd/dd.1
316

Would it be possible to somehow add the words "TRIM" and "UNMAP" to that sentence, to make it easier to find?

imp accepted this revision.Fri, Nov 30, 2:26 PM

My acceptance is provisional here.

If I can do

zcat image.gz | dd of=/dev/foo conv=sparse,erase

and have the data bits there, and the blocks of 0's erased, then I think this offers a cool-enough new feature to warrant putting it into dd and ditching a simpler, standalone utility.

sobomax updated this revision to Diff 51443.Fri, Nov 30, 5:44 PM

o Due to popular request rename "erase" into "trim".

o Fix typo in manpage.

o unify comparison (style).

This revision now requires review to proceed.Fri, Nov 30, 5:44 PM
sobomax marked 5 inline comments as done.Fri, Nov 30, 5:47 PM
sobomax added inline comments.
bin/dd/dd.c
480

ENOIDEA. :) I just copied code from some other place in the dd.c. I can only guess it's perhaps used to workaround some early cc(1) idiosyncrasy.

sobomax retitled this revision from Add new "erase" conversion for dd(1) to Add new "trim" conversion for dd(1).Fri, Nov 30, 5:54 PM
imp added a comment.Sat, Dec 1, 9:58 PM

There's a suggestion that the new trim program be named 'discard' to match linux's discard option on mount. Perhaps discard would be a better name. Though I hesitate to make such a bike-sheddy suggestion.

bin/dd/dd.c
480

It's because this code is old. K&R required you cast all long args. off_t is a long arg, so it has to be cast. It hasn't been required since we've had a ANSI compiler in the tree (so ~30 years, eg 4.4BSD could have ditched it).
V7 unix, which was pure K&R, had a similar cast to its calls to lseek().

danfe added a comment.Mon, Dec 3, 12:57 AM
In D18382#391628, @imp wrote:

There's a suggestion that the new trim program be named 'discard' to match linux's discard option on mount. Perhaps discard would be a better name.

Well I think it's a rather bad name and while both trim and erase are pretty common in flash media contexts, to an extent being self-explanatory, I'd very likely ignore discard when quickly glancing through the manpage.

Can we please not copy things and ideas coming from Linux so eagerly? They are often dubious and questionable (at best).