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)
Apr 27 2024, 2:11 AM
Unknown Object (File)
Apr 25 2024, 10:15 PM
Unknown Object (File)
Apr 23 2024, 7:45 AM
Unknown Object (File)
Apr 23 2024, 2:31 AM
Unknown Object (File)
Apr 22 2024, 12:27 AM
Unknown Object (File)
Apr 22 2024, 12:27 AM
Unknown Object (File)
Apr 22 2024, 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
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 41128
Build 38017: arc lint + arc unit

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
71

Any reason to not include do_dealloc into next line?

132

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
132

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
206

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
204

The ';' should be on separate line.

208

I do not understand this at all.

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

usr.bin/truncate/truncate.c
209

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
203

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
202

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.