update the Expected Logical Block Application Tag and Reference Tag in nvme copy command #587
Conversation
There was a problem hiding this comment.
- The commit subject should explain what changes and it should use a prefix "types: update ..."
- Please also add a commit message which explains the change. We aim for having all relevant information in the git tree.
- Updating the 'struct nvme_copy_range' struct sound reasonable because it is a real bug fix and doesn't introduce an API breakage
- Not so sure about
struct nvme_copy_range_f1. It is breaking API and I haven't really figured out yet if we should this.
|
See also #455 |
…n nvme copy command 1、The elbatm is behind elbat fields,whitch are defined in section 3.2.2 Copy command 2、Storage and Reference Space is declared as an big-endian order,the elbt[10] fields need to reassigned
zwxdg
changed the title
| copy[i].elbatm = cpu_to_le16(elbatms[i]); | ||
| copy[i].elbat = cpu_to_le16(elbats[i]); | ||
| for (j = 0; j < 8; j++) | ||
| copy[i].elbt[9 - j] = (eilbrts[i] >> (8 * j)) & 0xff; |
There was a problem hiding this comment.
I think the issue here is that this stores the EILBRT values in big-endian order, like the linked PR which was closed.
Storing them in little-endian order requires the Storage Tag Size and Protection Information Format to determine where the EILBRT should start in the shared range.
There was a problem hiding this comment.
Since both EILBRT and ELBST values are store in big-endian order, and the device software know the Storage Tag Size and what kind of Protection Information Format used, so i think the location of EILBRT in the shared range is handle by device software, as is the ELBST.
For nvme-cli，we only need to deliver EILBRT and ELBST in big-end mode to the device.
There was a problem hiding this comment.
Since both EILBRT and ELBST values are store in big-endian order
Most of the spec is using little endian and I can't find anything EILBRT and ELBST being big endian. The EILBRT and ELBST definition is, let's say a 'challenging' definition. Where do you find the info on big-endian?
There was a problem hiding this comment.
in section 5.2.1 Protection Information Formats. for example, the screenshot show the 16b guard protection information format, EILBRT and ELBST re store in big-endian order
There was a problem hiding this comment.
Ah, I didn't look at the Figure :)
I think you should use the be64 helpers as the argument type for eilbert is u64 which is in host endian.
But as I said pointed out, this change breaks the API. We can't just change data structures in this way. This was the reason why I didn't merge #455.
There was a problem hiding this comment.
Yeah I'd thought that the MSB/LSB in that figure might be "Most/Least Significant Bits" rather than defining each of these fields as big-endian byte ordering, particularly based on figure 122 in the next section on the shared space and the consistent mention of significant bits rather than bytes:
Definitely see how it could be describing the byte-ordering, but it feels odd without a more explicit call-out like NGUID or EUI64. Worth mentioning this would mean similar ordering changes are likely needed for the shared field and for application tag/guard fields across all IO commands.
There was a problem hiding this comment.
Ah, but also there's no requirement from the spec that the tags are divisible into bytes. Setting the reference tag in big-endian order should get the right bit ordering and properly handle cases like the spec example with a 30-bit reference tag.
Changes look good to me with fixing the ELBATM types location, the bit-ordering fix could be addressed more broadly but this works for copy.
There was a problem hiding this comment.
Thanks for the feedback. I looked again with fresh eyes I figured that the github webui confused me. So the two commits should be obviously merged and the final result is just:
--- a/src/nvme/types.h
+++ b/src/nvme/types.h
@@ -4640,8 +4640,8 @@ struct nvme_copy_range {
__le16 nlb;
__u8 rsvd18[6];
__le32 eilbrt;
- __le16 elbatm;
__le16 elbat;
+ __le16 elbatm;
};
/**
@@ -4661,8 +4661,8 @@ struct nvme_copy_range_f1 {
__le16 nlb;
__u8 rsvd18[8];
__u8 elbt[10];
- __le16 elbatm;
__le16 elbat;
+ __le16 elbatm;
};which is okay I think to merge.
|
I've applied the patch directly after squashing into one and updating the commit message. |
1、The elbatm is behind elbat fields, whitch are defined in section 3.2.2 Copy command.
2、Storage and Reference Space is declared as an big-endian order, the elbt[10] fields need to reassigned.