From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linux-iscsi.org ([67.23.28.174]:49306 "EHLO linux-iscsi.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750759AbdFCFc1 (ORCPT ); Sat, 3 Jun 2017 01:32:27 -0400 Message-ID: <1496467944.27407.299.camel@haakon3.risingtidesystems.com> Subject: Re: [PATCH 04/33] target: Fix BYTCHK=0 handling for VERIFY and WRITE AND VERIFY commands From: "Nicholas A. Bellinger" To: Bart Van Assche Cc: "hch@lst.de" , "ddiss@suse.de" , "hare@suse.com" , "target-devel@vger.kernel.org" , "agrover@redhat.com" , "stable@vger.kernel.org" Date: Fri, 02 Jun 2017 22:32:24 -0700 In-Reply-To: <1496422372.1214.9.camel@sandisk.com> References: <20170523234854.21452-1-bart.vanassche@sandisk.com> <20170523234854.21452-5-bart.vanassche@sandisk.com> <1496376930.27407.234.camel@haakon3.risingtidesystems.com> <1496422372.1214.9.camel@sandisk.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: On Fri, 2017-06-02 at 16:52 +0000, Bart Van Assche wrote: > On Thu, 2017-06-01 at 21:15 -0700, Nicholas A. Bellinger wrote: > > On Tue, 2017-05-23 at 16:48 -0700, Bart Van Assche wrote: > > > For VERIFY and WRITE AND VERIFY commands the size of the SCSI > > > Data-Out buffer can differ from the size of the data area on the > > > storage medium that is affected by the command. Make sure that > > > the Data-Out buffer size is computed correctly if the BYTCHK > > > field in the CDB is zero. This patch reverts commit 984a9d4c40be > > > and thereby restores commit 0e2eb7d12eaa. Additionally, > > > sbc_parse_cdb() is modified such that the data buffer size is > > > computed correctly for the affected commands if BYTCHK == 0. > > > This patch is the combination of two patches that got positive > > > reviews. > > > > > > References: commit 984a9d4c40be ("Revert "target: Fix VERIFY and WRITE VERIFY command parsing"") > > > References: commit 0e2eb7d12eaa ("target: Fix VERIFY and WRITE VERIFY command parsing") > > > Signed-off-by: Bart Van Assche > > > Cc: Hannes Reinecke > > > Cc: Christoph Hellwig > > > Cc: Andy Grover > > > Cc: David Disseldorp > > > Cc: > > > --- > > > drivers/target/target_core_sbc.c | 79 ++++++++++++++++++++++++++++++++++------ > > > 1 file changed, 67 insertions(+), 12 deletions(-) > > > > > > > This patch ignored the review comments from the last round: > > > > http://www.spinics.net/lists/target-devel/msg15306.html > > http://www.spinics.net/lists/target-devel/msg15327.html > > > > Until these are addressed as requested, dropping this patch for now. > > Hello Nic, > > 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. 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. Tthat said, I already reverted this patch once because you didn't want to make these very simple changes, so I'll not be merging it until the changes are made.