On 9/28/22 08:07, Damien Le Moal wrote: > On 9/28/22 01:52, Jens Axboe wrote: >> On 9/27/22 10:51 AM, Christoph Hellwig wrote: >>> On Tue, Sep 27, 2022 at 10:04:19AM -0600, Jens Axboe wrote: >>>> Ah yes, good point. We used to have this notion of 'fs' request, don't >>>> think we do anymore. Because it really should just be: >>> >>> A fs request is a !passthrough request. >> >> Right, that's the condition I made below too. >> >>>> if (zoned && (op & REQ_OP_WRITE) && fs_request) >>>> return NULL; >>>> >>>> for that condition imho. I guess we could make it: >>>> >>>> if (zoned && (op & REQ_OP_WRITE) && !(op & REQ_OP_DRV_OUT)) >>>> return NULL; >>> >>> Well, the only opcodes we do zone locking for is REQ_OP_WRITE and >>> REQ_OP_WRITE_ZEROES. So this should be: >>> >>> if (zoned && (op == REQ_OP_WRITE || op == REQ_OP_WRITE_ZEROES)) >>> return NULL; >> >> I'd rather just make it explicit and use that. Pankaj, do you want >> to spin a v2 with that? > > It would be nice to reuse the bio equivalent of > blk_req_needs_zone_write_lock(). > > The test would be: > > if (bio_needs_zone_write_locking()) > return NULL; Note that we could also add a "IS_ENABLED(CONFIG_BLK_DEV_ZONED) &&" to the condition or stub the helper to have this hunk disappear for the !CONFIG_BLK_DEV_ZONED case. > > With something like: > > static inline bool bio_needs_zone_write_locking() > { > if (!bdev_is_zoned(bio->bi_bdev)) > return false; > > switch (bio_op(bio)) { > case REQ_OP_WRITE_ZEROES: > > case REQ_OP_WRITE: > > return true; > default: > > return false; > > } > } > > Which also has the advantage that going forward, we could refine this to > plug writes to conventional zones (as these can be plugged). > -- Damien Le Moal Western Digital Research