Re: [RFC PATCH v2 1/4] block: change REQ_OP_ZONE_RESET from 6 to 13

From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Hannes Reinecke <hare@suse.de>, Coly Li <colyli@suse.de>,
	Christoph Hellwig <hch@lst.de>
Cc: "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"hare@suse.com" <hare@suse.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"linux-bcache@vger.kernel.org" <linux-bcache@vger.kernel.org>,
	"kbusch@kernel.org" <kbusch@kernel.org>,
	Jens Axboe <axboe@fb.com>,
	Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	Shaun Tancheff <shaun.tancheff@seagate.com>
Subject: Re: [RFC PATCH v2 1/4] block: change REQ_OP_ZONE_RESET from 6 to 13
Date: Mon, 18 May 2020 06:56:36 +0000	[thread overview]
Message-ID: <BY5PR04MB6900419B7FBB5913904B3AF1E7B80@BY5PR04MB6900.namprd04.prod.outlook.com> (raw)
In-Reply-To: 19eea726-590c-bf55-2dbf-35b71c9d1600@suse.de

On 2020/05/18 15:53, Hannes Reinecke wrote:
> On 5/17/20 7:30 AM, Coly Li wrote:
>> On 2020/5/16 23:36, Christoph Hellwig wrote:
>>> On Sat, May 16, 2020 at 09:05:39PM +0800, Coly Li wrote:
>>>> On 2020/5/16 20:50, Christoph Hellwig wrote:
>>>>> On Sat, May 16, 2020 at 08:44:45PM +0800, Coly Li wrote:
>>>>>> Yes you are right, just like REQ_OP_DISCARD which does not transfer any
>>>>>> data but changes the data on device. If the request changes the stored
>>>>>> data, it does transfer data.
>>>>>
>>>>> REQ_OP_DISCARD is a special case, because most implementation end up
>>>>> transferring data, it just gets attached in the low-level driver.
>>>>>
>>>>
>>>> Yes, REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL are quite similar to
>>>> REQ_OP_DISCARD. Data read from the LBA range of reset zone is not
>>>> suggested and the content is undefined.
>>>>
>>>> For bcache, similar to REQ_OP_DISCARD, REQ_OP_ZONE_RESET and
>>>> REQ_OP_ZONE_RESET_ALL are handled in same way: If the backing device
>>>> supports discard/zone_reset, and the operation successes, then cached
>>>> data on SSD covered by the LBA range should be invalid, otherwise users
>>>> will read outdated and garbage data.
>>>>
>>>> We should treat REQ_OP_ZONE_RESET and REQ_OP_ZONE_RESET_ALL being in
>>>> WRITE direction.
>>>
>>> No, the difference is that the underlying SCSI/ATA/NVMe command tend to
>>> usually actually transfer data.  Take a look at the special_vec field
>>> in struct request and the RQF_SPECIAL_PAYLOAD flag.
>>>
>>
>> Then bio_data_dir() will be problematic, as it is defined,
>>   52 /*
>>   53  * Return the data direction, READ or WRITE.
>>   54  */
>>   55 #define bio_data_dir(bio) \
>>   56         (op_is_write(bio_op(bio)) ? WRITE : READ)
>>
>> For the REQ_OP_ZONE_RESET bio, bio_data_dir() will report it as READ.
>>
>> Since the author is you, how to fix this ?
>>
> 
> Well, but I do think that Coly is right in that bio_data_dir() is very 
> unconvincing, or at least improperly documented.
> 
> I can't quite follow Christoph's argument in that bio_data_dir() 
> reflects whether data is _actually_ transferred (if so, why would 
> REQ_OP_ZONE_CLOSE() be marked as WRITE?)
> However, in most cases bio_data_dir() will only come into effect if 
> there are attached bvecs.
> 
> So the _correct_ usage for bio_data_dir() would be to check if there is 
> data attached to the bio (eg by using bio_has_data()), and only call 
> bio_data_dir() if that returns true. For false you'd need to add a 
> switch evaluating the individual commands.
> 
> And, btw, bio_has_data() needs updating, too; all the zone commands are 
> missing from there, too.

Not really. They all have 0 size, so bio_has_data() always return false for zone
management ops. It will return true only for report zones.

> 
> Cheers,
> 
> Hannes
> 

-- 
Damien Le Moal
Western Digital Research