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

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

Details

Reviewers

eugen_grosbein.net
asomers
kevans
chuck
jhb
imp

Commits

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

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

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30421
Build 28180: arc lint + arc unit

Event Timeline

allanjude created this revision.Apr 11 2020, 3:19 PM

Harbormaster completed remote builds in B30421: Diff 70431.Apr 11 2020, 3:19 PM

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

diskinfo -v md0 | grep TRIM Yes # TRIM/UNMAP support
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.

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

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

diskinfo -v md0 | grep TRIM Yes # TRIM/UNMAP support

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

Closed by commit rS360230: trim(8): candelete() returns wrong results because fd is opened O_WRONLY (authored by allanjude). · Explain WhyApr 23 2020, 8:15 PM

This revision was automatically updated to reflect the committed changes.

allanjude added a commit: rS360230: trim(8): candelete() returns wrong results because fd is opened O_WRONLY.