[Libguestfs] [libnbd PATCH 4/4] copy: Ignore extents if we encountered earlier error

[Laszlo Ersek]

On 02/02/22 15:54, Eric Blake wrote:
> On Wed, Feb 02, 2022 at 09:42:46AM +0100, Laszlo Ersek wrote:
>> On 02/01/22 20:44, Eric Blake wrote:
>>> In practice, the nbd extents callback will only be called once per
>>> nbd_aio_block_status, because we have only requested exactly one
>>> metacontext id, and because compliant servers send only one reply
>>> chunk per id.  In theory, a server could reply first with an error
>>> chunk (perhaps meaning "I'm unable to get status beyond offset X")
>>> followed by an extent chunk ("but here's what I was able to get")
>>> while still being compliant.  And a non-compliant server can send
>>> unexpected chunks, just to try and confuse us.  Right now, it would
>>> take a custom NBD server to actually trigger any scenario where *error
>>> would ever be non-zero on entry to next_extent() (nbdkit does not have
>>> that much power).  So while *error ever being non-zero is unlikely,
>>> our easiest course is to handle it the same way we would for a server
>>> sending us an unexpected id: ignore the rest of the extent callback.
>>> The later completion callback will handle the overall command failing.
>>
>> This commit message mostly answers my question under the cover letter,
>> and indeed it's quite the opposite of what I expected. I'd have expected
>> the command (the exchange with the server) completing, upon which time
>> the completion callback would have run; then the results (a series of
>> extent callbacks) would be passed to the extent callback.
>>
>> So, that's precisely *not* the case in reality. :/
> 
> Yeah, understanding the control flow is helpful.  The first thing to
> note: every client command has a sticky error field (see struct
> command->error); this field starts life 0, and while the command is
> live, it is set to non-zero in the following scenarios: if the client
> never sends the command to the server (for example, settings from
> nbd_set_strict_mode() determined it would be an out-of-bounds
> request); when the server replies with NBD_REPLY_TYPE_ERROR in a
> structured reply (although the command may still be live if it wasn't
> the final reply chunk); when the server replies with an error in a
> simple reply (with simple replies, the command is necessarily
> complete); or when a client mid-command callback returns -1 (only
> pread_structured and block_status have these, and the command may
> still be live if it wasn't the final reply chunk).  Once the command
> is complete, then we call the completion callback (if there is one)
> with *error set to the value of command->error, giving one more chance
> to change the value stored in command->error; and then
> nbd_aio_command_completed returns the value of command->error as the
> error status of the overall command (unless the completion callback
> returned 1 to auto-retire the command first).
> 
> Then we have nbd_aio_block_status which kicks off a transaction that
> will eventually send NBD_CMD_BLOCK_STATUS, and is associated with TWO
> callbacks: the mandatory mid-command nbd_extents callback to read
> block status information as it comes over the wire from the server,
> and the optional nbd_completion callback to mark when the server has
> nothing more to send.
> 
> The documentation states that the nbd_extents callback can be called 0
> or more times (0 if the server replies with an error, or even if
> nbd_set_strict_mode() determines that the command violates the NBD
> protocol and is never even sent to the server).  If you only have one
> metacontext id (as in nbdcopy only caring about base:allocation), one
> time is all the more we expect from a compliant server (a noncompliant
> server can send more than one reply, but we'd have to code up such a
> custom server, as nbdkit does not behave that way).  The *error
> parameter passed to nbd_extents is non-zero only if there is a running
> error detected so far (generally unlikely; a server would have to send
> an error chunk before a status chunk, or send more than one status
> chunk where the libnbd client set *error during the first time the
> callback was reached).
> 
> Then the nbd_completion callback is always called exactly once, after
> there is nothing further to be done with the command.  The *error
> parameter passed to the nbd_completion callback is the same value that
> will be returned from nbd_aio_command_comppleted (unless the callback
> returns 1 for auto-retiring).
> 
> And finally we have nbd_block_status: a synchronous thin wrapper that
> calls nbd_aio_block_status, nbd_aio_poll, nbd_aio_command_completed
> under the hood.  The nbd_extents callback is still mandatory, and is
> called mid-command prior to the function call completing, in response
> to server information as it comes over the wire.  The optional
> nbd_completion callback of nbd_aio_block_status is not exposed (the
> command completes synchronously, instead; whether we used the
> completion callback under the hood is an implementation detail that
> doesn't matter to the public interface).

Thanks. This is also really helpful. And I've peeked at the code
("generator/states-reply-structured.c") and it seems like a callback has
no way to "clear" an error that was made sticky earlier; good! (Upon
second reading, Rich's email also stated the same.)

> 
> I'm open to a documentation patch that gives more clarity on this
> sequence of callbacks.

Haha, that's my line! :) As in, "please write this up in a documentation
patch" :)

But, I know how this works -- I've been on the answering side. So I
thank you and Rich very much this info, and when I next need it, I will
find it in my (indexed) email archive.

> 
>>
>>> ---
>>>  copy/nbd-ops.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c
>>> index dfc62f20..ab30badd 100644
>>> --- a/copy/nbd-ops.c
>>> +++ b/copy/nbd-ops.c
>>> @@ -1,5 +1,5 @@
>>>  /* NBD client library in userspace.
>>> - * Copyright (C) 2020 Red Hat Inc.
>>> + * Copyright (C) 2020-2022 Red Hat Inc.
>>>   *
>>>   * This library is free software; you can redistribute it and/or
>>>   * modify it under the terms of the GNU Lesser General Public
>>> @@ -337,7 +337,7 @@ add_extent (void *vp, const char *metacontext,
>>>    extent_list *ret = vp;
>>>    size_t i;
>>>
>>> -  if (strcmp (metacontext, "base:allocation") != 0)
>>> +  if (strcmp (metacontext, "base:allocation") != 0 || *error)
>>>      return 0;
>>>
>>>    for (i = 0; i < nr_entries; i += 2) {
>>>
>>
>> So, I don't understand this. First, here we have a non-aio (i.e.,
>> synchronous) nbd_block_status call. in case add_extent (the extents
>> callback) is entered with *error !=0, that means (per docs) "the error
>> parameter contains the errno value of any previously detected error".
> 
> I guess what the docs are missing is the disclaimer that if the
> mid-command callback is reached, there is usually no previously
> detected error.  The only two situations where there is a previous
> error is if the server replies with NBD_REPLY_TYPE_ERROR first and
> then NBD_REPLY_TYPE_BLOCK_STATUS second (a custom server), or if the
> client negotiated more than one context and the first use of the
> callback assigns an error (by returning -1) before the second callback
> is reached.  The other cases where an error could be detected never
> reach the mid-command callback (if nbd_set_strict_mode() prevented the
> command from ever going to the server, or if the server replied with
> an error as a final chunk).

OK! Clear now!

> 
>>
>> In case we responded by returning -1, then the following (per docs)
>> would apply:
>>
>> "If the callback returns -1, and no earlier error has been detected,
>> then the overall block status command will fail with any non-zero value
>> stored into the callback's error parameter"
>>
>> But we don't do that, we say "everything is OK, I'll just ignore this",
>> like we ignore metadata contexts that we don't need.
> 
> Yes. But even though the mid-command callback ignores the error, the
> synchronous wrapper will still see the same error via
> nbd_aio_command_completed under the hood, so the overall
> nbd_blocK_status will fail with the same value.
> 
>>
>> So how is the "later completion callback" going to fail the entire
>> nbd_block_status() call? First, because this is not an AIO
>> nbd_block_status() call, we don't provide the completion callback
>> ourselves -- I guess it is some default function somewhere in libnbd.
> 
> Right - the synchronous wrappers return the same value as would be
> present in *error given to the completion callback of an asynchronous
> API (whether the synchronous wrappers actually use the completion
> callback, or get at the value directly, is an implementation detail).
> And remember that the completion callback to the asynchronous APIs is
> optional; if it is not provided, that error value is visible through
> nbd_aio_command_completed.
> 
>>
>> Or is it the following: upon getting (*error != 0) on entry, we just
>> decline looking at the (likely nonsensical) "entries" array, and permit
>> the error we got via *error to "survive" through all further
>> add_extent() calls, and to finally fail the outermost nbd_block_status()
>> call?
> 
> Yes, that is the best way of looking at it.  In fact, the "entries"
> array is most likely valid, rather than non-sensical, but since we
> know that our response to a failing nbd_block_status() is to die,
> there's no point wasting time processing the valid extents to prolong
> the time before we die.  In other words, this patch is optional; it
> does not add safety (as the error will resurface as the result of
> nbd_block_status, where we will fail), but merely speed (since we are
> going to fail anyways, why waste the time getting to that failure).
> 
>>
>> Next question: even if that's the case, should we *first* check *error,
>> and only check "metacontext" if *error is zero? What says that
>> "metacontext" is defined if *error is nonzero?
> 
> The nbd_extents callback is only reached any time the server replies
> with NBD_REPLY_TYPE_BLOCK_STATUS with a metacontext id that we
> recognized.  If we did not recognize the id, or if the server never
> replies with that chunk, then we don't call the mid-command callback
> in the first place.  So the "metacontext" and "extents" parameters to
> nbd_extents are always valid, even if *error is nonzero.
> 
> And now that I've written this email, I wish I could incorporate some
> of it into the commit message, even though it is already pushed.
> 

Your response will stay in the mailing list archive :)

(This is BTW one reason why I like to apply my own patches from the
list, for pushing: with "git am -m" (and "-m" can be made the default
config), Message-Id footers are created in the commit messages
automatically. It's always nice to have a permanent link from the commit
to the posting / thread!)

Thank you!
Laszlo