On Fri, 2017-06-02 at 22:32 -0700, Nicholas A. Bellinger wrote: > On Fri, 2017-06-02 at 16:52 +0000, Bart Van Assche wrote: > > In this patch series I have addressed all comments that made sense to me. Sorry > > if you feel offended because I had not addressed the two comments you referred to > > above. The reason I had not addressed these comments is because these comments > > are wrong in my opinion. Hence, please reconsider this patch. > > Nope. Here are the details again. > > First, it drops setting SCF_SCSI_DATA_CDB for WRITE_VERIFY in all cases, > and only sets it for BYTCHK=0. > > Yes, I understand the spec says hosts are not supposed to send a payload > when BYTCHK=0, but that doesn't stop some from trying. > > Any CDB that can potentially allocate SGLS via target_alloc_sgl() must > set this flag. No other CDBs set SCF_SCSI_DATA_CDB based on bits in the > CDB, and *_VERIFY is no exception. A quote from the SBC-4 section about VERIFY(10): "If the byte check (BYTCHK) field is set to 00b, then no Data-Out Buffer transfer shall occur". In other words, if a VERIFY or WRITE VERIFY command is received with BYTCHK=0, transferring the Data-Out buffer is not only superfluous it is also against the SCSI specs. Today target_cmd_size_check() terminates SCSI commands for which the size of the Data-Out buffer exceeds the expected size with TCM_INVALID_CDB_FIELD so the data transfer doesn't happen anyway. Hence it is not necessary to allocate an SGL with target_alloc_sgl() if BYTCHK=0. > Secondly, the force setting of size in sbc_parse_verify(), instead of > what was actually received over the write is totally wrong. Like I said > before, the size in sbc_parse_cdb() is what's extracted from the CDB > transfer length, and not what the spec says the correct size should be. Please take the SCSI specs seriously instead of ignoring the SCSI specs. I think for VERIFY and WRITE VERIFY with BYTCHK=0, the size extracted from the CDB should be zero bytes. What's needed in my opinion to make VERIFY and WRITE VERIFY processing compliant with the SCSI specs is the following: - Patch 04/33 from this series that fixes the parsing of these commands. - Patch 25/33 from this series that fixes handling of too large Data-Out buffers for the iSCSI target driver (the qla2xxx and ib_srpt target drivers already handle this case correctly). - Patch 30/33 from this series that makes target_cmd_size_check() send the correct sense code to the initiator system for too large Data-Out buffers. Bart.