Page MenuHomeFreeBSD

truncate(1): Add hole-punching support
ClosedPublic

Authored by khng on Aug 16 2021, 1:19 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 8, 1:44 AM
Unknown Object (File)
Sat, Apr 27, 2:11 AM
Unknown Object (File)
Thu, Apr 25, 10:15 PM
Unknown Object (File)
Tue, Apr 23, 7:45 AM
Unknown Object (File)
Tue, Apr 23, 2:31 AM
Unknown Object (File)
Mon, Apr 22, 12:27 AM
Unknown Object (File)
Mon, Apr 22, 12:27 AM
Unknown Object (File)
Mon, Apr 22, 12:27 AM

Details

Summary

This commit adds hole-punching support to the truncate(1) utility. If
the option -d is specified, truncate(1) performs zeroing. In case the
operation is supported by the underlying file systems of the specified files,
hole-punching is performed instead of zeroing.

Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

khng requested review of this revision.Aug 16 2021, 1:19 PM

Remove MACHINE-specific from Makefile.depend

Did you considered adding some more options to truncate(1) instead?

usr.bin/fspacectl/fspacectl.c
64 ↗(On Diff #93747)

The body of the loop clearly deserves {} around it

92 ↗(On Diff #93747)

!do_dealloc

106–107 ↗(On Diff #93747)

return ()

In D31556#711650, @kib wrote:

Did you considered adding some more options to truncate(1) instead?

This sounds fine to me.

In D31556#711650, @kib wrote:

Did you considered adding some more options to truncate(1) instead?

Or trim(8), which already performs a similar operation on device files.

khng marked 3 inline comments as done.

Move the functionality to truncate(1) instead.

khng retitled this revision from Add fspacectl(1), a utility to perform file space management operations to truncate(1): Add hole-punching support.Aug 18 2021, 5:46 PM
khng edited the summary of this revision. (Show Details)
In D31556#711650, @kib wrote:

Did you considered adding some more options to truncate(1) instead?

Or trim(8), which already performs a similar operation on device files.

truncate is a better place for it, imho, though Linux's man pages make no mention of this. It also has no trim command. all imho.

dd also supports the BIO_DELETE operations. I've not checked to see if gnu's coreutil's dd can operate on files or not. But it seems a bit wrong to add it there.

The truncate implementation looks good. I have but one minor nit beyond noting history here.

usr.bin/truncate/truncate.1
8

Nit: No need for this 'blank line' between the copyright notices.

usr.bin/truncate/truncate.1
37
46

Might be remove list of suffixes from all sizes, just typeset it as a meta-symbol.
Then note somewhere that that size is a number with following accepted suffixes.

I hope you see what I mean.

usr.bin/truncate/truncate.c
72

Any reason to not include do_dealloc into next line?

135

Where is this enforced?

usr.bin/truncate/truncate.1
183

Isn't it useful to be able to omit the length and create a hole from the specified offset until EOF?

khng marked 4 inline comments as done.

Fix copyright notice, styles. The argument checking is rearranged to make it more obvious. Remove duplication of size/offset/length suffix documentation and group them together.

usr.bin/truncate/truncate.1
183

I thought it could be easily misused to accidentally turn a file into a completely sparse when you forgot to type the preceding dash for instance.

usr.bin/truncate/truncate.c
135

Around line 145, but I reordered it to be in the same line below this comment block.

This revision is now accepted and ready to land.Aug 18 2021, 7:25 PM
  • Disallow users calling with length == 0.
  • Resume from signal interruption.
This revision now requires review to proceed.Aug 18 2021, 8:48 PM
usr.bin/truncate/truncate.c
218

Why? A signal most likely comes from user trying to stop the operation. Instead, the utility would try to restart.

In fact I do not think that EINTR is possible, because you do not install any signal handlers, but still I believe that this is simply wrong to do.

khng marked an inline comment as done.

Restore the old one.

usr.bin/truncate/truncate.c
216

The ';' should be on separate line.

220

I do not understand this at all.

I don't even think the loop for fspacectl call is required.

usr.bin/truncate/truncate.c
221

I think it would be cleaner if you added 'do_truncate' control var, set to 1 by default. This 'else' is quite confusing, I must admit.

kib added inline comments.
usr.bin/truncate/truncate.c
207

You do not need 'else'

This revision is now accepted and ready to land.Aug 18 2021, 11:15 PM
This revision was automatically updated to reflect the committed changes.

I believe this commit has broken the cross-builds on GitHub.

usr.bin/truncate/truncate.c
206

At a quick glance, it looks like SPACECTL_DEALLOC is only defined if __BSD_VISIBLE, hence throwing:

/Users/runner/work/freebsd-src/freebsd-src/usr.bin/truncate/truncate.c:206:22: error: use of undeclared identifier 'SPACECTL_DEALLOC'
                        r = fspacectl(fd, SPACECTL_DEALLOC, &sr, 0, &sr);

I believe this commit has broken the cross-builds on GitHub.

A basic background for this: usr.bin/truncate is currently a listed target in _basic_bootstrap_tools.