From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 12/23] sd: handle REQ_UNMAP Date: Wed, 29 Mar 2017 16:57:39 +0200 Message-ID: References: <20170323143341.31549-1-hch@lst.de> <20170323143341.31549-13-hch@lst.de> <1490719722.2573.8.camel@sandisk.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1490719722.2573.8.camel@sandisk.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Bart Van Assche , Alasdair G Kergon , "lars.ellenberg@linbit.com" , Mike Snitzer , Christoph Hellwig , "Martin K. Petersen" , "philipp.reisner@linbit.com" , Jens Axboe , "shli@kernel.org" Cc: "linux-block@vger.kernel.org" , "linux-raid@vger.kernel.org" , dm , Linux SCSI List , "drbd-dev@lists.linbit.com" List-Id: linux-raid.ids On 28/03/2017 18:48, Bart Van Assche wrote: >> + if (rq->cmd_flags & REQ_UNMAP) { >> + switch (sdkp->provisioning_mode) { >> + case SD_LBP_WS16: >> + return sd_setup_write_same16_cmnd(cmd, true); >> + case SD_LBP_WS10: >> + return sd_setup_write_same10_cmnd(cmd, true); >> + } >> + } >> + >> if (sdp->no_write_same) >> return BLKPREP_INVALID; >> if (sdkp->ws16 || sector > 0xffffffff || nr_sectors > 0xffff) > Users can change the provisioning mode from user space from SD_LBP_WS16 into > SD_LBP_WS10 so I'm not sure it's safe to skip the (sdkp->ws16 || sector > > 0xffffffff || nr_sectors > 0xffff) check if REQ_UNMAP is set. Yeah, if REQ_UNMAP is set you should probably check sdkp->provisioning_mode instead of sdkp->ws16, but apart from this it should still go through the checks below. Plus, if the provisioning mode is not ws10 or ws16, should sd_setup_write_zeroes_cmnd: 1) do a WRITE SAME without UNMAP (what Christoph's code does) 2) return BLKPREP_INVALID 3) ignore provisioning mode and do a WRITE SAME with UNMAP 4) do a WRITE SAME without UNMAP for SD_LBP_{ZERO,FULL,DISABLE}, do a WRITE SAME with UNMAP for SD_LBP_{WS10,WS16,UNMAP}. I'm in favor of (4). The distinction between SD_LBP_UNMAP, SD_LBP_WS10 and SD_LBP_WS16 is as problematic as discard_zeroes_data in my opinion. Thanks, Paolo