| Message ID | 20200516035434.82809-4-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block layer change necessary for bcache zoned device support | expand |
On Sat, May 16, 2020 at 11:54:33AM +0800, Coly Li wrote: > The bcache driver is bio based and NOT request based multiqueued driver, > if a zoned SMR hard drive is used as backing device of a bcache device, > calling blk_revalidate_disk_zones() for the bcache device will fail due > to the following check in blk_revalidate_disk_zones(), > 478 if (WARN_ON_ONCE(!queue_is_mq(q))) > 479 return -EIO; > > Now bcache is able to export the zoned information from the underlying > zoned SMR drives and format zonefs on top of a bcache device, the > resitriction that a zoned device should be multiqueued is unnecessary > for now. > > Although in commit ae58954d8734c ("block: don't handle bio based drivers > in blk_revalidate_disk_zones") it is said that bio based drivers should > not call blk_revalidate_disk_zones() and just manually update their own > q->nr_zones, but this is inaccurate. The bio based drivers also need to > set their zone size and initialize bitmaps for cnv and seq zones, it is > necessary to call blk_revalidate_disk_zones() for bio based drivers. Why would you need these bitmaps for bcache? There is no reason to serialize requests for stacking drivers, and you can already derive if a zone is sequential or not from whatever internal information you use. So without a user that actually makes sense: NAK.
On 2020/5/16 20:40, Christoph Hellwig wrote: > On Sat, May 16, 2020 at 11:54:33AM +0800, Coly Li wrote: >> The bcache driver is bio based and NOT request based multiqueued driver, >> if a zoned SMR hard drive is used as backing device of a bcache device, >> calling blk_revalidate_disk_zones() for the bcache device will fail due >> to the following check in blk_revalidate_disk_zones(), >> 478 if (WARN_ON_ONCE(!queue_is_mq(q))) >> 479 return -EIO; >> >> Now bcache is able to export the zoned information from the underlying >> zoned SMR drives and format zonefs on top of a bcache device, the >> resitriction that a zoned device should be multiqueued is unnecessary >> for now. >> >> Although in commit ae58954d8734c ("block: don't handle bio based drivers >> in blk_revalidate_disk_zones") it is said that bio based drivers should >> not call blk_revalidate_disk_zones() and just manually update their own >> q->nr_zones, but this is inaccurate. The bio based drivers also need to >> set their zone size and initialize bitmaps for cnv and seq zones, it is >> necessary to call blk_revalidate_disk_zones() for bio based drivers. > > Why would you need these bitmaps for bcache? There is no reason to > serialize requests for stacking drivers, and you can already derive > if a zone is sequential or not from whatever internal information > you use. > > So without a user that actually makes sense: NAK. > It is OK for me to set the zone_nr and zone size without calling blk_revalidate_disk_zones(). Coly Li
On Sat, May 16, 2020 at 09:13:50PM +0800, Coly Li wrote: > It is OK for me to set the zone_nr and zone size without calling > blk_revalidate_disk_zones(). Yes. And without having seen your code I'm pretty sure it should get simpler by not using blk_revalidate_disk_zones.
On 2020/05/16 12:55, Coly Li wrote: > The bcache driver is bio based and NOT request based multiqueued driver, > if a zoned SMR hard drive is used as backing device of a bcache device, > calling blk_revalidate_disk_zones() for the bcache device will fail due > to the following check in blk_revalidate_disk_zones(), > 478 if (WARN_ON_ONCE(!queue_is_mq(q))) > 479 return -EIO; > > Now bcache is able to export the zoned information from the underlying > zoned SMR drives and format zonefs on top of a bcache device, the > resitriction that a zoned device should be multiqueued is unnecessary > for now. > > Although in commit ae58954d8734c ("block: don't handle bio based drivers > in blk_revalidate_disk_zones") it is said that bio based drivers should > not call blk_revalidate_disk_zones() and just manually update their own > q->nr_zones, but this is inaccurate. The bio based drivers also need to > set their zone size and initialize bitmaps for cnv and seq zones, it is > necessary to call blk_revalidate_disk_zones() for bio based drivers. > > This patch removes the above queue_is_mq() restriction to permit > bcache driver calls blk_revalidate_disk_zones() for bcache device zoned > information initialization. > > Fixes: ae58954d8734c ("block: don't handle bio based drivers in blk_revalidate_disk_zones") > Signed-off-by: Coly Li <colyli@suse.de> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Damien Le Moal <damien.lemoal@wdc.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Jens Axboe <axboe@kernel.dk> > Cc: Keith Busch <kbusch@kernel.org> > --- > block/blk-zoned.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/block/blk-zoned.c b/block/blk-zoned.c > index f87956e0dcaf..1e0708c68267 100644 > --- a/block/blk-zoned.c > +++ b/block/blk-zoned.c > @@ -475,8 +475,6 @@ int blk_revalidate_disk_zones(struct gendisk *disk) > > if (WARN_ON_ONCE(!blk_queue_is_zoned(q))) > return -EIO; > - if (WARN_ON_ONCE(!queue_is_mq(q))) > - return -EIO; Same comment as Christoph: BIO drivers can set nr_zones and chunk_sectors themselves, manually. They also do not need the bitmaps as BIO devices do not have an elevator, and as Christoph said, can (if needed) look at the bitmaps of the underlying device (e.g. for zone conv vs seq zone type differentiation). So I do not think this patch is needed, and would only cause more memory consumption for no good reason that I can see. > > /* > * Ensure that all memory allocations in this context are done as if >
On 2020/05/16 22:14, Coly Li wrote: > On 2020/5/16 20:40, Christoph Hellwig wrote: >> On Sat, May 16, 2020 at 11:54:33AM +0800, Coly Li wrote: >>> The bcache driver is bio based and NOT request based multiqueued driver, >>> if a zoned SMR hard drive is used as backing device of a bcache device, >>> calling blk_revalidate_disk_zones() for the bcache device will fail due >>> to the following check in blk_revalidate_disk_zones(), >>> 478 if (WARN_ON_ONCE(!queue_is_mq(q))) >>> 479 return -EIO; >>> >>> Now bcache is able to export the zoned information from the underlying >>> zoned SMR drives and format zonefs on top of a bcache device, the >>> resitriction that a zoned device should be multiqueued is unnecessary >>> for now. >>> >>> Although in commit ae58954d8734c ("block: don't handle bio based drivers >>> in blk_revalidate_disk_zones") it is said that bio based drivers should >>> not call blk_revalidate_disk_zones() and just manually update their own >>> q->nr_zones, but this is inaccurate. The bio based drivers also need to >>> set their zone size and initialize bitmaps for cnv and seq zones, it is >>> necessary to call blk_revalidate_disk_zones() for bio based drivers. >> >> Why would you need these bitmaps for bcache? There is no reason to >> serialize requests for stacking drivers, and you can already derive >> if a zone is sequential or not from whatever internal information >> you use. >> >> So without a user that actually makes sense: NAK. >> > > It is OK for me to set the zone_nr and zone size without calling > blk_revalidate_disk_zones(). Yes, no problem with that. This is how device mapper BIO based targets handle zoned devices. See dm_set_device_limits() which uses bdev_stack_limits() for handling chunk_sectors (zone size) and directly set q->limits.zoned to blk_queue_zoned_model(q) of the the underlying device. For the number of zones, see dm_table_set_restrictions() which uses blkdev_nr_zones(t->md->disk) to set q->nr_zones. Note that blkdev_nr_zones() uses the gendisk capacity of the logical device (the bcache device in your case) and the zone size, so both must be set first before calling blkdev_nr_zones(). > > Coly Li >
diff --git a/block/blk-zoned.c b/block/blk-zoned.c index f87956e0dcaf..1e0708c68267 100644 --- a/block/blk-zoned.c +++ b/block/blk-zoned.c @@ -475,8 +475,6 @@ int blk_revalidate_disk_zones(struct gendisk *disk) if (WARN_ON_ONCE(!blk_queue_is_zoned(q))) return -EIO; - if (WARN_ON_ONCE(!queue_is_mq(q))) - return -EIO; /* * Ensure that all memory allocations in this context are done as if
The bcache driver is bio based and NOT request based multiqueued driver, if a zoned SMR hard drive is used as backing device of a bcache device, calling blk_revalidate_disk_zones() for the bcache device will fail due to the following check in blk_revalidate_disk_zones(), 478 if (WARN_ON_ONCE(!queue_is_mq(q))) 479 return -EIO; Now bcache is able to export the zoned information from the underlying zoned SMR drives and format zonefs on top of a bcache device, the resitriction that a zoned device should be multiqueued is unnecessary for now. Although in commit ae58954d8734c ("block: don't handle bio based drivers in blk_revalidate_disk_zones") it is said that bio based drivers should not call blk_revalidate_disk_zones() and just manually update their own q->nr_zones, but this is inaccurate. The bio based drivers also need to set their zone size and initialize bitmaps for cnv and seq zones, it is necessary to call blk_revalidate_disk_zones() for bio based drivers. This patch removes the above queue_is_mq() restriction to permit bcache driver calls blk_revalidate_disk_zones() for bcache device zoned information initialization. Fixes: ae58954d8734c ("block: don't handle bio based drivers in blk_revalidate_disk_zones") Signed-off-by: Coly Li <colyli@suse.de> Cc: Christoph Hellwig <hch@lst.de> Cc: Damien Le Moal <damien.lemoal@wdc.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Jens Axboe <axboe@kernel.dk> Cc: Keith Busch <kbusch@kernel.org> --- block/blk-zoned.c | 2 -- 1 file changed, 2 deletions(-)