Fix a number of style issues with trim(8)

[imp]

Summary

o use __FBSDID() macro
o we don't need both -q and -v. The default is -q on all other utilities,

so eliminate -q.

o we don't need both -N and -f. The default is -f on all other utilities,

so eliminate -f.

o we don't need to check to make sure it's a CHR or BLK device before

calling the disk ioctls, so eliminate that check.

o Improve error messages. Elimiante the '%s' construct for filenames,

because traditionally that's not done.

o abstract out opendev() to open the device by raw devname or by

short name in /dev.

o Update man pages with these changes.

[eugen_grosbein.net]

The operation in question is dangerous and can easily result in loss of data if used by unexperienced root. This is why I want it to NOT defaults to -f but instead defaults to verbose dry-run mode. Hence the need for -f and -q.

[eugen_grosbein.net]

The check for CHR and SBLK is to issue nice and correct error message in case of pilot error instead of generic and obscure error after ioctl() so I'd prefer to keep it.

[eugen_grosbein.net]

I'm fine with rest of changes.

[rpokala]

usr.sbin/trim/trim.8

54 ↗

(On Diff #51361)

Should probably specifically mention the requirement for DIOCGDELETE.

143 ↗

(On Diff #51361)

What about MMC?

[cem]

usr.sbin/trim/trim.8

54 ↗

(On Diff #51361)

Do we usually mention implementation ioctls in admin manual pages? I suspect not, but there may be exceptions (and this might be a good exception).

usr.sbin/trim/trim.c

48–49 ↗

(On Diff #51361)

I'd prefer we pass around an fd instead of a path. If we keep the paths, they should probably be const char *.

dryrun/verbose should be a single int flags; or at least bools in new programs.

50 ↗

(On Diff #51361)

This should be const char *

57 ↗

(On Diff #51361)

Prefer bool for booleans in new programs

67–70 ↗

(On Diff #51361)

Is dry run usually -N or -n? For some reason I thought it was the latter, although I'm not finding any evidence to support that.

130–184 ↗

(On Diff #51361)

Seems like these are basically reimplementations of libgeom g_mediasize() and g_delete().

Yeah, g_open() -> g_device_path_open() has that stupid g_sectorsize() check that intentionally fails on anything without DIOCGSECTORSIZE, and g_mediasize() doesn't attempt to simulate DIOCGMEDIASIZE using stat.

But it's not like there's anything special about g_opened fds. We could open the path once in this program and use a combination of fstat(), g_mediasize(), and g_delete() instead of reinventing the wheel.

[cem]

usr.sbin/trim/trim.c

130–184 ↗

(On Diff #51361)

E.g., https://reviews.freebsd.org/P239

[eugen_grosbein.net]

I strongly disagree against GEOM'ifying or CAM'ifyng such simple code.

First, DIOCGDELETE is not GEOM-specific ioctl() and we already have ZFS zvols that can be created as device nodes in /dev with non-default volmode=dev property instead of default volmode=geom. It is handy (and recommended) for pass such non-GEOM "disk" to another OS by means of iSCSI or bhyve(8).

It is also possible for some external driver to present /dev nodes without corresponding GEOM providers. It can define its own struct fileops with .fo_ioctl handler to process DIOCGDELETE and other ioctls.

Also, we already have virtio_blk driver that presents /dev/vtbd* device nodes to FreeBSD running as virtualized guest and this driver is not CAM-enabled.

I'd like to make the TRIM utility as generic as possible.

[cem]

In D18380#391248, @eugen_grosbein.net wrote:

First, DIOCGDELETE is not GEOM-specific ioctl()

I don't know why you say that. It was created specifically for GEOM (r169284). That's what the 'G' in the name represents, along with other similar ioctls (r92698).

On the other hand, there's nothing GEOM-specific about g_delete; it doesn't care what the underlying fd is.

I would be ok moving the g_delete and mediasize, etc, routines to libc for generic use on non-GEOM devices, but that doesn't leave much in libgeom (that's ok).

and we already have ZFS zvols that can be created as device nodes in /dev with non-default volmode=dev property instead of default volmode=geom. It is handy (and recommended) for pass such non-GEOM "disk" to another OS by means of iSCSI or bhyve(8).

It is also possible for some external driver to present /dev nodes without corresponding GEOM providers. It can define its own struct fileops with .fo_ioctl handler to process DIOCGDELETE and other ioctls.

Sure, I understand that the idea is to increase the scope of where the ioctl is valid. It's still the same ioctl number and g_delete() is perfectly capable of issuing it.

Also, we already have virtio_blk driver that presents /dev/vtbd* device nodes to FreeBSD running as virtualized guest and this driver is not CAM-enabled.

I'd like to make the TRIM utility as generic as possible.

I don't disagree (outside of DIOCGDELETE's origin); I just don't follow how this series of facts lead to the conclusion that we shouldn't use the library we already have, in base, that wraps the ioctls. It's sort of similar to when we make syscalls from utilities, we generally use the libc function wrappers instead of invoking SYSCALL directly.

[cem]

usr.sbin/trim/trim.c

104–106 ↗

(On Diff #51361)

I'll copy my note from the paste here:

It seems super dubious to trim the exact same offset and length on multiple devices. The only use case I can imagine is something like reformatting an existing RAID or mirrorset that all happen to be the same size.

I think we should default to only processing a single file/device, and require -f/force to process many devices. Or even just require multiple invocations of the command. Typical N for such scenarios is so small, and giant TRIMs are so costly, that I think any additional fork/exec overhead would be in the noise.

That raises another question around semantics of the ioctl — is it expected to be synchronous? It is for geom_devs; I don't know about zvols or future implementations. We should document expectations.

178–180 ↗

(On Diff #51361)

This can silently fail after partial completion due to signal, returning zero error, and there is no good recovery path. (Obviously, this utility doesn't introduce the problem; it's just exposed to it and doesn't explicitly mask signals.)

It'd be great if geom_dev (and ZFS and other implementations) updated arg[0] and arg[1] with the remaining segment on interrupt and returned EINTR instead of zero, to make recovery possible.

[cem]

usr.sbin/trim/trim.c

111–126 ↗

(On Diff #51361)

Another reimplementation of libgeom — g_device_path_open(), minus the stupid sectorsize check. 😂

We should just remove the silly sectorsize check from libgeom and use it here.

142–143 ↗

(On Diff #51361)

It seems super dubious to either issue trim(8) on a directory, or even use a directory's st_size as a reference point for trimming something else.

148 ↗

(On Diff #51361)

I was going to suggest removing S_ISBLK since we don't have those, but removing the check entirely works for me.

[eugen_grosbein.net]

usr.sbin/trim/trim.c

104–106 ↗

(On Diff #51361)

It seems super dubious to trim the exact same offset and length on multiple devices.

offset == 0 and length == 0 (defaults) means that trim() function erases whole device and there is no reason to disallow erase of multiple different devices with one utility invocation.

[eugen_grosbein.net]

usr.sbin/trim/trim.c

142–143 ↗

(On Diff #51361)

Why not? That's just a tool. What happened with "tools, not policy" principle and why do we have make artifical policy on how the tool should be used?

[eugen_grosbein.net]

usr.sbin/trim/trim.c

148 ↗

(On Diff #51361)

We had SBLK in the past. We may be having them in the future. There is no harm in such a check.

Entire removal of the check disables nice and readable error message in case of pilot error and replaces it with obscure ioctl() error. That's wrong.

[eugen_grosbein.net]

In D18380#391343, @cem wrote:

In D18380#391248, @eugen_grosbein.net wrote:

First, DIOCGDELETE is not GEOM-specific ioctl()

I don't know why you say that. It was created specifically for GEOM (r169284). That's what the 'G' in the name represents, along with other similar ioctls (r92698).

There is nothing GEOM-specific in this ioctl and 'G' in the name seems to be kind of mistake in the retrospection (it was not 11 years ago). And it does not mean that now trim(8) should restrict itself to libgeom, or be bloated with extra GEOM-style code instead several very simple lines including plain ioctl() call.

[cem]

In D18380#391393, @eugen_grosbein.net wrote:

There is nothing GEOM-specific in this ioctl and 'G' in the name seems to be kind of mistake in the retrospection (it was not 11 years ago).

Sure; the same goes for the rest of the GEOM ioctls that have 'G' in the name and are wrapped in libgeom. Maybe the libgeom name is a misnomer at this point (like 'G'), but not worth changing.

And it does not mean that now trim(8) should restrict itself to libgeom, or be bloated with extra GEOM-style code instead several very simple lines including plain ioctl() call.

I'm not sure how it would be restricted or bloated by libgeom. Using the shared libraries we already have is generally less bloaty than creating new copies of them.

libgeom.so.5 is 34kB. trim is 18kB. I don't really see any significance here. It's a short lived program and plenty of programs outside of the g* stuff you'd expect link libgeom already: bectl, bsdlabel, ccdconfig, disklabel, fdisk, hastd, mdconfig, zfs, zfsbootcfg, zpool, boot0cfg, efibootmgr, efidp, efivar, fstyp, sade, zdb, zfsd, zhack, zinject, ztest.

I.e., anyone running ZFS already has libgeom.so resident in memory all the time anyway. And it's common in disk-related tools.

usr.sbin/trim/trim.c

104–106 ↗

(On Diff #51361)

Ok, that's reasonable for that specific case; we could still require -f for other combinations of offset and length.

In particular, we actually set length to the device size any time it is zero, regardless of the value of offset. If offset != 0, we request a trim range beyond the end of the device.

142–143 ↗

(On Diff #51361)

Are you contending that trim on a directory makes sense, or that trim on a file/device using a directory's st_size makes any sense, or both?

Because I think it's pretty clear that the former is nonsense, and the latter seems like nonsense too. But I want to know which specific parts you disagree with.

Directories aren't files or disk devices; that's just part of how unix works. You might as well try to argue we should allow trimming network interfaces or sysctls, or allow users to create recursive directory structures (tools, not policy!), or something else crazy like that.

148 ↗

(On Diff #51361)

We may be having them in the future.

I don't think there's any reason to expect them to come back to devfs. They were removed in FreeBSD 3/4 timeframe (1999-2000) to consolidate on a single devfs interface and move away from meaningful major+minor numbers. There's no reason to expect them to suddenly become useful again.

You can of course mknod a VBLK inode still (at least, in tmpfs and some other weirder fs) but it doesn't do anything, isn't connected to a real device, and has no size.

There is no harm in such a check.

Sure there is; nonsense checks confuse readers by suggesting it's a reasonable thing to check. It isn't.

It's a moot point anyway; Warner has just removed the check entirely.

[eugen_grosbein.net]

usr.sbin/trim/trim.c

104–106 ↗

(On Diff #51361)

I prefer to require -f for any combination of offset and lenght before discarding any data.

we request a trim range beyond the end of the device

Yes. It seems to be harmless.

[eugen_grosbein.net]

Closed prematurely.

[imp]

I'm pissed this was committed. It wasn't ready and in total breach of protocol.
This matter is *NOT* settled and you're lucky I don't just remove it from the tree.

[eugen_grosbein.net]

In D18380#391441, @imp wrote:

I'm pissed this was committed. It wasn't ready and in total breach of protocol.
This matter is *NOT* settled and you're lucky I don't just remove it from the tree.

Fine, I've removed it.