On Tue, Mar 25, 2025 at 12:43:26PM -0700, Caleb Sander Mateos wrote:
> 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;
Yes, it has been addressed in my local version, and we need to make it
a hw/sw interface.
>
> >
> > 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;
> }
Right.
Please feel free to see if the revised patch is good:
