| Message ID | 20200611162655.4538-2-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nbd: reduce max_block restrictions | expand |
On 6/11/20 11:26 AM, Vladimir Sementsov-Ogievskiy wrote: > The NBD spec was recently updated to clarify that max_block doesn't > relate to NBD_CMD_WRITE_ZEROES with NBD_CMD_FLAG_FAST_ZERO (which > mirrors Qemu flag BDRV_REQ_NO_FALLBACK). To drop the restriction we > need new max_pwrite_zeroes_fast. > > Default value of new max_pwrite_zeroes_fast is zero and it means > use max_pwrite_zeroes. So this commit semantically changes nothing. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/block_int.h | 8 ++++++++ > block/io.c | 17 ++++++++++++----- > 2 files changed, 20 insertions(+), 5 deletions(-) Hmm, this is an optimization, rather than a correctness issue. I'm sorry I didn't review it sooner, but at this point, I think it is better as 5.2 material. > > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 791de6a59c..277e32fe31 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -626,6 +626,14 @@ typedef struct BlockLimits { > * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */ > int32_t max_pwrite_zeroes; > > + /* > + * Maximum number of bytes that can zeroed at once if flag > + * BDRV_REQ_NO_FALLBACK specified. Must be multiple of > + * pwrite_zeroes_alignment. > + * If 0, max_pwrite_zeroes is used for no-fallback case. > + */ > + int64_t max_pwrite_zeroes_fast; Nice that this is 64-bit off the bat (I know you have another series about converting more stuff to 64-bit). > + > /* Optimal alignment for write zeroes requests in bytes. A power > * of 2 is best but not mandatory. Must be a multiple of > * bl.request_alignment, and must be less than max_pwrite_zeroes > diff --git a/block/io.c b/block/io.c > index df8f2a98d4..0af62a53fd 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -1774,12 +1774,13 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, > bool need_flush = false; > int head = 0; > int tail = 0; > - > - int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX); > + int max_write_zeroes; 32-bit... > int alignment = MAX(bs->bl.pwrite_zeroes_alignment, > bs->bl.request_alignment); > int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER); > > + assert(alignment % bs->bl.request_alignment == 0); Would this look any better using the QEMU_IS_ALIGNED macro? > + > if (!drv) { > return -ENOMEDIUM; > } > @@ -1788,12 +1789,18 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, > return -ENOTSUP; > } > > - assert(alignment % bs->bl.request_alignment == 0); > - head = offset % alignment; > - tail = (offset + bytes) % alignment; > + if ((flags & BDRV_REQ_NO_FALLBACK) && bs->bl.max_pwrite_zeroes_fast) { > + max_write_zeroes = bs->bl.max_pwrite_zeroes_fast; ...but you try to assign something that may be 64-bit into it. Risk of overflow. Maybe we should get your 64-bit cleanup series in first. > + } else { > + max_write_zeroes = bs->bl.max_pwrite_zeroes; > + } > + max_write_zeroes = MIN_NON_ZERO(max_write_zeroes, INT_MAX); > max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment); > assert(max_write_zeroes >= bs->bl.request_alignment); > > + head = offset % alignment; > + tail = (offset + bytes) % alignment; > + > while (bytes > 0 && !ret) { > int num = bytes; > >
diff --git a/include/block/block_int.h b/include/block/block_int.h index 791de6a59c..277e32fe31 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -626,6 +626,14 @@ typedef struct BlockLimits { * pwrite_zeroes_alignment. May be 0 if no inherent 32-bit limit */ int32_t max_pwrite_zeroes; + /* + * Maximum number of bytes that can zeroed at once if flag + * BDRV_REQ_NO_FALLBACK specified. Must be multiple of + * pwrite_zeroes_alignment. + * If 0, max_pwrite_zeroes is used for no-fallback case. + */ + int64_t max_pwrite_zeroes_fast; + /* Optimal alignment for write zeroes requests in bytes. A power * of 2 is best but not mandatory. Must be a multiple of * bl.request_alignment, and must be less than max_pwrite_zeroes diff --git a/block/io.c b/block/io.c index df8f2a98d4..0af62a53fd 100644 --- a/block/io.c +++ b/block/io.c @@ -1774,12 +1774,13 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, bool need_flush = false; int head = 0; int tail = 0; - - int max_write_zeroes = MIN_NON_ZERO(bs->bl.max_pwrite_zeroes, INT_MAX); + int max_write_zeroes; int alignment = MAX(bs->bl.pwrite_zeroes_alignment, bs->bl.request_alignment); int max_transfer = MIN_NON_ZERO(bs->bl.max_transfer, MAX_BOUNCE_BUFFER); + assert(alignment % bs->bl.request_alignment == 0); + if (!drv) { return -ENOMEDIUM; } @@ -1788,12 +1789,18 @@ static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs, return -ENOTSUP; } - assert(alignment % bs->bl.request_alignment == 0); - head = offset % alignment; - tail = (offset + bytes) % alignment; + if ((flags & BDRV_REQ_NO_FALLBACK) && bs->bl.max_pwrite_zeroes_fast) { + max_write_zeroes = bs->bl.max_pwrite_zeroes_fast; + } else { + max_write_zeroes = bs->bl.max_pwrite_zeroes; + } + max_write_zeroes = MIN_NON_ZERO(max_write_zeroes, INT_MAX); max_write_zeroes = QEMU_ALIGN_DOWN(max_write_zeroes, alignment); assert(max_write_zeroes >= bs->bl.request_alignment); + head = offset % alignment; + tail = (offset + bytes) % alignment; + while (bytes > 0 && !ret) { int num = bytes;
The NBD spec was recently updated to clarify that max_block doesn't relate to NBD_CMD_WRITE_ZEROES with NBD_CMD_FLAG_FAST_ZERO (which mirrors Qemu flag BDRV_REQ_NO_FALLBACK). To drop the restriction we need new max_pwrite_zeroes_fast. Default value of new max_pwrite_zeroes_fast is zero and it means use max_pwrite_zeroes. So this commit semantically changes nothing. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/block_int.h | 8 ++++++++ block/io.c | 17 ++++++++++++----- 2 files changed, 20 insertions(+), 5 deletions(-)