[Libguestfs] [PATCH nbdkit 5/5] vddk: Implement parallel thread model
Nir Soffer
nsoffer at redhat.com
Thu Oct 28 00:34:53 UTC 2021
On Wed, Oct 27, 2021 at 8:45 PM Laszlo Ersek <lersek at redhat.com> wrote:
...
> (hopefully that means nbdkit_error is thread-safe)
>
> > + return ! cmd->error ? 0 : -1;
>
> (6) Using the ! operator with the ternary operator ?: is awkward IMO;
> how about just
>
> return cmd->error ? -1 : 0;
cmd->failed is more consistent, but I think we should get rid of this field.
>
> > +}
> > +
> > +/* Add an extent to the list of extents. */
> > +static int
> > +add_extent (struct nbdkit_extents *extents,
> > + uint64_t *position, uint64_t next_position, bool is_hole)
> > +{
> > + uint32_t type = 0;
> > + const uint64_t length = next_position - *position;
> > +
> > + if (is_hole) {
> > + type = NBDKIT_EXTENT_HOLE;
> > + /* Images opened as single link might be backed by another file in the
> > + chain, so the holes are not guaranteed to be zeroes. */
> > + if (!single_link)
> > + type |= NBDKIT_EXTENT_ZERO;
> > + }
> > +
> > + assert (*position <= next_position);
> > + if (*position == next_position)
> > + return 0;
> > +
> > + if (vddk_debug_extents)
> > + nbdkit_debug ("adding extent type %s at [%" PRIu64 "...%" PRIu64 "]",
> > + is_hole ? "hole" : "allocated data",
> > + *position, next_position-1);
> > + if (nbdkit_add_extent (extents, *position, length, type) == -1)
> > + return -1;
> > +
> > + *position = next_position;
> > + return 0;
> > +}
> > +
> > +/* Asynchronous commands are completed when this function is called. */
> > +static void
> > +complete_command (void *vp, VixError err)
> > +{
> > + struct command *cmd = vp;
> > +
> > + if (err != VIX_OK) {
> > + VDDK_ERROR (err, "asynchronous %s failed", command_type_string (cmd->type));
> > + cmd->error = true;
> > + }
> > +
> > + ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&cmd->mutex);
> > + cmd->completed = true;
> > + pthread_cond_signal (&cmd->cond);
> > +}
>
> Seems OK. The ordering seems to match the counterpart in
> send_command_and_wait().
>
> (7) Suggestion: in the definition of "struct command", document that the
> mutex & condvar protect the "completed" field, and that "error" is only
> meaningful once "completed" is set. (From the current comments on the
> fields, I wasn't initially sure whether "error" should be checked
> independently of "completed".)
The real issue is having 2 fields to manage the state, allowing
4 possible states, when only 2 states are valid.
We can replace the fields with:
enum command_state { STATE_QUEUED, STATE_RUNNING, STATE_SUCCEEDED,
STATE_FAILED };
...
struct command {
enum command_state state
I did not have time to review the rest of the patch.
Nir
More information about the Libguestfs
mailing list