On Mon, Mar 24, 2025 at 6:16 PM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
>
> On Mon, Mar 24, 2025 at 03:26:06PM -0700, Caleb Sander Mateos wrote:
> > On Mon, Mar 24, 2025 at 6:49 AM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
> > >
> > > IO split is usually bad in io_uring world, since -EAGAIN is caused and
> > > IO handling may have to fallback to io-wq, this way does hurt performance.
> > >
> > > ublk starts to support zero copy recently, for avoiding unnecessary IO
> > > split, ublk driver's segment limit should be aligned with backend
> > > device's segment limit.
> > >
> > > Another reason is that io_buffer_register_bvec() needs to allocate bvecs,
> > > which number is aligned with ublk request segment number, so that big
> > > memory allocation can be avoided by setting reasonable max_segments limit.
> > >
> > > So add segment parameter for providing ublk server chance to align
> > > segment limit with backend, and keep it reasonable from implementation
> > > viewpoint.
> > >
> > > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > > ---
> > > drivers/block/ublk_drv.c | 15 ++++++++++++++-
> > > include/uapi/linux/ublk_cmd.h | 9 +++++++++
> > > 2 files changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index acb6aed7be75..53a463681a41 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -74,7 +74,7 @@
> > > #define UBLK_PARAM_TYPE_ALL \
> > > (UBLK_PARAM_TYPE_BASIC | UBLK_PARAM_TYPE_DISCARD | \
> > > UBLK_PARAM_TYPE_DEVT | UBLK_PARAM_TYPE_ZONED | \
> > > - UBLK_PARAM_TYPE_DMA_ALIGN)
> > > + UBLK_PARAM_TYPE_DMA_ALIGN | UBLK_PARAM_TYPE_SEGMENT)
> > >
> > > struct ublk_rq_data {
> > > struct kref ref;
> > > @@ -580,6 +580,13 @@ static int ublk_validate_params(const struct ublk_device *ub)
> > > return -EINVAL;
> > > }
> > >
> > > + if (ub->params.types & UBLK_PARAM_TYPE_SEGMENT) {
> > > + const struct ublk_param_segment *p = &ub->params.seg;
> > > +
> > > + if (!is_power_of_2(p->seg_boundary_mask + 1))
> > > + return -EINVAL;
> >
> > Looking at blk_validate_limits(), it seems like there are some
> > additional requirements? Looks like seg_boundary_mask has to be at
> > least PAGE_SIZE - 1
>
> Yeah, it isn't done in ublk because block layer runs the check, and it
> will be failed when starting the device. That said we take block layer's
> default setting, which isn't good from UAPI viewpoint, since block
> layer may change the default setting.
Even though blk_validate_limits() rejects it, it appears to log a
warning. That seems undesirable for something controllable from
userspace.
/*
* By default there is no limit on the segment boundary alignment,
* but if there is one it can't be smaller than the page size as
* that would break all the normal I/O patterns.
*/
if (!lim->seg_boundary_mask)
lim->seg_boundary_mask = BLK_SEG_BOUNDARY_MASK;
if (WARN_ON_ONCE(lim->seg_boundary_mask < BLK_MIN_SEGMENT_SIZE - 1))
return -EINVAL;
>
> Also it is bad to associate device property with PAGE_SIZE which is
> a variable actually. The latest kernel has replaced PAGE_SIZE with 4096
> for segment limits.
>
> I think we can take 4096 for validation here.
>
> > and max_segment_size has to be at least PAGE_SIZE
> > if virt_boundary_mask is set?
>
> If virt_boundary_mask is set, max_segment_size will be ignored usually
> except for some stacking devices.
Sorry, I had it backwards. The requirement is if virt_boundary_mask is
*not* set:
/*
* Stacking device may have both virtual boundary and max segment
* size limit, so allow this setting now, and long-term the two
* might need to move out of stacking limits since we have immutable
* bvec and lower layer bio splitting is supposed to handle the two
* correctly.
*/
if (lim->virt_boundary_mask) {
if (!lim->max_segment_size)
lim->max_segment_size = UINT_MAX;
} else {
/*
* The maximum segment size has an odd historic 64k default that
* drivers probably should override. Just like the I/O size we
* require drivers to at least handle a full page per segment.
*/
if (!lim->max_segment_size)
lim->max_segment_size = BLK_MAX_SEGMENT_SIZE;
if (WARN_ON_ONCE(lim->max_segment_size < BLK_MIN_SEGMENT_SIZE))
return -EINVAL;
}
Best,
Caleb
