Nir Soffer
2022-Feb-20 12:13 UTC
[Libguestfs] [PATCH libnbd 4/8] copy: Separate finishing a command from freeing it
free_command() was abused as a completion callback. Introduce
finish_command() completion callback, so code that want to free a
command does not have to add dummy errors.
This will make it easier to manage worker state when a command
completes.
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
copy/multi-thread-copying.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
index 855d1ba4..aa6a9f41 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -126,21 +126,22 @@ multi_thread_copying (void)
}
}
free (workers);
}
static void wait_for_request_slots (uintptr_t index);
static unsigned in_flight (uintptr_t index);
static void poll_both_ends (uintptr_t index);
static int finished_read (void *vp, int *error);
-static int free_command (void *vp, int *error);
+static int finished_command (void *vp, int *error);
+static void free_command (struct command *command);
static void fill_dst_range_with_zeroes (struct command *command);
static struct command *create_command (uint64_t offset, size_t len, bool zero,
uintptr_t index);
/* There are 'threads' worker threads, each copying work ranges from
* src to dst until there are no more work ranges.
*/
static void *
worker_thread (void *indexp)
{
@@ -402,53 +403,52 @@ finished_read (void *vp, int *error)
command->offset, strerror (*error));
exit (EXIT_FAILURE);
}
if (allocated || sparse_size == 0) {
/* If sparseness detection (see below) is turned off then we write
* the whole command.
*/
dst->ops->asynch_write (dst, command,
(nbd_completion_callback) {
- .callback = free_command,
+ .callback = finished_command,
.user_data = command,
});
}
else { /* Sparseness detection. */
const uint64_t start = command->offset;
const uint64_t end = start + command->slice.len;
uint64_t last_offset = start;
bool last_is_zero = false;
uint64_t i;
struct command *newcommand;
- int dummy = 0;
/* Iterate over whole blocks in the command, starting on a block
* boundary.
*/
for (i = MIN (ROUND_UP (start, sparse_size), end);
i + sparse_size <= end;
i += sparse_size) {
if (is_zero (slice_ptr (command->slice) + i-start, sparse_size)) {
/* It's a zero range. If the last was a zero too then we do
* nothing here which coalesces. Otherwise write the last data
* and start a new zero range.
*/
if (!last_is_zero) {
/* Write the last data (if any). */
if (i - last_offset > 0) {
newcommand = create_subcommand (command,
last_offset, i - last_offset,
false);
dst->ops->asynch_write (dst, newcommand,
(nbd_completion_callback) {
- .callback = free_command,
+ .callback = finished_command,
.user_data = newcommand,
});
}
/* Start the new zero range. */
last_offset = i;
last_is_zero = true;
}
}
else {
/* It's data. If the last was data too, do nothing =>
@@ -471,46 +471,46 @@ finished_read (void *vp, int *error)
} /* for i */
/* Write the last_offset up to i. */
if (i - last_offset > 0) {
if (!last_is_zero) {
newcommand = create_subcommand (command,
last_offset, i - last_offset,
false);
dst->ops->asynch_write (dst, newcommand,
(nbd_completion_callback) {
- .callback = free_command,
+ .callback = finished_command,
.user_data = newcommand,
});
}
else {
newcommand = create_subcommand (command,
last_offset, i - last_offset,
true);
fill_dst_range_with_zeroes (newcommand);
}
}
/* There may be an unaligned tail, so write that. */
if (end - i > 0) {
newcommand = create_subcommand (command, i, end - i, false);
dst->ops->asynch_write (dst, newcommand,
(nbd_completion_callback) {
- .callback = free_command,
+ .callback = finished_command,
.user_data = newcommand,
});
}
/* Free the original command since it has been split into
* subcommands and the original is no longer needed.
*/
- free_command (command, &dummy);
+ free_command (command);
}
return 1; /* auto-retires the command */
}
/* Fill a range in dst with zeroes. This is called from the copying
* loop when we see a zero range in the source. Depending on the
* command line flags this could mean:
*
* --destination-is-zero:
@@ -523,29 +523,28 @@ finished_read (void *vp, int *error)
* zeroing command or fallback to writing a command
* of zeroes.
*
* This takes over ownership of the command and frees it eventually.
*/
static void
fill_dst_range_with_zeroes (struct command *command)
{
char *data;
size_t data_size;
- int dummy = 0;
if (destination_is_zero)
goto free_and_return;
/* Try efficient zeroing. */
if (dst->ops->asynch_zero (dst, command,
(nbd_completion_callback) {
- .callback = free_command,
+ .callback = finished_command,
.user_data = command,
},
allocated))
return;
/* Fall back to loop writing zeroes. This is going to be slow
* anyway, so do it synchronously. XXX
*/
data_size = MIN (request_size, command->slice.len);
data = calloc (1, data_size);
@@ -559,36 +558,43 @@ fill_dst_range_with_zeroes (struct command *command)
if (len > data_size)
len = data_size;
dst->ops->synch_write (dst, data, len, command->offset);
command->slice.len -= len;
command->offset += len;
}
free (data);
free_and_return:
- free_command (command, &dummy);
+ free_command (command);
}
static int
-free_command (void *vp, int *error)
+finished_command (void *vp, int *error)
{
struct command *command = vp;
- struct buffer *buffer = command->slice.buffer;
if (*error) {
fprintf (stderr, "write at offset %" PRId64 " failed:
%s\n",
command->offset, strerror (*error));
exit (EXIT_FAILURE);
}
+ free_command (command);
+
+ return 1; /* auto-retires the command */
+}
+
+static void
+free_command (struct command *command)
+{
+ struct buffer *buffer = command->slice.buffer;
+
if (buffer != NULL) {
if (--buffer->refs == 0) {
free (buffer->data);
free (buffer);
}
}
free (command);
-
- return 1; /* auto-retires the command */
}
--
2.35.1
Richard W.M. Jones
2022-Feb-20 18:43 UTC
[Libguestfs] [PATCH libnbd 4/8] copy: Separate finishing a command from freeing it
On Sun, Feb 20, 2022 at 02:13:59PM +0200, Nir Soffer wrote:> free_command() was abused as a completion callback. Introduce > finish_command() completion callback, so code that want to free aIn the code it's called "finished_command". ACK patches 1, 2, 3 & 4 (with this fix to the commit message). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Eric Blake
2022-Feb-21 15:08 UTC
[Libguestfs] [PATCH libnbd 4/8] copy: Separate finishing a command from freeing it
On Sun, Feb 20, 2022 at 02:13:59PM +0200, Nir Soffer wrote:> free_command() was abused as a completion callback. Introduce > finish_command() completion callback, so code that want to free a > command does not have to add dummy errors. > > This will make it easier to manage worker state when a command > completes. > > Signed-off-by: Nir Soffer <nsoffer at redhat.com> > --- > copy/multi-thread-copying.c | 34 ++++++++++++++++++++-------------- > 1 file changed, 20 insertions(+), 14 deletions(-)In addition to Rich's review,> > static int > -free_command (void *vp, int *error) > +finished_command (void *vp, int *error) > { > struct command *command = vp; > - struct buffer *buffer = command->slice.buffer; > > if (*error) { > fprintf (stderr, "write at offset %" PRId64 " failed: %s\n", > command->offset, strerror (*error)); > exit (EXIT_FAILURE); > } > > + free_command (command); > + > + return 1; /* auto-retires the command */ > +} > + > +static void > +free_command (struct command *command) > +{ > + struct buffer *buffer = command->slice.buffer;Do we want to allow 'free_command (NULL)', in which case this should check if command is non-NULL before initializing buffer? Doing so may make some other cleanup paths easier to write. But for now, all callers pass in non-NULL, so ACK either way. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org