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

[allanjude]

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

[asomers]

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

[eugen_grosbein.net]

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.

[allanjude]

In D24371#536107, @asomers wrote:

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

Yes, there is a missing not

[allanjude]

In D24371#536111, @eugen_grosbein.net wrote:

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)

[eugen_grosbein.net]

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?

[eugen_grosbein.net]

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? :-)