| Message ID | 20200403101250.33245-4-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Introduce Zone Append for writing to zoned block devices | expand |
So this new callback is exectured just before blk_revalidate_zone_cb returns and thus control is transferred back to the driver. What speaks against just implementing this logic after the callback returns? ->report_zones is not just called for validation, but does that matter? If yes we can pass a flag, which still seems a bit better than a code flow with multiple callbacks.
On Tue, Apr 07, 2020 at 09:53:50AM -0700, Christoph Hellwig wrote: > So this new callback is exectured just before blk_revalidate_zone_cb > returns and thus control is transferred back to the driver. What > speaks against just implementing this logic after the callback returns? > ->report_zones is not just called for validation, but does that matter? > If yes we can pass a flag, which still seems a bit better than a > code flow with multiple callbacks. Sorry, that reply should have been for the next patch.
On 2020/04/08 1:53, Christoph Hellwig wrote: > So this new callback is exectured just before blk_revalidate_zone_cb > returns and thus control is transferred back to the driver. What > speaks against just implementing this logic after the callback returns? > ->report_zones is not just called for validation, but does that matter? > If yes we can pass a flag, which still seems a bit better than a > code flow with multiple callbacks. > (Changed the subject to point to the correct patch) Yes. Indeed we can do that. A flag will keep the interface of the report_zones method simpler. But the second call to the revalidate callback done after the zone report is done is still needed so that the wp_ofst array can be updated under the queue freeze and avoid races with number of zones, bitmaps etc updates. I have not found any good way to avoid that one if we want to preserve blk_revalidate_disk_zones() as a generic helper. Of course if we reimplement this in sd_zbc, things are simple, but then we will have a lot of code repeated.
On Wed, Apr 08, 2020 at 08:29:13AM +0000, Damien Le Moal wrote: > (Changed the subject to point to the correct patch) > > Yes. Indeed we can do that. A flag will keep the interface of the report_zones > method simpler. > > But the second call to the revalidate callback done after the zone report is > done is still needed so that the wp_ofst array can be updated under the queue > freeze and avoid races with number of zones, bitmaps etc updates. I have not > found any good way to avoid that one if we want to preserve > blk_revalidate_disk_zones() as a generic helper. Of course if we reimplement > this in sd_zbc, things are simple, but then we will have a lot of code repeated. True, I missed the second call. But given that it does something entirely different multiplexing it to the same interface does indeed seem rather odd. So I think we can pass a 'finish' or so callback to blk_revalidate_disk_zones. I'm not even sure we need the args or a flag. Ithink we can just make sure calls to blk_revalidate_disk_zones are synchronized and then have a pending_wps pointer int the scsi_disk structure. If that is set, sd_zbc_report_zones updates it, and then the finish callback puts it into the real place.
diff --git a/block/blk-zoned.c b/block/blk-zoned.c index 05741c6f618b..00b025b8b7c0 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -50,6 +50,20 @@ bool blk_req_needs_zone_write_lock(struct request *rq) } EXPORT_SYMBOL_GPL(blk_req_needs_zone_write_lock); +bool blk_req_zone_write_trylock(struct request *rq) +{ + unsigned int zno = blk_rq_zone_no(rq); + + if (test_and_set_bit(zno, rq->q->seq_zones_wlock)) + return false; + + WARN_ON_ONCE(rq->rq_flags & RQF_ZONE_WRITE_LOCKED); + rq->rq_flags |= RQF_ZONE_WRITE_LOCKED; + + return true; +} +EXPORT_SYMBOL_GPL(blk_req_zone_write_trylock); + void __blk_req_zone_write_lock(struct request *rq) { if (WARN_ON_ONCE(test_and_set_bit(blk_rq_zone_no(rq), diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 36111b10d514..e591b22ace03 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1746,6 +1746,7 @@ extern int bdev_write_page(struct block_device *, sector_t, struct page *, #ifdef CONFIG_BLK_DEV_ZONED bool blk_req_needs_zone_write_lock(struct request *rq); +bool blk_req_zone_write_trylock(struct request *rq); void __blk_req_zone_write_lock(struct request *rq); void __blk_req_zone_write_unlock(struct request *rq);