Page MenuHomeFreeBSD

trim(8): candelete() returns wrong results because fd is opened O_WRONLY
ClosedPublic

Authored by allanjude on Apr 11 2020, 3:19 PM.

Details

Summary

The candelete() function does not properly detect if the disk supports TRIM
because the file descriptor is opened write-only, so the attempt to read
the GEOM::candelete attribute fails

Correct by switching the open() flag to O_RDWR

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

The change LGTM, but double check your commit message. Is there a missing "not"?

This revision is now accepted and ready to land.Apr 11 2020, 4:53 PM

I'm not sure I understand the problem.

The candelete() function does properly detect if the disk supports TRIM

Does or does not? Anyway, it works for me just fine using stable/11 system:

mdconfig -at malloc -s 1m

md0

  1. diskinfo -v md0 | grep TRIM Yes # TRIM/UNMAP support
  2. trim -f /dev/md0; echo $?

trim /dev/md0 offset 0 length 1048576
0

The function candelete() was copied from diskinfo(8) code as noted in the commit message. The diskinfo utility opens the device with O_RDONLY, too. It all was tested before committing.

The change LGTM, but double check your commit message. Is there a missing "not"?

Yes, there is a missing not

I'm not sure I understand the problem.

The candelete() function does properly detect if the disk supports TRIM

Does or does not? Anyway, it works for me just fine using stable/11 system:

mdconfig -at malloc -s 1m

md0

  1. diskinfo -v md0 | grep TRIM Yes # TRIM/UNMAP support
  2. trim -f /dev/md0; echo $?

trim /dev/md0 offset 0 length 1048576
0

The function candelete() was copied from diskinfo(8) code as noted in the commit message. The diskinfo utility opens the device with O_RDONLY, too. It all was tested before committing.

Yes, it works on mdconfig even though candelete() incorrectly returns 0, because the DIOCGDELETE is not conditional on candelete().

But if you send a trim that for example, exceeds the maximum size allowed by the device, DIOCGDELETE returns an error, and candelete() incorrectly returns that the device does not support trim, even though it does, and this results in the wrong error message (trim not supported, rather than DIOCGDELETE failed)

This seems to be driver-dependent as it works as expected for md:

trim -f -l 2000000 /dev/md0
trim /dev/md0 offset 0 length 2000000
trim: ioctl(DIOCGDELETE) failed: /dev/md0: Invalid argument

Can you please describe your real case when you need to use some device and explicit length/offset specification?

Also, trim(8) calls candelete() for cosmetic reasons only and in verbose mode only (by default).
"trim -q" does not call candelete() and always reveals real error of system call.

I'm not against suggested change but worry a little on changing opening mode just for cosmetic purpose as I'm not aware of implications for changing O_RDONLY to O_RDWR for trimming. Maybe there are non-existent.

OTOH, the tool is for destroying data and what can possible go worse? :-)