qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 5/6] block/nbd: Do not force-cap *pnum


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 5/6] block/nbd: Do not force-cap *pnum
Date: Tue, 22 Jun 2021 12:07:53 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

21.06.2021 21:53, Eric Blake wrote:
On Sat, Jun 19, 2021 at 01:53:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
+++ b/block/nbd.c
@@ -1702,7 +1702,7 @@ static int coroutine_fn nbd_client_co_block_status(
           .type = NBD_CMD_BLOCK_STATUS,
           .from = offset,
           .len = MIN(QEMU_ALIGN_DOWN(INT_MAX, bs->bl.request_alignment),
-                   MIN(bytes, s->info.size - offset)),
+                   s->info.size - offset),
           .flags = NBD_CMD_FLAG_REQ_ONE,
       };


Hmm..

I don't that this change is correct. In contrast with file-posix you don't get 
extra information for free, you just make a larger request. This means that 
server will have to do more work.

Not necessarily.  The fact that we have passed NBD_CMD_FLAG_REQ_ONE
means that the server is still only allowed to give us one extent in
its answer, and that it may not give us information beyond the length
we requested.  You are right that if we lose the REQ_ONE flag we may
result in the server doing more work to provide us additional extents
that we will then be ignoring because we aren't yet set up for
avoiding REQ_ONE.  Fixing that is a longer-term goal.  But in the
short term, I see no harm in giving a larger length to the server with
REQ_ONE.


(look at blockstatus_to_extents, it calls bdrv_block_status_above in a loop).

For example, assume that nbd export is a qcow2 image with all clusters 
allocated. With this change, nbd server will loop through the whole qcow2 
image, load all L2 tables to return big allocated extent.

No, the server is allowed to reply with less length than our request,
and that is particularly true if the server does NOT have free access
to the full length of our request.  In the case of qcow2, since
bdrv_block_status is (by current design) clamped at cluster
boundaries, requesting a 4G length will NOT increase the amount of the
server response any further than the first cluster boundary (that is,
the point where the server no longer has free access to status without
loading another cluster of L2 entries).

No. No matter where bdrv_block_status_above is clamped. If the whole disk is 
allocated, blockstatus_to_extents() in nbd/server.c will loop through the whole 
requested range and merge all the information into one extent. This doesn't 
violate NBD_CMD_FLAG_REQ_ONE: we have one extent on output and don't go beyound 
the length. It's valid for the server to try to satisfy as much as possible of 
request, and blockstatus_to_extents works in this way currently.

Remember that nbd_extent_array_add() can merge new extent to the previous if it 
has the same type.



So, only server can decide, could it add some extra free information to request 
or not. But unfortunately NBD_CMD_FLAG_REQ_ONE doesn't allow it.

What the flag prohibits is the server giving us more information than
the length we requested.  But this patch is increasing our request
length for the case where the server CAN give us more information than
we need locally, on the hopes that even though the server can only
reply with one extent, we aren't wasting as many network
back-and-forth trips when a larger request would have worked.



--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]