[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH] block: split large discard requests from block
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH] block: split large discard requests from block frontend |
Date: |
Fri, 1 Apr 2016 19:19:14 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 |
On 01.04.2016 14:22, Olaf Hering wrote:
> Large discard requests lead to sign expansion errors in qemu.
> Since there is no API to tell a guest about the limitations qmeu
> has to split a large request itself.
>
> Signed-off-by: Olaf Hering <address@hidden>
> Cc: Stefan Hajnoczi <address@hidden>
> Cc: Kevin Wolf <address@hidden>
> ---
> block/io.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
Hi!
Let me first nag about some style problems, and then I'll come to the
technical/design aspect. :-)
> diff --git a/block/io.c b/block/io.c
> index c4869b9..5b6ed58 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -2442,7 +2442,7 @@ static void coroutine_fn bdrv_discard_co_entry(void
> *opaque)
> rwco->ret = bdrv_co_discard(rwco->bs, rwco->sector_num,
> rwco->nb_sectors);
> }
>
> -int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
> +static int __bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
Two things: First, I think we do not like identifiers to start with
underscores, especially not with two underscores or with underscore and
a capital letter, because in C those combinations are generally reserved
for compiler/language extensions or for the operating system (think of
__attribute__(), __asm__(), or _Bool).
Second, the coroutine_fn should stay. This is just an empty macro (I
think) that signifies that a certain function is run inside of a
coroutine. That fact will not change if you put a wrapper around it.
> int nb_sectors)
This should be aligned to the opening paranthesis of the line above.
> {
> BdrvTrackedRequest req;
> @@ -2524,6 +2524,26 @@ out:
> return ret;
> }
>
> +int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num,
> + int nb_sectors)
> +{
> + int num, ret;
> + int limit = BDRV_REQUEST_MAX_SECTORS;
> + int remaining = nb_sectors;
> + int64_t sector_offset = sector_num;
> +
> + do {
> + num = remaining > limit ? limit : remaining;
num = MIN(limit, remaining); would work just as fine and maybe look a
bit better.
> + ret = __bdrv_co_discard(bs, sector_offset, num);
> + if (ret < 0)
> + break;
In qemu, every block is enclosed by {} braces, even if it contains only
a single statement.
This is something that the checkpatch script (scripts/checkpatch.pl in
the qemu tree) can detect. It is advisible to run that scripts on
patches before they are sent to the mailing list.
> + remaining -= num;
> + sector_offset += num;
> + } while (remaining > 0);
> +
> + return ret;
> +}
> +
> int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors)
> {
> Coroutine *co;
>
So, now about the technical aspect:
Guest requests are not issued to BlockDriverStates directly, they pass
through a BlockBackend first. The check whether the request is too large
is done there (in blk_check_request() called by blk_co_discard() and
blk_aio_discard()).
Thus, if a discard request exceeds BDRV_REQUEST_MAX_SECTORS, it will be
denied at the BlockBackend level and not reach bdrv_co_discard() at all.
At least that is how it's supposed to be. If it isn't, that's a bug. ;-)
Finally, I'm not sure whether this is something that should be done in
the block layer. I personally feel like it is the device models'
responsibility to only submit requests that adhere to the limits
established by the block layer.
In any case, do you have a test case where a guest was able to submit a
request that led to the overflow error you described in the commit message?
Max
signature.asc
Description: OpenPGP digital signature