Implement ANA support #146
Conversation
hreinecke
force-pushed
the
ana-rework
branch
3 times, most recently
from
2102d3c
to
36e7580
Compare
|
@hreinecke Thank you for this pull request. I made some comments using this GitHub review system. When I ran nvme test group with the changes, many test cases failed. My comments are trying to avoid those failures. Please check and see if each comment makes sense. I wondered if the added test case nvme/054 should be skipped if the kernel does not support ANA. However, I noticed that the oldest long term branch 4.19.y supports ANA, so there is no need to check if the kernel supports ANA. |
|
Today I looked back the series, and noticed that "local -n" was used before this patch series. That's why @hreinecke used "local -n" to simplify the scripts. This means that the existing "local -n" did not work with bash version 4.2, but nobody complained about it. It might be the time to bump up the bash version requirement for blktests from 4.2 to 4.3. I'm not yet sure if do that bash requirement version bump. Now I'm trying to remove the "local -n" by myself to see how the script will look like. Tomorrow, I will share my work, hopefully. |
|
@hreinecke I spent some time to improve your series, and pushed the outcome to the ana-rework branch in my repo. I dropped one of your patches titled "nvme: sanitize _nvme_get_ctrl_list", and added 7 commits to reflect my comments. Through this work, I found that "local -n" nameref can be avoided with rather clean way. So now I think we should keep the bash version 4.2 requirement. I also confirmed that the all test cases in nvme test groups pass with tranports loop, rdma and tcp. Feel free to squash my 7 commits to your commits, if you think they are reasonable. |
There is no reason why loop is required, so switch to the default transport type. Signed-off-by: Hannes Reinecke <[email protected]>
All invocations are using 'nvme_trtype' as argument to _create_nvmet_ports(), so we can also make it optional and default to nvme_trtype. Signed-off-by: Hannes Reinecke <[email protected]>
We need to sanitize the transport parameters in _create_nvmet_port() to avoid having invalid transport values when selecting a transport type. Signed-off-by: Hannes Reinecke <[email protected]>
When _find_nvme_ns() fails it should return a non-zero statue to allow for the error to be propagated to the caller. Signed-off-by: Hannes Reinecke <[email protected]>
Rework the loop to iterate over ports, and check for the correct subsysnqn afterwards. With that we can drop 'sed' calls and simplify the loop. Signed-off-by: Hannes Reinecke <[email protected].
When calling 'nvme connect' we cannot assume that a namespace is always present (eg if connecting to a discovery service), so better check for the controller device to ensure that 'nvme connect' has succeeded. Signed-off-by: Hannes Reinecke <[email protected]>
A subsystem might have more than one port, and that port might be configured differently from the default settings. So rather than to rely on the default settings we should extract the parameters from the passed in ports on that subsystem and issue 'nvme connect' with the extracted parameters. Signed-off-by: Hannes Reinecke <[email protected]>
The -n option of the bash local variable declaration declares that the variable is used as nameref. However, this nameref feature was introduced to bash at its version 4.3. While blktests requires bash version 4.2. To not rely on the bash version 4.3, replace the nameref feature with a loop to parse arguments. Signed-off-by: Shin'ichiro Kawasaki <[email protected]>
A subsystem might have more than one namespace, so delete all of them in _remove_nvmet_subsystem(). Signed-off-by: Hannes Reinecke <[email protected]>
Support only long options for _create_nvmet_ns() to simplify calling sequence and allow to pass in an options ANA group id. If no UUID is passed the kernel will generate a UUID, so return the UUID to allow the caller to identify the generated namespace. Signed-off-by: Hannes Reinecke <[email protected]>
hreinecke
force-pushed
the
ana-rework
branch
2 times, most recently
from
8fa865d
to
221f663
Compare
|
I've now included your changes (especially the 'read -ra' trick, very nice!), and added a few more cleanups which I had pending. Please check. |
hreinecke
force-pushed
the
ana-rework
branch
3 times, most recently
from
1d67613
to
51058c7
Compare
|
Hi @hreinecke , I took a look in the branch. IMO, all changes have good motivations. Still I observe some test run failures and a shellcheck warning, but they do not look a big problems. So I hope these changes settled in the blktests. On the other hand, I think this PR is too big to apply to the master branch in one shot. It has 26 patches. Also, I think it's the better to get reviews on other Linux nvme experts. For these two reasons, I suggest to separate the 26 patches into 3 or 4 series and post to linux-nvme list one by one for review. This will take some time (I guess one week or two for each series, then in total one month or so), but will help to catch some missing mistakes, and will not surprise blktests users. If you agree, I would like to drive this multi-series cycle. Based on your branch, I will repeat the steps to, create a series, post to linux-nvme, and reflect review comments and apply them to the blktests master. Or if you want to do it by yourself, it is perfectly okay for me. Please let me know your thoughts. |
Most callers use the default settings anyway, so simplify them by using long options for _create_nvmet_subsystem(). Signed-off-by: Hannes Reinecke <[email protected]>
Add an option '--blkdev none' for _nvmet_target_setup() to indicate that no block devices (and no namespaces) should be created when setting up the target. Signed-off-by: Hannes Reinecke <[email protected]>
_nvmet_target_setup() is in tests/nvme/rc, but _nvmet_target_cleanup() is in common/nvme. So move the former into common/nvme to have both functions in the same place. Signed-off-by: Hannes Reinecke <[email protected]>
Simplify the testcase by not creating a namespace when setting up the target and have to loop iterating over all namespaces. Signed-off-by: Hannes Reinecke <[email protected]>
Simplify the testcase by not creating a namespace when setting up the target and have to loop iterating over all namespaces. Signed-off-by: Hannes Reinecke <[email protected]>
Simplify the testcase by not creating a namespace when setting up the target and have to loop iterating over all namespaces. Signed-off-by: Hannes Reinecke <[email protected]>
Most of the steps in _nvmet_setup() are not required here, and doesn't work with discovery connections. So open-code it to avoid the pitfalls and make the test simpler. Signed-off-by: Hannes Reinecke <[email protected]>
The default helper functions are doing quite some things which are unneccesary for this test, so open-code them to call just the required functions. Signed-off-by: Hannes Reinecke <[email protected]>
Rework fcloop handling to create only one local port but several remote ports. That way we can keep the global setting for local port addresses and don't need to worry about passing the correct local port address when calling 'nvme connect'. Signed-off-by: Hannes Reinecke <[email protected]>
Always called without arguments, so drop the assignment. Signed-off-by: Hannes Reinecke <[email protected]>
Add a function to set the ANA state and group id for a port. Signed-off-by: Hannes Reinecke <[email protected]>
Add a function to set the ANA groupid for a namespace. Signed-off-by: Hannes Reinecke <[email protected]>
Add an argument '--ports' to _nvmet_target_setup() to specify the number of ports to create. Signed-off-by: Hannes Reinecke <[email protected]>
A port might have several ANA groups, and we have to remove all ANA groups with a group id other than 1, otherwise we cannot remove the port itself. Signed-off-by: Hannes Reinecke <[email protected]>
Add a test for basic ANA support by creating 4 paths, run a fio process to generate load, and then switch port states to check if I/O continues uninterrupted. Signed-off-by: Hannes Reinecke <[email protected]>
Add a test for rapid namespace remapping to simulate short-lived namespaces being created and deleted in rapid succession with a cluster. Signed-off-by: Hannes Reinecke <[email protected]>
|
It's perfectly fine to split it into a multi-series patchset. Please go ahead. |
|
I have just applied two other patches by Guixin, which conflict with this PR. Yesterday, I spent sometime to resolve the conflicts, and pushed it to here. Will evaluate them and split into a few series to post to linux-block. |
|
I split the patches into three series. The first series was applied to the blktests master reflecting review comments on the list, from the commit e3fe06e to fe6cd13. Now I have just posted the second series for the review. |
|
The second series was applied on Nov/19. |
Hi all,
here's a patchset to implement ANA support, together with a basic test for ANA failover while I/O is running.
As usual, comments and reviews are welcome.