Re: [Qemu-devel] [PATCH 2/7] nbd/server: Trace server noncompliance on unaligned requests

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

05.04.2019 23:04, Eric Blake wrote:
> On 4/5/19 9:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 03.04.2019 6:05, Eric Blake wrote:
>>> We've recently added traces for clients to flag server non-compliance;
>>> let's do the same for servers to flag client non-compliance. According
> 
> Thus, s/Trace server/Trace client/ in the subject line.
> 
>>>
>>> Qemu as client used to have one spot where it sent non-compliant
>>> requests: if the server sends an unaligned reply to
>>> NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire
>>> disk, the next request would start at that unaligned point; this was
>>> fixed in commit a39286dd when the client was taught to work around
>>> server non-compliance; but is equally fixed if the server is patched
>>> to not send unaligned replies in the first place (the next few patches
>>> will address that).
> 
> I'll have to reword this now that we know 4.0 will still have some of
> those server bugs in place (as the tail of this series is deferred to 4.1).
> 
> 
>>> @@ -660,6 +662,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
>>>
>>>        if (client->opt == NBD_OPT_GO) {
>>>            client->exp = exp;
>>> +        client->check_align = blocksize ?
>>> +            blk_get_request_alignment(exp->blk) : 0;
>>
>> I think better set in same place, where sizes[0] set, or use sizes[0] here or add separate local
>> varibale for it, so that, if "sizes[0] =" changes somehow, we will not forget to fix this place too.
> 
> I don't want to set client->check_align for NBD_OPT_INFO; but I can
> indeed use a temporary variable or hoist the computation so that it is
> all in one spot.
> 
> 
>>> +++ b/nbd/trace-events
>>> @@ -71,4 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t
>>>    nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'"
>>>    nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)"
>>>    nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32
>>> +nbd_co_receive_align_compliance(const char *op) "client sent non-compliant unaligned %s request"
>>
>> Don't you want print request->from, request->len and client->check_align as well?
> 
> Wouldn't hurt.
> 
>>
>>>    nbd_trip(void) "Reading request"
>>>
>>
>> Patch seems correct anyway, so if you are in a hurry, it's OK as is:
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
> 
> Here's what I'll squash in; I think it is obvious enough to still keep
> your R-b, if I send the pull request before you reply back.
> 
> diff --git i/nbd/server.c w/nbd/server.c
> index 3040ceb5606..cb38dfe6902 100644
> --- i/nbd/server.c
> +++ w/nbd/server.c
> @@ -535,6 +535,7 @@ static int nbd_negotiate_handle_info(NBDClient
> *client, uint16_t myflags,
>       bool blocksize = false;
>       uint32_t sizes[3];
>       char buf[sizeof(uint64_t) + sizeof(uint16_t)];
> +    uint32_t check_align = 0;
> 
>       /* Client sends:
>           4 bytes: L, name length (can be 0)
> @@ -611,7 +612,7 @@ static int nbd_negotiate_handle_info(NBDClient
> *client, uint16_t myflags,
>        * whether this is OPT_INFO or OPT_GO. */
>       /* minimum - 1 for back-compat, or actual if client will obey it. */
>       if (client->opt == NBD_OPT_INFO || blocksize) {
> -        sizes[0] = blk_get_request_alignment(exp->blk);
> +        check_align = sizes[0] = blk_get_request_alignment(exp->blk);
>       } else {
>           sizes[0] = 1;
>       }
> @@ -662,8 +663,7 @@ static int nbd_negotiate_handle_info(NBDClient
> *client, uint16_t myflags,
> 
>       if (client->opt == NBD_OPT_GO) {
>           client->exp = exp;
> -        client->check_align = blocksize ?
> -            blk_get_request_alignment(exp->blk) : 0;
> +        client->check_align = check_align;
>           QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
>           nbd_export_get(client->exp);
>           nbd_check_meta_export(client);
> @@ -2136,7 +2136,10 @@ static int nbd_co_receive_request(NBDRequestData
> *req, NBDRequest *request,
>            * The block layer gracefully handles unaligned requests, but
>            * it's still worth tracing client non-compliance
>            */
> -
> trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type));
> +        trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type,
> +                                                             request->from,
> +                                                             request->len,
> +
> client->check_align));

something strange here with brackets.

>       }
>       valid_flags = NBD_CMD_FLAG_FUA;
>       if (request->type == NBD_CMD_READ && client->structured_reply) {
> diff --git i/nbd/trace-events w/nbd/trace-events
> index 87a2b896c35..ec2d46c9247 100644
> --- i/nbd/trace-events
> +++ w/nbd/trace-events
> @@ -71,5 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int
> extents, uint32_t id, uint64_t
>   nbd_co_send_structured_error(uint64_t handle, int err, const char
> *errname, const char *msg) "Send structured error reply: handle = %"
> PRIu64 ", error = %d (%s), msg = '%s'"
>   nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type,
> const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16
> " (%s)"
>   nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len)
> "Payload received: handle = %" PRIu64 ", len = %" PRIu32
> -nbd_co_receive_align_compliance(const char *op) "client sent
> non-compliant unaligned %s request"
> +nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t
> len, uint32_t align) "client sent non-compliant unaligned %s request:
> from=0x%" PRIx64 ", len=0x%x, align=0x%x"

%x or % PRIx32 - doesn't matter?

>   nbd_trip(void) "Reading request"
> 
> 
> 

OK for me, thanks.


-- 
Best regards,
Vladimir