On 04/09/2015 05:50 AM, Ilias Tsitsimpis wrote:
Thanks for this patch series and sorry for the late reply.
Comments are inline.
Thanks for taking a look.
On Tue, Apr 07, 2015 at 01:47PM, Andy Grover wrote:
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.
I still think this makes sense, because the range of opcode values is
going to remain very small. Given the futureproofing you propose for
cmd_entry, we might never need additional opcodes. So 3 bits seems like
enough, and giving it even a u8 likely too many.
+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.
How you describe it is how I also initially thought about doing it. I
see some very trivial pros and cons either way, but if UNKNOWN_OPCODE is
more straightforward in your view, then let's do it that way.
@@ -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).
good point.
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.
I guess it was an unstated assumption on my part that userspace would
never set SKIPPED/UNKNOWN for OP_CMD. I guess this actually is possible,
good point.
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.
will do.
- 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?
Sounds good. For small iov_cnts the cmd size is going to be sized to fit
the sense_buffer anyways.
I will post a v2 soon.
Thanks! -- Regards -- Andy
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
