On Mon, Feb 10, 2025 at 12:17:07PM -0800, Luis Chamberlain wrote: > On Mon, Feb 10, 2025 at 05:03:19PM +0800, Ming Lei wrote: > > PAGE_SIZE is applied in some block device queue limits, this way is > > very fragile and is wrong: > > > > - queue limits are read from hardware, which is often one readonly > > hardware property > > > > - PAGE_SIZE is one config option which can be changed during build time. > > This is true. > > > In RH lab, it has been found that max segment size of some mmc card is > > less than 64K, then this kind of card can't work in case of 64K PAGE_SIZE. > > This is true, but check the note on block/blk-merge.c blk_bvec_map_sg(). > It would seem that this is a limitation of MMC/SD and that this should > ideally be fixed. The mmc card works just fine in case of 4K page size, there isn't any limitation for the mmc/ssd from storage viewpoint, the failure is just because this card's max segment size is < 64KB in case of 64K page size. > > > Fix this issue by using BLK_MIN_SEGMENT_SIZE in related code for dealing > > with queue limits and checking if bio needn't split. Define BLK_MIN_SEGMENT_SIZE > > as 4K(minimized PAGE_SIZE). > > But indeed if the block driver isn't yet fixed, then sure, we have to > deal with the issue, I am not convinced that the logic below addresses > this in a generic way, rather it seems to conflate the areas where we > do need the generic block layer min defined, and when we have a block > min segment limit. > > > Cc: Yi Zhang <yi.zhang@xxxxxxxxxx> > > Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx> > > Cc: John Garry <john.g.garry@xxxxxxxxxx> > > Cc: Bart Van Assche <bvanassche@xxxxxxx> > > Cc: Keith Busch <kbusch@xxxxxxxxxx> > > Link: https://lore.kernel.org/linux-block/20250102015620.500754-1-ming.lei@xxxxxxxxxx/ > > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx> > > --- > > V2: > > - cover bio_split_rw_at() > > - add BLK_MIN_SEGMENT_SIZE > > > > block/blk-merge.c | 2 +- > > block/blk-settings.c | 6 +++--- > > block/blk.h | 2 +- > > include/linux/blkdev.h | 1 + > > 4 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/block/blk-merge.c b/block/blk-merge.c > > index 15cd231d560c..b55c52a42303 100644 > > --- a/block/blk-merge.c > > +++ b/block/blk-merge.c > > @@ -329,7 +329,7 @@ int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim, > > > > if (nsegs < lim->max_segments && > > bytes + bv.bv_len <= max_bytes && > > - bv.bv_offset + bv.bv_len <= PAGE_SIZE) { > > + bv.bv_offset + bv.bv_len <= BLK_MIN_SEGMENT_SIZE) { > > nsegs++; > > bytes += bv.bv_len; > > I'll note that the 64k BLK_MAX_SEGMENT_SIZE is an old "odd historic" default > value, ie, not a documented hard limit but some odd old thing which > blk_validate_limits() encourages block drivers to override, so a soft > max. BLK_MAX_SEGMENT_SIZE is default or fallback max segment size if the hardware doesn't provide this limit, so nothing odd here because block layer has to use something reasonable here. > > That said, if we validate this soft max and if you also validate the min There isn't soft max segment size. > shouldn't value in the above instead be lim->max_segment_size instead, min segment size is page_size and it is soft, and has been applied for long time. This patch just fixes it as 4k(min(page_size)). > provided that we also address the coment in blk_bvec_map_sg()? The comment in blk_bvec_map_sg() has been removed, and blk_bvec_map_sg has been re-written in commit b7175e24d6ac ("block: add a dma mapping iterator") by following segment limits only. > > More forward looking -- are you using BLK_MIN_SEGMENT_SIZE here due to > the same mmc/sd limitations ? Can we overcome the mmc/sd limitations by > only using this BLK_MIN_SEGMENT_SIZE only on block drivers which have the > scatterlists limitation? Please see my comment above, the mmc card doesn't have any limitation, it is just that its max segment size is < 64K, which is absolutely allowed from storage viewpoint. Thanks, Ming
