Re: [RFC 0/3] 64bit block-layer part I

[Kevin Wolf <kwolf@redhat.com>]

Am 30.03.2020 um 16:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> There is an idea to make NBD protocol extension to support 64bit
> write-zero/discard/block-status commands (commands, which doesn't
> transfer user data). It's needed to increase performance of zeroing
> large ranges (up to the whole image). Zeroing of the whole image is used
> as first step of mirror job, qemu-img convert, it should be also used at
> start of backup actually..
> 
> We need to support it in block-layer, so we want 64bit write_zeros.
> Currently driver handler now have int bytes parameter.
> 
> write_zeros path goes through normal pwritev, so we need 64bit write,
> and then we need 64bit read for symmetry, and better, let's make all io
> path work with 64bit bytes parameter.
> 
> Actually most of block-layer already have 64bit parameters: offset is
> sometimes int64_t and sometimes uint64_t. bytes parameter is one of
> int64_t, uint64_t, int, unsigned int...
> 
> I think we need one type for all of this, and this one type is int64_t.
> Signed int64_t is a bit better than uint64_t: you can use same variable
> to get some result (including error < 0) and than reuse it as an
> argument without any type conversion.
> 
> So, I propose, as a first step, convert all uint64_t parameters to
> int64_t.
> 
> Still, I don't have good idea of how to split this into more than 3
> patches, so, this is an RFC.

I think the split in three patches isn't too bad because it's not a
whole lot of code. But of course, it is little code that has lots of
implications which does make it hard to review. If we think that we
might bisect a bug in the series later, maybe it would be better to
split it into more patches.

write/write_zeroes has to be a single thing, I'm afraid. But I guess
read could be a separate patch, as could be copy_range. Not sure about
discard.

> What's next?
> 
> Converting write_zero and discard is not as simple: we can't just
> s/int/uint64_t/, as some functions may use some int variables for
> calculations and this will be broken by something larger than int.
> 
> So, I think the simplest way is to add .bdrv_co_pwritev_zeros64 and
> .bdrv_co_pdiscard64 and update drivers one-by-one. If at some point all
> drivers updated - drop unused 32bit functions, and then drop "64" suffix
> from API. If not - we'll live with both APIs.

We already have too many unfinished conversions in QEMU, let's not add
one more.

Fortunately, we already have a tool that could help us here: Things like
bs->bl.max_pwrite_zeroes. We could make BDRV_REQUEST_MAX_BYTES the
default value and only drivers that override it can get bigger requests.

> Another thing to do is updating default limiting of request (currently
> they are limited to INT_MAX).

As above, I wouldn't update the default, but rather enable drivers to
overload the default with a larger value. This will involve changing
some places where we use MIN() between INT_MAX and the driver's value.

> Then we may move some drivers to 64bit discard/write_zero: I think about
> qcow2, file-posix and nbd (as a proof-of-concept for already proposed
> NBD extension).

Makes sense to me.

Kevin