Hi Andy,
Thanks for this patch series and sorry for the late reply.
Comments are inline.
<snip>
On Tue, Apr 07, 2015 at 01:47PM, Andy Grover wrote:
> @@ -140,25 +140,37 @@ processed by userspace.
>
> TCMU commands start with a common header containing "len_op", a 32-bit
> value that stores the length, as well as the opcode in the lowest
> -unused bits. Currently only two opcodes are defined, TCMU_OP_PAD and
Why keep both the length and the opcode in the same variable? Maybe now
that you are adding more variables inside the common header we can split
this one into two separate fields, to make things easier.
<snip>
> +When the opcode is PAD, userspace should set CMD_SKIPPED bit (bit 0)
> +in hdr.uflags, and then similarly update cmd_tail as above. (The
> +kernel inserts PAD entries to ensure each CMD entry fits contigously
> +into the circular buffer.)
With this approach, the user will have to set the CMD_SKIPPED everytime
they encounter a PAD opcode. I'd propose we name the bit
"UNKNOWN_OPCODE" instead, and have userspace set it only in case where
it cannot handle the given opcode. Then, TCMU should complete the
specified command properly; for example for a TCMU_OP_CMD command, TCMU
should terminate the command with the sense key set to ILLEGAL REQUEST.
Having a separate UNKNOWN_OPCODE bit allows us to tell the difference
between the cases where the user handled the opcode properly, even if it
was PAD, and the cases where they didn't.
<snip>
> @@ -346,6 +346,9 @@ static int tcmu_queue_cmd_ring(struct tcmu_cmd *tcmu_cmd)
> tcmu_flush_dcache_range(entry, sizeof(*entry));
> tcmu_hdr_set_op(&entry->hdr, TCMU_OP_PAD);
> tcmu_hdr_set_len(&entry->hdr, pad_size);
Now that entry->hdr contains more that one fields, maybe replace
entry->hdr with entry->hdr.len_op (there are a few places where this
needs to be changed).
<snip>
> @@ -543,13 +548,16 @@ static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
> tcmu_flush_dcache_range(entry, sizeof(*entry));
>
> if (tcmu_hdr_get_op(&entry->hdr) == TCMU_OP_PAD) {
> + if (!(entry->hdr.uflags & TCMU_OP_UFLAG_CMD_SKIPPED))
> + pr_warn("PAD completed without SKIPPED bit set\n");
> +
> UPDATE_HEAD(udev->cmdr_last_cleaned, tcmu_hdr_get_len(&entry->hdr), udev->cmdr_size);
> continue;
> }
Here you do not handle the case were opcode is TCMU_OP_CMD but the user
has set the CMD_SKIPPED bit.
<snip>
> @@ -840,14 +848,14 @@ static int tcmu_configure_device(struct se_device *dev)
> udev->data_size = TCMU_RING_SIZE - CMDR_SIZE;
>
> mb = udev->mb_addr;
> - mb->version = 1;
> + mb->version = 2;
> mb->cmdr_off = CMDR_OFF;
> mb->cmdr_size = udev->cmdr_size;
>
> WARN_ON(!PAGE_ALIGNED(udev->data_off));
> WARN_ON(udev->data_size % PAGE_SIZE);
>
> - info->version = "1";
> + info->version = "2";
Use TCMU_MAILBOX_VERSION for both 'mb->version' and 'info->version'
instead of the hardcoded value. Also update TCMU_MAILBOX_VERSION and
TCMU_VERSION in the header file.
<snip>
> @@ -97,13 +102,11 @@ static inline void tcmu_hdr_set_len(struct tcmu_cmd_entry_hdr *hdr, __u32 len)
> struct tcmu_cmd_entry {
> struct tcmu_cmd_entry_hdr hdr;
>
> - uint16_t cmd_id;
> - uint16_t __pad1;
> -
> union {
> struct {
> uint64_t cdb_off;
> - uint64_t iov_cnt;
> + uint32_t iov_cnt;
> + uint32_t iov_bidi_cnt;
Adding the extra iov_bidi_cnt field enables support for BIDI commands
but doesn't ensure full parity with SAM. As we discussed in a previous
email, SAM specifies the fields that a transport protocol service should
carry for a Send SCSI Command request. Among these fields are the 'Task
Attribute', 'CRN' and 'Command Priority' fields. I understand that none
of these are supported by TCMU right now, but maybe they will one day.
Should that day come, we should be able to support them without changing
the ABI again.
So, I think it makes sense to add a few reserved bytes inside
tcmu_cmd_entry.req, to support this kind of functionality in the future,
without having to go through the process of changing the ABI again. I
think 16 bytes would be more than enough. What do you think?
Thanks,
Ilias
> struct iovec iov[0];
> } req;
> struct {
> --
> 2.1.0
Attachment:
signature.asc
Description: Digital signature
