From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757653AbcHYHTZ (ORCPT ); Thu, 25 Aug 2016 03:19:25 -0400 Received: from mx0a-00003501.pphosted.com ([67.231.144.15]:41419 "EHLO mx0a-000cda01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756676AbcHYHTV (ORCPT ); Thu, 25 Aug 2016 03:19:21 -0400 Authentication-Results: seagate.com; dkim=pass header.s="google" header.d=seagate.com MIME-Version: 1.0 In-Reply-To: References: <20160822042321.24367-1-shaun@tancheff.com> <20160822042321.24367-2-shaun@tancheff.com> From: Shaun Tancheff Date: Thu, 25 Aug 2016 02:18:52 -0500 Message-ID: Subject: Re: [PATCH v6 1/4] libata: Safely overwrite attached page in WRITE SAME xlat To: Tom Yan Cc: "Martin K. Petersen" , Shaun Tancheff , linux-ide@vger.kernel.org, LKML , Tejun Heo , Christoph Hellwig , Damien Le Moal , Hannes Reinecke , Josh Bingaman , Hannes Reinecke Content-Type: text/plain; charset=UTF-8 X-Proofpoint-PolicyRoute: Outbound X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-08-25_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 suspectscore=1 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 impostorscore=0 lowpriorityscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1604210000 definitions=main-1608250084 X-Proofpoint-Spam-Policy: Default Domain Policy Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 25, 2016 at 1:31 AM, Tom Yan wrote: > On 25 August 2016 at 05:28, Shaun Tancheff wrote: >> On Wed, Aug 24, 2016 at 12:31 AM, Tom Yan wrote: >>> On 24 August 2016 at 11:33, Martin K. Petersen >>> wrote: >>>>>>>>> "Tom" == Tom Yan writes: >>>> >>>> Tom> Nope, SCSI Write Same commands does not have payload (or in SCSI >>>> Tom> terms, parameter list / data-out buffer). >>>> >>>> WRITE SAME has a a payload of 1 logical block (unless NDOB is set but we >>>> have had no good reason to support that yet). >>> >>> Interesting, I wasn't aware of the bit. I just didn't see any >>> parameter list defined for any of the Write Same commands. Ah wait, it >>> carries the pattern (the "same") and so. >>> >>> Hmm, it doesn't seem like the translation implemented in this patch >>> series cares about the payload though? >> >> As repeated here and elsewhere the payload is: >> scsi_sglist(cmd) >> and it was put there by scsi_init_io() when it called scsi_init_sgtable() > > What I mean is: > > put_unaligned_le32(0u, &sctpg[10]); > > So even if the payload of the SCSI Write Same commands instruct a > non-zero pattern writing, SCT Write Same will conveniently ignore that > do zero-filling anyway. That's what I mean by "doesn't care about the > payload". If you would like to add support for that it would be nice. I am not planning to do so here. > Though that would only be case with SG_IO though. SCSI Write Same > issued from block layer (BLKZEROOUT) will be instructing zero-filling > anyway. >>>> UNMAP has a payload that varies based on the number of range >>>> descriptors. The SCSI disk driver only ever issues a single descriptor >>>> but since libata doesn't support UNMAP this doesn't really come into >>>> play. >>>> >>>> Ideally there would be a way to distinguish between device limits for >>>> WRITE SAME with the UNMAP bit and for "regular" WRITE SAME. One way to >>>> do that would be to transition the libata discard implementation over to >>>> single-range UNMAP, fill out the relevant VPD page B0 fields and leave >>>> the WRITE SAME bits for writing zeroes. >>>> >>>> One reason that has not been particularly compelling is that the WRITE >>>> SAME payload buffer does double duty to hold the ATA DSM TRIM range >>> >>> Huh? Why would the SATL care about its payload buffer for TRIM (i.e. >>> when the UNMAP bit is set)? Doesn't it just read the LBA and NUMBER OF >>> BLOCKS field and pack TRIM ranges/payload according to that? >>> >>>> descriptors and matches the required ATA payload size. Whereas the UNMAP >>> >>> Why would it need to "matches the required ATA payload size"? >>> >>>> command would only provide 24 bytes of TRIM range space. >>> >>> I don't really follow. The UNMAP descriptor has LBA (8 bytes / 64-bit) >>> and NUMBER OF BLOCKS (4 bytes / 32-bit) field of the same length as >>> Write Same (16). Even if the SCSI disk driver will only send one >>> descriptor, it should work as good as Write Same (16). >> >> The "payload" is the data block transferred with the command. >> The "descriptor" is, in this context, the contents of the payload as >> it "describes" what the action the command is supposed to perform. >> > > I know right. > >> The "payload" contains the "descriptor" that "describes" how >> DSM TRIM should determine which logical blocks it should UNMAP. > > This should only be the case of UNMAP command, but not SCSI Write Same > with UNMAP bit set. And either way it should not affect how large the > ATA TRIM payload can be. The current SATL does not report support for UNMAP. If you think it should be added please submit a patch. If you would like to extend the current translate to support sending multiple blocks of trim data please submit a patch. >>>> Also, please be careful with transfer lengths, __data_len, etc. As >>>> mentioned, the transfer length WRITE SAME is typically 512 bytes and >>>> that's the number of bytes that need to be DMA'ed and transferred over >>>> the wire by the controller. But from a command completion perspective we >>>> need to complete however many bytes the command acted upon. Unlike reads >>>> and writes there is not a 1:1 mapping between the transfer length and >>>> the affected area. So we do a bit of magic after the buffer has been >>>> mapped to ensure that the completion byte count matches the number of >>>> blocks that were affected by the command rather than the size of the >>>> data buffer in bytes. >>>> >>>> -- >>>> Martin K. Petersen Oracle Linux Engineering >> -- >> Shaun Tancheff -- Shaun Tancheff