libnvme: Fix the Reference Tag in copy command

[Francis-Pravin]

The elbt is declared as elbt[10] to store 80-bit refernce tag value. For now,
the Kernel supports 32-bit and 48-bit refernce tag only. Therefore the value is
received in the 64-bit variable. So elbt[2] to elbt[9] is used to store that 64-bit value.

Identify the type of copy format using args->format instead of structure size.
Also, use a single 64-bit ilbrt instead of using two ilbrt with different datatypes.

Signed-off-by: Francis Pravin Antony Michael Raj [email protected]
Signed-off-by: Jonathan Derrick [email protected]

[codecov-commenter]

Codecov Report

Merging #455 (1724f29) into master (0e4d1ce) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
- Coverage   18.17%   18.16%   -0.02%     
==========================================
  Files          31       31              
  Lines        5211     5214       +3     
  Branches      998      999       +1     
==========================================
  Hits          947      947              
- Misses       3992     3995       +3     
  Partials      272      272              

Impacted Files	Coverage Δ
src/nvme/ioctl.c	0.00% <0.00%> (ø)
src/nvme/util.c	0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[igaw]

You need to find another way to address this. We can fix this properly when we do the next major version of the library until then we have to find ways around without breaking the API.

[igaw]

This breaks the API

[Francis-Pravin]

I have one more patch for nvme-cli to fix this issue. In that patch, this renamed ilbrt variable is handled. I hope the API will not break once it is merged.

[igaw]

As far I can tell, libnvme is not just used by nvme-cli. This is why I keep repeating we can't break the API for the 1.x series of the library. For 2.x sure, I don't have any problems to modify the API.

BTW, I would suggest to open an in issue with the exact work description and queue it up for the 2.x library. I fear if we don't write it down, this will be lost.

[Francis-Pravin]

Hi @igaw
I missed to mention a point.
In struct nvme_copy_args, ilbrt and ilbrt_u64 are used to store the reference tag values of 32-bit and 64-bit respectively. The ilbrt_u64 is only assigned with cfg.ilbrt value in nvme-cli. Whereas ilbrt is not assigned with any value. But, both the variables are handled in nvme_copy() libnvme. It seems fishy for me. That's why I tried to use single ilbrt instead of two variable with different data type.

[igaw]

and this one too.

[bjpaupor]

Isn't this just a more complicated way of storing the eilbrt little-endian order in the last 8 bytes of elbt (as is removed above)?

[Francis-Pravin]

I have verified the copy command with QEMU emulated NVMe device. On using previous logic, the controller received the eilbrt value as zero via format1. Hence, I stored the value byte by byte. Here observed the controller is receiving the appropriate value.

[igaw]

Which version of the nvme-cli and libnvme did you use? This might be caused by my brown bag release where I
crippled the nvme_init_copy_range_f1 function. The result was, calling nvme_init_copy_range_f1 was a no-op.

linux-nvme/nvme-cli@fd582b0

[Francis-Pravin]

Always I use the latest version of nvme-cli and libnvme. The issue was observed even before the above mentioned changes.

[igaw]

That would have been too easy...

[igaw]

If I understand the situation the main issue is that:

copy[i].elbt[2] = cpu_to_le64(eilbrts[i]);

is not working correctly. I suggest you just fix this using the existing API as we can't change it (only extend it).

[jderrick]

Hi @igaw ,

copy[i].elbt[2] = cpu_to_le64(eilbrts[i]);

Was the intent here to fill [2] - [9] ?
I have a question about this:

1. Shouldn't this start at [0]
2. Wouldn't this be non-little endian?

The same thing appears to be taking place in qemu:
https://elixir.bootlin.com/qemu/latest/source/hw/nvme/ctrl.c#L2658

[igaw]

I haven't really digged into the details about this feature until now. I think we have two things to address here.

First, is the handling of the API backwards compatibility, meaning how read the struct nvme_copy_args. The current version looks okay but it is awkwardly used. That means we have functional dependency on the version of struct. I think this is what you also tried to resolve and I said no.

        if (args->args_size == size_v1) {                                                                     
                cdw3 = 0;                                                                                     
                cdw14 = args->ilbrt;                                                                          
        } else {                                                                                              
                cdw3 = (args->ilbrt_u64 >> 32) & 0xffffffff;                                                  
                cdw14 = args->ilbrt_u64 & 0xffffffff;                                                         
        }         

Second we have to handle various sizes of LBST/ELBST/ILBRT/EIBTR.

Command Set Specification
Figure 35: Copy – Source Range Entries Descriptor Format 1h

elbt:

This field specifies variable sized Expected Logical Block Storage Tag (ELBST) and
Expected Initial Logical Block Reference Tag (EILBRT) fields, which are defined in section
5.2.1.4.1, to be used for the read portion of the copy operation. If the namespace is not
formatted to use end-to-end protection information, then this field is ignored.

As I understand the current version of nvme_copy() is assuming a specific STS value. To address this I think we need to extend struct nvme_copy_args with a STS field like struct nvme_io_args and update nvme_copy() and nvme_init_copy_range_f1().

Comments?

[bjpaupor]

@igaw To the second point, there's a few problems with the current implementation and with the proposed fix here.

• The current implementation only actually sets elbt[2] to eilbrt & 0xFF rather than elbt[2-9] to the full value. This could be fixed by using a 64-bit pointer or by setting it byte-wise like this proposed fix.
• The proposed fix here nearly works as it does set the correct bytes, but the problem is that it's equivalent to properly setting elbt[2] = cpu_to_be64(elibrt), as a big-endian value.
• Even adjusting the current implementation to use a 64-bit pointer gives a problem that it will start the value from elbt[2-X] rather than elbt[X-9].

Only way I see to properly set this value is by breaking the API to get the Storage Tag Size and using something like:
elbt << STS = cpu_to_le64(eilbrt)

[Francis-Pravin]

Thanks for the comments.
I totally agree it. To handle the various sizes of LBST/ELBST/ILBRT/EIBTR, STS and PIF values are required. Because, the size of the reference tag are based on those values. Hence We need to extend the struct nvme_copy_args like in struct nvme_io_args. I think it will breaks the API. As @igaw commented above we can't break the API for the 1.x series of the library.
Already an issue has been created in libnvme to fix this issue in 2.0 version. #459
Please add your suggestion on closing this PR.

[igaw]

I think there is a misunderstanding. I said, we can't break the API but this doesn't mean we cannot extend it. This is where the versioning of the struct args comes into play. If a struct arg is passed into a function we check the size of it and only access added members if it is possible, hence the size check. So in this case the STS member needs to be added after the last current member (plus pending if needed) and then we can use it in the function.

See also 9350e73

[bjpaupor]

That should work for the nvme_copy function and set tags for the write portion of the copy command, but the nvme_init_copy_range_f1 function doesn't have a 'struct args' to extend for the read portion tags.

For that we'd need to update the signature to include PIF and STS, and maybe convert the list of parameters to a struct that could be versioned and extended in the future.

[igaw]

Got it. In this case we have to introduce a nvme_init_copy_range_f1 v2 (which we are going to merge when we do the 2.x release of the library).

[igaw]

Not sure about adding struct args to the util function though. We had this discussion when we introduce the struct args in the api-types.h header if we should do the same for the helpers and decided against it. Can't remember the argument though.

[Francis-Pravin]

I need some clarification on identifying the copy format type based on the sizeof_args value.
In nvme-cli, args_size is assigned with sizeof(args) which means args_size will be assigned with whole structure size value. But, in libnvme the format type is identified based on the size_v1 and size_v2. Based on my understanding, the control will always go to else condition only. Could you please throw light on this issue.

[igaw]

A nvme-cli binary build against v1 will only pass in arg_size = size_v1. If you happen to have a newer library installed which has support for v2 it will not attempt to read anything but v1 members. The arg_size is only relevant for binary incompatibility.

[igaw]

Closing due to inactivity.