On Tue, Jan 23, 2024 at 01:50:32PM +0900, Damien Le Moal wrote:
> > + return -EINVAL;
> > + return 0;
> > + }
> > +
> > + if (WARN_ON_ONCE(!IS_ENABLED(CONFIG_BLK_DEV_ZONED)))
> > + return -EINVAL;
> > +
> > + if (lim->zone_write_granularity < lim->logical_block_size)
> > + lim->zone_write_granularity = lim->logical_block_size;
>
> This check and change needs to be against the physical block size. Otherwise,
> SMR drives will choke on it.
It probably should, but this mirrors what is done in
blk_queue_zone_write_granularity. And I want to match that behavior
at least for now, we can then improve and document it once this is
the only interface to validate the limits.
> > +
> > + if (lim->max_zone_append_sectors) {
> > + if (WARN_ON_ONCE(!lim->chunk_sectors))
> > + return -EINVAL;
>
> chunk_sectors is the zone size. So we should probably check that it is set after
> the IS_ENABLED() check earlier in the function, no ?
Possibly. In fact I'm wondering where the check comes from, as we
don't seem to have it in the existing helpers.
> > + if (!lim->logical_block_size)
> > + lim->logical_block_size = SECTOR_SIZE;
> > +
> > + if (!lim->physical_block_size)
> > + lim->physical_block_size = SECTOR_SIZE;
> > + if (lim->physical_block_size < lim->logical_block_size)
> > + lim->physical_block_size = lim->physical_block_size;
> > +
> > + if (!lim->io_min)
> > + lim->io_min = SECTOR_SIZE;
>
> This should be lim->logical_block_size, no ?
This comes from the default in blk_set_default_limits.
>
> > + if (lim->io_min < lim->physical_block_size)
> > + lim->io_min = lim->physical_block_size;
>
> But then given that log <= phys, you could set io_min to physical_block_size if
> it is not set.
Which is what we do here, so the above is actually redundant and
can be removed.
> > + if (!lim->max_hw_sectors)
> > + lim->max_hw_sectors = BLK_SAFE_MAX_SECTORS;
> > + if (WARN_ON_ONCE(lim->max_hw_sectors < PAGE_SIZE / SECTOR_SIZE))
>
> You can use PAGE_SECTORS here.
Yes.
