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
