Conversation
lorelei-sakai
left a comment
lorelei-sakai
left a comment
There was a problem hiding this comment.
Deprecation is mostly a question of how and when we communicate it. We'll try to figure out what we want to do for this next week once the whole team is around.
The message isn't really hurting anything, but if we do deprecate it, it might make sense to deprecate all the run-time messages together.
Historically, the dmsetup compression message was used to turn on and off compression, before the use of lvm for managing vdo volumes. However, vdo volumes are usually managed by lvm nowadays, which reloads the table line to turn on and off compression. Having two methods to change compression state (table line and message) also results in the possibility of having unexpected state if both are in use. Hence, mark the compression message as deprecated, in hopes of removing it in a couple kernel versions. Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
performSetVDOCompressing() is intended to act like the compression on/off message -- it calls the same VDO function, allowing stopping compression while IOs are in progress. This is a potentially valuable feature for unit tests insofar as it allows a IO to land in the packer, then compression can turn on/off without the IO making progress, and then the behavior of the IO can be verified with the packer having changed state. However, no unit test actually does this, and it's not clear that we could actually ever use a unit test that uses this behavior. In fact, in every case, we can reload the table line and do a suspend/resume cycle in order to change the compression status, just as we (hopefully) see users do on the kernel target. As part, then, of deprecating the message in favor of changing the table line, replace all uses of performSetVDOCompressing() with modifyCompressDedupe(), which does a suspend/resume with a new table line. Signed-off-by: Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
sweettea
force-pushed
the
deprecate/comp/msg
branch
from
0c3788a to
4e15bf7
Compare
|
I realized a missing part of this was updating unit tests to not (essentially) use the message. So +1 change. |
|
The unit test changes aren't strictly necessary, but I agree that if we deprecate the message, the unit tests should switch to using the new preferred code pathway. So that commit is fine. |
|
I shouldn't have let this issue linger for so long. There's really no reason to deprecate the online compression enabling and disabling feature, and it's independent of the work in PR #241 so I'm closing this one. |
3 participants
As discussed, this change emits a warning when people use the compression message.
Happy to send this upstream myself if preferred once this is acceptable.