On Thu, 2015-04-09 at 13:17 -0700, Andy Grover wrote:
> 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.
>
On a separate note, if you're going to break compatibility for adding
bidi support, it would make sense to also consider future proofing the
API now to allow protection buffers to communicated to user-space as
well.
--nab
--
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
