Re: [PATCH V2] block: make segment size limit workable for > 4K PAGE_SIZE

[Ming Lei <ming.lei@redhat.com>]

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@redhat.com>
> > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > Cc: John Garry <john.g.garry@oracle.com>
> > Cc: Bart Van Assche <bvanassche@acm.org>
> > Cc: Keith Busch <kbusch@kernel.org>
> > Link: https://lore.kernel.org/linux-block/20250102015620.500754-1-ming.lei@redhat.com/
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> > 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