tcmu: enable max_data_area_mb support #95
Conversation
This patch allows the user to set the ring buffer data area size. It requires this kernel patch: https://www.spinics.net/lists/target-devel/msg16140.html In this patchset. https://www.spinics.net/lists/target-devel/msg16121.html and it requires this rtslib patch: open-iscsi/rtslib-fb#112
| wwn=wwn, | ||
| hw_max_sectors=hw_max_sectors) | ||
| except: | ||
| raise ExecutionError("UserBackedStorageObject creation failed.") |
There was a problem hiding this comment.
Andy,
One question/comment for my patch. I was not sure how much rtslib compat support was needed. Do you normally require users to update rtslib and targetcli together? The code above handles where you have a new targetcli but old rtslib that does not support max_data_area support. I can drop this if you want.
There was a problem hiding this comment.
Yeah, I've tried to keep the coupling clean, although it hasn't always been possible. It's a good thing to do.
There was a problem hiding this comment.
[edit: formatting]
Going forward then to make it easier, do you think it would be ok to separate the write(enable=1) step from the constructor? So for tcmu you might do:
dev = UserBackedStorageObject(pass_in_required_args_like_name_and_config_like_usual.
new_arg_deferred_enable=true)
try:
dev.set_control("setting", value)
catch RTSLibControlError:
# app decides if fatal or not
dev.enable = 1
We won't have to update rtslib for every new control setting. rtslib could also have a function that reads the info file and returns a list of control settings. In this deferred mode, targetcli could then display and set them similar to attributes. We would then not have to update targetcli for every new setting too. So you would do
backstores/user:rbd> create name=test cfgstring=rbd/test
// isimiar to get attribute
/backstores/user:rbd/test> get control
dev_config
dev_size
hw_block_size
max_data_area_mb/backstores/user:rbd/test> set control max_data_area_mb=10
/backstores/user:rbd/test> enable
There was a problem hiding this comment.
This sounds like a solid improvement to me.
There was a problem hiding this comment.
Ok. I am going to close this and the rtslib PR.
Will open new PRs when I have code for this.
Thanks.
|
Test and works for me. |
| raise ExecutionError("UserBackedStorageObject creation failed.") | ||
| wwn=wwn, hw_max_sectors=hw_max_sectors, | ||
| max_data_area_mb=max_data_area_mb) | ||
| except TypeError as e: |
There was a problem hiding this comment.
There is a bug here. I need a generic except statement to catch other exceptions thrown. Will fix and repush.
|
@mikechristie how's this PR looking? |
|
I waswaiting for you to reply to my last comment :) https://github.com/open-iscsi/targetcli-fb/pull/95/files#r147223224 |
|
doh, replied. Sounds good! |
This patch allows the user to set the ring buffer data area size. It requires this kernel patch:
https://www.spinics.net/lists/target-devel/msg16140.html
In this patchset.
https://www.spinics.net/lists/target-devel/msg16121.html
and it requires this rtslib patch:
open-iscsi/rtslib-fb#112