Page MenuHomeFreeBSD

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

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

Details

Reviewers
eugen_grosbein.net
imp
sbruno
bcr
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 22010

Event Timeline

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

I like this, independent of a standalone utility.

sbruno accepted this revision.Nov 30 2018, 2:35 AM
This revision is now accepted and ready to land.Nov 30 2018, 2:35 AM
danfe added a subscriber: danfe.Nov 30 2018, 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.Nov 30 2018, 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.Nov 30 2018, 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.Nov 30 2018, 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.Nov 30 2018, 5:44 PM
sobomax marked 5 inline comments as done.Nov 30 2018, 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).Nov 30 2018, 5:54 PM
imp added a comment.Dec 1 2018, 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.Dec 3 2018, 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).

bcr accepted this revision.Dec 30 2018, 12:27 PM
bcr added a subscriber: bcr.

Manpages is OK with it.

This revision is now accepted and ready to land.Dec 30 2018, 12:27 PM
eugen_grosbein.net requested changes to this revision.Dec 30 2018, 6:47 PM

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

You need now fix clist[] array in the args.c as it must be sorted or else bsearch() fails to find "trim" and dd conv=trim bails out with error "unknown conversion trim".

This revision now requires changes to proceed.Dec 30 2018, 6:47 PM
jilles added a subscriber: jilles.Dec 31 2018, 9:40 PM

A new test in bin/dd/tests that does dd if=/dev/null of=/dev/null status=none conv=trim would be nice (by not writing/trimming any bytes, it does not need a fake disk setup, but it does verify that the option is recognized). Even better would be something using md but that would need more ATF/Kyua magic.

sobomax updated this revision to Diff 52950.Jan 17 2019, 9:50 PM

Keep clist[] array in the args.c sorted by the name so that bsearch() DTRT.

Reported by: eugen_grosbein.net

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

You need now fix clist[] array in the args.c as it must be sorted or else bsearch() fails to find "trim" and dd conv=trim bails out with error "unknown conversion trim".

@eugen_grosbein.net thanks, good catch. Should be fixed now. Also updated diff to match C_PROGRESS changes.

araujo added a subscriber: araujo.Jan 18 2019, 2:57 PM