[PATCH 2/6] qga: Move command execution code to a separate function
In qmp_guest_set_user_password() we have a part of code that we can reuse
in the future commits. Move this code to a separate function.
Signed-off-by: Alexander Ivanov
---
qga/commands-posix.c | 139 ++-
1 file changed, 72 insertions(+), 67 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 6169bbf7a0..e7b82aaf37 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2114,20 +2114,78 @@ int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList
*vcpus, Error **errp)
#endif /* __linux__ */
#if defined(__linux__) || defined(__FreeBSD__)
-void qmp_guest_set_user_password(const char *username,
- const char *password,
- bool crypted,
- Error **errp)
+
+static void run_command(const char *argv[], const char *in_str, Error **errp)
{
Error *local_err = NULL;
-char *passwd_path = NULL;
pid_t pid;
-int status;
+int in_len, status;
int datafd[2] = { -1, -1 };
+
+if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
+error_setg(errp, "cannot create pipe FDs");
+goto out;
+}
+
+pid = fork();
+if (pid == 0) {
+close(datafd[1]);
+setsid();
+dup2(datafd[0], 0);
+reopen_fd_to_null(1);
+reopen_fd_to_null(2);
+
+execve(argv[0], (char *const *)argv, environ);
+_exit(EXIT_FAILURE);
+} else if (pid < 0) {
+error_setg_errno(errp, errno, "failed to create child process");
+goto out;
+}
+close(datafd[0]);
+datafd[0] = -1;
+
+in_len = strlen(in_str);
+
+if (qemu_write_full(datafd[1], in_str, in_len) != in_len) {
+error_setg_errno(errp, errno, "cannot write new account password");
+goto out;
+}
+close(datafd[1]);
+datafd[1] = -1;
+
+ga_wait_child(pid, , _err);
+if (local_err) {
+error_propagate(errp, local_err);
+goto out;
+}
+
+if (!WIFEXITED(status)) {
+error_setg(errp, "child process has terminated abnormally");
+goto out;
+}
+
+if (WEXITSTATUS(status)) {
+error_setg(errp, "child process has failed: %s", argv[0]);
+}
+
+out:
+if (datafd[0] != -1) {
+close(datafd[0]);
+}
+if (datafd[1] != -1) {
+close(datafd[1]);
+}
+}
+
+void qmp_guest_set_user_password(const char *username,
+ const char *password,
+ bool crypted,
+ Error **errp)
+{
+char *passwd_path = NULL;
char *rawpasswddata = NULL;
-size_t rawpasswdlen;
char *chpasswddata = NULL;
-size_t chpasswdlen;
+size_t rawpasswdlen;
rawpasswddata = (char *)qbase64_decode(password, -1, , errp);
if (!rawpasswddata) {
@@ -2155,79 +2213,26 @@ void qmp_guest_set_user_password(const char *username,
passwd_path = g_find_program_in_path("chpasswd");
#endif
-chpasswdlen = strlen(chpasswddata);
-
if (!passwd_path) {
error_setg(errp, "cannot find 'passwd' program in PATH");
goto out;
}
-if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
-error_setg(errp, "cannot create pipe FDs");
-goto out;
-}
-
-pid = fork();
-if (pid == 0) {
-close(datafd[1]);
-/* child */
-setsid();
-dup2(datafd[0], 0);
-reopen_fd_to_null(1);
-reopen_fd_to_null(2);
-
+const char *argv[] = {
#ifdef __FreeBSD__
-const char *h_arg;
-h_arg = (crypted) ? "-H" : "-h";
-execl(passwd_path, "pw", "usermod", "-n", username, h_arg, "0", NULL);
+passwd_path, "pw", "usermod", "-n", username,
+(crypted) ? "-H" : "-h", "0", NULL};
#else
-if (crypted) {
-execl(passwd_path, "chpasswd", "-e", NULL);
-} else {
-execl(passwd_path, "chpasswd", NULL);
-}
+passwd_path, "chpasswd", (crypted) ? "-e" : NULL, NULL
#endif
-_exit(EXIT_FAILURE);
-} else if (pid < 0) {
-error_setg_errno(errp, errno, "failed to create child process");
-goto out;
-}
-close(datafd[0]);
-datafd[0] = -1;
+};
-if (qemu_write_full(datafd[1], chpasswddata, chpasswdlen) != chpasswdlen) {
-error_setg_errno(errp, errno, "cannot write new account password");
-goto out;
-}
-close(datafd[1]);
-datafd[1] = -1;
-
-ga_wait_child(pid, , _err);
-if (local_err) {
-error_propagate(errp, local_err);
-goto out;
-}
-
-if (!WIFEXITED(status)) {
-error_setg(errp, "child process has terminated abnormally&qu
[PATCH 4/6] qga: Add user creation functionality
Add an optional argument "create" to guest-set-user-password command to
create a user with provided username and password.
Signed-off-by: Alexander Ivanov
---
qga/commands-posix.c | 19 +++
qga/commands-win32.c | 22 ++
qga/qapi-schema.json | 5 -
3 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 461b4d7bb6..26711a1a72 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2189,6 +2189,7 @@ out:
void qmp_guest_set_user_password(const char *username,
const char *password,
bool crypted,
+ bool has_create, bool create,
Error **errp)
{
char *passwd_path = NULL;
@@ -2227,6 +2228,24 @@ void qmp_guest_set_user_password(const char *username,
goto out;
}
+/* create new user if requested */
+if (has_create && create) {
+char *str = g_shell_quote(username);
+char *cmd = g_strdup_printf(
+/* we want output only from useradd command */
+"id -u %s >/dev/null 2>&1 || useradd -m %s",
+str, str);
+const char *argv[] = {
+"/bin/sh", "-c", cmd, NULL
+};
+run_command(argv, NULL, errp);
+g_free(str);
+g_free(cmd);
+if (*errp) {
+goto out;
+}
+}
+
const char *argv[] = {
#ifdef __FreeBSD__
passwd_path, "pw", "usermod", "-n", username,
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 5aa43a9ed7..618d862c00 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1921,6 +1921,7 @@ get_net_error_message(gint error)
void qmp_guest_set_user_password(const char *username,
const char *password,
bool crypted,
+ bool has_create, bool create,
Error **errp)
{
NET_API_STATUS nas;
@@ -1952,6 +1953,27 @@ void qmp_guest_set_user_password(const char *username,
goto done;
}
+if (has_create && create) {
+USER_INFO_1 ui = { 0 };
+
+ui.usri1_name = user;
+ui.usri1_password = wpass;
+ui.usri1_priv = USER_PRIV_USER;
+ui.usri1_flags = UF_SCRIPT | UF_DONT_EXPIRE_PASSWD;
+nas = NetUserAdd(NULL, 1, (LPBYTE) & ui, NULL);
+
+if (nas == NERR_Success) {
+goto done;
+}
+
+if (nas != NERR_UserExists) {
+gchar *msg = get_net_error_message(nas);
+error_setg(errp, "failed to add user: %s", msg);
+g_free(msg);
+goto done;
+}
+}
+
pi1003.usri1003_password = wpass;
nas = NetUserSetInfo(NULL, user,
1003, (LPBYTE),
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index b39be4cdc2..e96d463639 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1059,6 +1059,8 @@
# @password: the new password entry string, base64 encoded
#
# @crypted: true if password is already crypt()d, false if raw
+# @create: #optinal user will be created if it does not exist yet.
+# The default value is false. (since 8.2)
#
# If the @crypted flag is true, it is the caller's responsibility to
# ensure the correct crypt() encryption scheme is used. This command
@@ -1078,7 +1080,8 @@
# Since: 2.3
##
{ 'command': 'guest-set-user-password',
- 'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool' } }
+ 'data': { 'username': 'str', 'password': 'str', 'crypted': 'bool',
+ '*create': 'bool' } }
##
# @GuestMemoryBlock:
--
2.34.1
[PATCH 1/6] qga: Add process termination functionality
We need to terminate processes executed with guest-exec command. Add
guest-exec-terminate command for process termination by PID.
Signed-off-by: Alexander Ivanov
---
qga/commands-common.h | 2 ++
qga/commands-win32.c | 64 +++
qga/commands.c| 34 +++
qga/qapi-schema.json | 13 +
4 files changed, 113 insertions(+)
diff --git a/qga/commands-common.h b/qga/commands-common.h
index 8c1c56aac9..34b9a22578 100644
--- a/qga/commands-common.h
+++ b/qga/commands-common.h
@@ -80,4 +80,6 @@ GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh,
*/
char *qga_get_host_name(Error **errp);
+int kill_process_tree(int64_t pid);
+
#endif
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 697c65507c..5aa43a9ed7 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "guest-agent-core.h"
#include "vss-win32.h"
@@ -2522,3 +2523,66 @@ GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
error_setg(errp, QERR_UNSUPPORTED);
return NULL;
}
+
+int kill_process_tree(int64_t pid)
+{
+PROCESSENTRY32 proc_entry;
+HANDLE snapshot, process;
+GList *pid_entry, *pid_list = NULL;
+bool added, success;
+int res = 0;
+
+snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+if (snapshot == INVALID_HANDLE_VALUE) {
+return GetLastError();
+}
+
+pid_list = g_list_append(pid_list, GUINT_TO_POINTER(pid));
+
+proc_entry.dwSize = sizeof(PROCESSENTRY32);
+do {
+added = false;
+for (success = Process32First(snapshot, _entry);
+ success; success = Process32Next(snapshot, _entry)) {
+gpointer ppid_p, pid_p;
+ppid_p = GUINT_TO_POINTER(proc_entry.th32ParentProcessID);
+pid_p = GUINT_TO_POINTER(proc_entry.th32ProcessID);
+if (g_list_find(pid_list, ppid_p) && !g_list_find(pid_list,
pid_p)) {
+pid_list = g_list_append(pid_list, pid_p);
+added = true;
+}
+}
+} while (added);
+
+for (success = Process32First(snapshot, _entry);
+ success; success = Process32Next(snapshot, _entry)) {
+if (g_list_find(pid_list, GUINT_TO_POINTER(proc_entry.th32ProcessID)))
{
+g_debug("killing pid=%u ppid=%u name=%s",
+(guint)proc_entry.th32ProcessID,
+(guint)proc_entry.th32ParentProcessID,
+proc_entry.szExeFile);
+}
+}
+
+CloseHandle(snapshot);
+
+for (pid_entry = pid_list; pid_entry; pid_entry = pid_entry->next) {
+pid = GPOINTER_TO_UINT(pid_entry->data);
+process = OpenProcess(PROCESS_TERMINATE, FALSE, pid);
+if (process == INVALID_HANDLE_VALUE) {
+if (!res) {
+res = GetLastError();
+if (res == ERROR_FILE_NOT_FOUND) {
+res = 0;
+}
+}
+continue;
+}
+TerminateProcess(process, 255);
+CloseHandle(process);
+}
+
+g_list_free(pid_list);
+
+return res;
+}
diff --git a/qga/commands.c b/qga/commands.c
index ce172edd2d..af8459c587 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -529,6 +529,40 @@ done:
return ge;
}
+void qmp_guest_exec_terminate(int64_t pid, Error **errp)
+{
+GuestExecInfo *gei;
+
+slog("guest-exec-terminate called, pid: %u", (uint32_t)pid);
+
+gei = guest_exec_info_find(pid);
+if (gei == NULL) {
+error_setg(errp, QERR_INVALID_PARAMETER, "pid");
+return;
+}
+
+if (gei->finished) {
+return;
+}
+
+#ifdef G_OS_WIN32
+char buf[32];
+int res;
+
+res = kill_process_tree(pid);
+if (res != 0) {
+snprintf(buf, sizeof(buf), "win32 err %d", res);
+error_setg(errp, QERR_QGA_COMMAND_FAILED, buf);
+}
+#else
+if (kill(pid, SIGKILL) < 0) {
+if (errno != ESRCH) {
+error_setg(errp, QERR_QGA_COMMAND_FAILED, strerror(errno));
+}
+}
+#endif
+}
+
/* Convert GuestFileWhence (either a raw integer or an enum value) into
* the guest's SEEK_ constants. */
int ga_parse_whence(GuestFileWhence *whence, Error **errp)
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 876e2a8ea8..b39be4cdc2 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1326,6 +1326,19 @@
'*input-data': 'str', '*capture-output':
'GuestExecCaptureOutput' },
'returns': 'GuestExec' }
+##
+# @guest-exec-terminate:
+#
+# Terminate process associated with PID retrieved via guest-exec.
+#
+# @pid: pid returned from guest-exec
+#
+# Returns: Nothing on success.
+#
+# Since: 8.2
+##
+{ 'command': 'guest-exec-terminate',
+ 'data':{ 'pid': 'int' } }
##
# @GuestHostName:
--
2.34.1
[PATCH 3/6] qga: Let run_command() work without input data
run_command() has in_str argument that specifies the input string.
Let it be NULL if there is no input.
Signed-off-by: Alexander Ivanov
---
qga/commands-posix.c | 35 ++-
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index e7b82aaf37..461b4d7bb6 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2122,16 +2122,22 @@ static void run_command(const char *argv[], const char
*in_str, Error **errp)
int in_len, status;
int datafd[2] = { -1, -1 };
-if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
-error_setg(errp, "cannot create pipe FDs");
-goto out;
+if (in_str) {
+if (!g_unix_open_pipe(datafd, FD_CLOEXEC, NULL)) {
+error_setg(errp, "cannot create pipe FDs");
+goto out;
+}
}
pid = fork();
if (pid == 0) {
-close(datafd[1]);
setsid();
-dup2(datafd[0], 0);
+if (in_str) {
+close(datafd[1]);
+dup2(datafd[0], 0);
+} else {
+reopen_fd_to_null(0);
+}
reopen_fd_to_null(1);
reopen_fd_to_null(2);
@@ -2141,17 +2147,20 @@ static void run_command(const char *argv[], const char
*in_str, Error **errp)
error_setg_errno(errp, errno, "failed to create child process");
goto out;
}
-close(datafd[0]);
-datafd[0] = -1;
-in_len = strlen(in_str);
+if (in_str) {
+close(datafd[0]);
+datafd[0] = -1;
-if (qemu_write_full(datafd[1], in_str, in_len) != in_len) {
-error_setg_errno(errp, errno, "cannot write new account password");
-goto out;
+in_len = strlen(in_str);
+if (qemu_write_full(datafd[1], in_str, in_len) != in_len) {
+error_setg_errno(errp, errno, "cannot write new account password");
+goto out;
+}
+
+close(datafd[1]);
+datafd[1] = -1;
}
-close(datafd[1]);
-datafd[1] = -1;
ga_wait_child(pid, , _err);
if (local_err) {
--
2.34.1
[PATCH 5/6] qga: Add timeout for fsfreeze
In some cases it would be useful to thaw a filesystem by timeout after
freezing this filesystem by guest-fsfreeze-freeze-list. Add an optional
argument "timeout" to the command.
Signed-off-by: Alexander Ivanov
---
qga/commands-posix.c | 21 ++---
qga/commands-win32.c | 16 ++--
qga/guest-agent-core.h | 3 ++-
qga/main.c | 19 ++-
qga/qapi-schema.json | 9 -
5 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 26711a1a72..e8a79e0a41 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -707,13 +707,17 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error
**errp)
return GUEST_FSFREEZE_STATUS_THAWED;
}
-int64_t qmp_guest_fsfreeze_freeze(Error **errp)
+int64_t qmp_guest_fsfreeze_freeze(bool has_timeout, int64_t timeout,
+ Error **errp)
{
-return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
+return qmp_guest_fsfreeze_freeze_list(false, NULL, has_timeout, timeout,
+ errp);
}
int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
strList *mountpoints,
+ bool has_timeout,
+ int64_t timeout,
Error **errp)
{
int ret;
@@ -734,8 +738,11 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
has_mountpoints,
return -1;
}
+if (!has_timeout || timeout < 0) {
+timeout = 0;
+}
/* cannot risk guest agent blocking itself on a write in this state */
-ga_set_frozen(ga_state);
+ga_set_frozen(ga_state, timeout);
ret = qmp_guest_fsfreeze_do_freeze_list(has_mountpoints, mountpoints,
mounts, errp);
@@ -780,6 +787,12 @@ static void guest_fsfreeze_cleanup(void)
}
}
}
+
+gboolean ga_frozen_timeout_cb(gpointer data)
+{
+guest_fsfreeze_cleanup();
+return G_SOURCE_REMOVE;
+}
#endif
/* linux-specific implementations. avoid this if at all possible. */
@@ -3119,6 +3132,8 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp)
int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
strList *mountpoints,
+ bool has_timeout,
+ int64_t timeout,
Error **errp)
{
error_setg(errp, QERR_UNSUPPORTED);
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 618d862c00..51fd6dcd58 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1221,13 +1221,16 @@ GuestFsfreezeStatus qmp_guest_fsfreeze_status(Error
**errp)
* Freeze local file systems using Volume Shadow-copy Service.
* The frozen state is limited for up to 10 seconds by VSS.
*/
-int64_t qmp_guest_fsfreeze_freeze(Error **errp)
+int64_t qmp_guest_fsfreeze_freeze(bool has_timeout, int64_t timeout,
+ Error **errp)
{
return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
}
int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
strList *mountpoints,
+ bool has_timeout,
+ int64_t timeout,
Error **errp)
{
int i;
@@ -1240,8 +1243,11 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
has_mountpoints,
slog("guest-fsfreeze called");
+if (!has_timeout || timeout < 0) {
+timeout = 0;
+}
/* cannot risk guest agent blocking itself on a write in this state */
-ga_set_frozen(ga_state);
+ga_set_frozen(ga_state, timeout);
qga_vss_fsfreeze(, true, mountpoints, _err);
if (local_err) {
@@ -1299,6 +1305,12 @@ static void guest_fsfreeze_cleanup(void)
vss_deinit(true);
}
+gboolean ga_frozen_timeout_cb(gpointer data)
+{
+guest_fsfreeze_cleanup();
+return G_SOURCE_REMOVE;
+}
+
/*
* Walk list of mounted file systems in the guest, and discard unused
* areas.
diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
index b4e7c52c61..d8d1bb9505 100644
--- a/qga/guest-agent-core.h
+++ b/qga/guest-agent-core.h
@@ -39,8 +39,9 @@ void ga_enable_logging(GAState *s);
void G_GNUC_PRINTF(1, 2) slog(const gchar *fmt, ...);
void ga_set_response_delimited(GAState *s);
bool ga_is_frozen(GAState *s);
-void ga_set_frozen(GAState *s);
+void ga_set_frozen(GAState *s, int64_t timeout);
void ga_unset_frozen(GAState *s);
+gboolean ga_frozen_timeout_cb(gpointer data);
const char *ga_fsfreeze_hook(GAState *s);
int64_t ga_get_fd_handle(GAState *s, Error **errp);
int ga_parse_whence(GuestFileWhence *whence, Error **errp);
diff --git a/qga/main.c b/qga/main.c
index 8668b9f3d3..6c7c7d68d8 100644
--- a/qga/main.c
+++ b/qga/main.
[PATCH 6/6] qga: Cancel async snapshot before abort
VSS requestor calls abort after the timeout of the backup operation
expires. In the result later the process hangs on some internal VSS
lock. Cancel async snapshot before abort.
Signed-off-by: Alexander Ivanov
---
qga/vss-win32/requester.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/qga/vss-win32/requester.cpp b/qga/vss-win32/requester.cpp
index 9884c65e70..20680a42a1 100644
--- a/qga/vss-win32/requester.cpp
+++ b/qga/vss-win32/requester.cpp
@@ -533,6 +533,7 @@ void requester_freeze(int *num_vols, void *mountpoints,
ErrorSet *errset)
}
if (wait_status != WAIT_OBJECT_0) {
+vss_ctx.pAsyncSnapshot->Cancel();
err_set(errset, E_FAIL,
"couldn't receive Frozen event from VSS provider");
goto out;
--
2.34.1
[PATCH 0/6] qga: Assorted patches, let us discuss
* Add a command to terminate processes executed with guest-exec command. * Add an optional argumnet to guest-set-user-password for user creation. * Add an optional argument to guest-fsfreeze-freeze-list to thaw a freezed filesystem by timeout * Fix a freeze after a backup abort. Alexander Ivanov (6): qga: Add process termination functionality qga: Move command execution code to a separate function qga: Let run_command() work without input data qga: Add user creation functionality qga: Add timeout for fsfreeze qga: Cancel async snapshot before abort qga/commands-common.h | 2 + qga/commands-posix.c| 184 +++- qga/commands-win32.c| 102 +++- qga/commands.c | 34 +++ qga/guest-agent-core.h | 3 +- qga/main.c | 19 +++- qga/qapi-schema.json| 27 +- qga/vss-win32/requester.cpp | 1 + 8 files changed, 298 insertions(+), 74 deletions(-) -- 2.34.1
[PATCH v3 12/21] parallels: Let image extensions work in RW mode
Now we support extensions saving and can let to work with them in
read-write mode.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 4
block/parallels.c | 17 -
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 0a632a2331..a0de4f1b07 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -177,10 +177,6 @@ static BdrvDirtyBitmap
*parallels_load_bitmap(BlockDriverState *bs,
return NULL;
}
-/* We support format extension only for RO parallels images. */
-assert(!(bs->open_flags & BDRV_O_RDWR));
-bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
return bitmap;
}
diff --git a/block/parallels.c b/block/parallels.c
index 4c2cb09e43..0a2956b45f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
if (ph.ext_off) {
-if (flags & BDRV_O_RDWR) {
-/*
- * It's unsafe to open image RW if there is an extension (as we
- * don't support it). But parallels driver in QEMU historically
- * ignores the extension, so print warning and don't care.
- */
-warn_report("Format Extension ignored in RW mode");
-} else {
-ret = parallels_read_format_extension(
-bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
-if (ret < 0) {
-goto fail;
-}
+ret = parallels_read_format_extension(
+bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+if (ret < 0) {
+goto fail;
}
}
--
2.34.1
[PATCH v3 16/21] parallels: Truncate images on the last used cluster
On an image closing there can be unused clusters in the end of the image.
Truncate these clusters and update data_end field.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 354fa0f2cc..472311e2e6 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1456,6 +1456,23 @@ fail:
return ret;
}
+static int parallels_truncate_unused_clusters(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+uint64_t end_off = 0;
+if (s->used_bmap_size > 0) {
+end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+if (end_off == s->used_bmap_size) {
+end_off = 0;
+} else {
+end_off = (end_off + 1) * s->cluster_size;
+}
+}
+end_off += s->data_start * BDRV_SECTOR_SIZE;
+s->data_end = end_off / BDRV_SECTOR_SIZE;
+return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+}
+
static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -1473,8 +1490,7 @@ static int GRAPH_RDLOCK
parallels_inactivate(BlockDriverState *bs)
parallels_update_header(bs);
/* errors are ignored, so we might as well pass exact=true */
-ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
-true, PREALLOC_MODE_OFF, 0, NULL);
+ret = parallels_truncate_unused_clusters(bs);
if (ret < 0) {
error_report("Failed to truncate image: %s", strerror(-ret));
}
--
2.34.1
[PATCH v3 09/21] parallels: Create used bitmap even if checks needed
All the checks were fixed to work with used bitmap. Create used bitmap in
parallels_open() even if need_check is true.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 481b6992f4..925aa9e569 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1423,13 +1423,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
need_check = need_check || s->data_end > file_nb_sectors;
-if (!need_check) {
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-goto fail;
-}
-need_check = need_check || ret < 0; /* These are correctable errors */
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
}
+need_check = need_check || ret < 0; /* These are correctable errors */
/*
* We don't repair the image here if it's opened for checks. Also we don't
--
2.34.1
[PATCH v3 18/21] parallels: Remove unnecessary data_end field
Since we have used bitmap, field data_end in BDRVParallelsState is
redundant and can be removed.
Add parallels_data_end() helper and remove data_end handling.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 33 +
block/parallels.h | 1 -
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index d497cdbe41..98967dff90 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -266,6 +266,13 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
s->used_bmap = NULL;
}
+static int64_t parallels_data_end(BDRVParallelsState *s)
+{
+int64_t data_end = s->data_start * BDRV_SECTOR_SIZE;
+data_end += s->used_bmap_size * s->cluster_size;
+return data_end;
+}
+
int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -277,7 +284,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-host_off = s->data_end * BDRV_SECTOR_SIZE;
+host_off = parallels_data_end(s);
prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
bytes = *clusters * s->cluster_size;
prealloc_bytes = prealloc_clusters * s->cluster_size;
@@ -300,9 +307,6 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
-if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
-s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
-}
} else {
next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
@@ -318,8 +322,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
* branch. In the other case we are likely re-using hole. Preallocate
* the space if required by the prealloc_mode.
*/
-if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
-host_off < s->data_end * BDRV_SECTOR_SIZE) {
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
if (ret < 0) {
return ret;
@@ -760,13 +763,7 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
}
}
-if (high_off == 0) {
-res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
-} else {
-res->image_end_offset = high_off + s->cluster_size;
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-}
-
+res->image_end_offset = parallels_data_end(s);
return 0;
}
@@ -805,8 +802,6 @@ static int64_t
parallels_check_unused_clusters(BlockDriverState *bs,
return ret;
}
-s->data_end = end_off / BDRV_SECTOR_SIZE;
-
parallels_free_used_bitmap(bs);
ret = parallels_fill_used_bitmap(bs);
if (ret < 0) {
@@ -1394,8 +1389,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
s->data_start = data_start;
-s->data_end = s->data_start;
-if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
+if (s->data_start < (s->header_size >> BDRV_SECTOR_BITS)) {
/*
* There is not enough unused space to fit to block align between BAT
* and actual data. We can't avoid read-modify-write...
@@ -1438,11 +1432,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
for (i = 0; i < s->bat_size; i++) {
sector = bat2sect(s, i);
-if (sector + s->tracks > s->data_end) {
-s->data_end = sector + s->tracks;
+if (sector + s->tracks > file_nb_sectors) {
+need_check = true;
}
}
-need_check = need_check || s->data_end > file_nb_sectors;
ret = parallels_fill_used_bitmap(bs);
if (ret == -ENOMEM) {
diff --git a/block/parallels.h b/block/parallels.h
index 18b4f8068e..a6a048d890 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -79,7 +79,6 @@ typedef struct BDRVParallelsState {
unsigned int bat_size;
int64_t data_start;
-int64_t data_end;
uint64_t prealloc_size;
ParallelsPreallocMode prealloc_mode;
--
2.34.1
[PATCH v3 01/21] parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()
After used bitmap freeng s->used_bmap points to the freed memory. If we try to free used bitmap one more time it leads to double free error. Set s->used_bmap to NULL to exclude double free error. Signed-off-by: Alexander Ivanov --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 1d695ce7fb..01a61a4ebd 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -245,6 +245,7 @@ static void parallels_free_used_bitmap(BlockDriverState *bs) BDRVParallelsState *s = bs->opaque; s->used_bmap_size = 0; g_free(s->used_bmap); +s->used_bmap = NULL; } static int64_t coroutine_fn GRAPH_RDLOCK -- 2.34.1
[PATCH v3 21/21] tests: Add parallels format support to image-fleecing
Use a different bitmap name for parallels images because their has own ID
format, and can't contain an arbitrary string.
Replace hardcoded 'qcow2' format to iotests.imgfmt.
Add 'parallels' to supported formats.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/tests/image-fleecing | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/image-fleecing
b/tests/qemu-iotests/tests/image-fleecing
index 5e3b2c7e46..dd940b7203 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -28,7 +28,7 @@ import iotests
from iotests import log, qemu_img, qemu_io
iotests.script_initialize(
-supported_fmts=['qcow2'],
+supported_fmts=['qcow2', 'parallels'],
supported_platforms=['linux'],
required_fmts=['copy-before-write'],
unsupported_imgopts=['compat']
@@ -61,12 +61,17 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
if push_backup:
assert use_cbw
+if iotests.imgfmt == 'parallels':
+bitmap_name = '----'
+else:
+bitmap_name = 'bitmap0'
+
log('--- Setting up images ---')
log('')
qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
if bitmap:
-qemu_img('bitmap', '--add', base_img_path, 'bitmap0')
+qemu_img('bitmap', '--add', base_img_path, bitmap_name)
if use_snapshot_access_filter:
assert use_cbw
@@ -75,7 +80,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
qemu_img('create', '-f', 'qcow2', fleece_img_path, '64M')
if push_backup:
-qemu_img('create', '-f', 'qcow2', target_img_path, '64M')
+qemu_img('create', '-f', iotests.imgfmt, target_img_path, '64M')
for p in patterns:
qemu_io('-f', iotests.imgfmt,
@@ -130,7 +135,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
}
if bitmap:
-fl_cbw['bitmap'] = {'node': src_node, 'name': 'bitmap0'}
+fl_cbw['bitmap'] = {'node': src_node, 'name': bitmap_name}
log(vm.qmp('blockdev-add', fl_cbw))
--
2.34.1
[PATCH v3 03/21] parallels: Make mark_used() a global function
We will need this function and a function for marking unused clusters (will
be added in the next patch) in parallels-ext.c too. Let it be a global
function parallels_mark_used().
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 14 --
block/parallels.h | 3 +++
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 8962bc9fe5..e9a8cbe430 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,8 +178,8 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
-static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
- uint32_t bitmap_size, int64_t off, uint32_t count)
+int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
+uint32_t bitmap_size, int64_t off, uint32_t count)
{
BDRVParallelsState *s = bs->opaque;
uint32_t cluster_index = host_cluster_index(s, off);
@@ -232,7 +232,8 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
continue;
}
-err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
+err2 = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+ host_off, 1);
if (err2 < 0 && err == 0) {
err = err2;
}
@@ -366,7 +367,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
}
}
-ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off,
to_allocate);
+ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+ host_off, to_allocate);
if (ret < 0) {
/* Image consistency is broken. Alarm! */
return ret;
@@ -831,7 +833,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
+ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
assert(ret != -E2BIG);
if (ret == 0) {
continue;
@@ -891,7 +893,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
* considered, and the bitmap size doesn't change. This specifically
* means that -E2BIG is OK.
*/
-ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
+ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
if (ret == -EBUSY) {
res->check_errors++;
goto out_repair_bat;
diff --git a/block/parallels.h b/block/parallels.h
index 6b199443cf..bb18ee0527 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -90,6 +90,9 @@ typedef struct BDRVParallelsState {
Error *migration_blocker;
} BDRVParallelsState;
+int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
+uint32_t bitmap_size, int64_t off, uint32_t count);
+
int parallels_read_format_extension(BlockDriverState *bs,
int64_t ext_off, Error **errp);
--
2.34.1
[PATCH v3 08/21] parallels: Add a note about used bitmap in parallels_check_duplicate()
In parallels_check_duplicate() We use a bitmap for duplication detection. This bitmap is not related to used_bmap field in BDRVParallelsState. Add a comment about it to avoid confusion. Signed-off-by: Alexander Ivanov --- block/parallels.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index 2ce3e40ce3..481b6992f4 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -839,7 +839,10 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res, bool fixed = false; /* - * Create a bitmap of used clusters. + * Create a bitmap of used clusters. Please note that this bitmap is not + * related to used_bmap field in BDRVParallelsState and is created only for + * local usage. + * * If a bit is set, there is a BAT entry pointing to this cluster. * Loop through the BAT entries, check bits relevant to an entry offset. * If bit is set, this entry is duplicated. Otherwise set the bit. -- 2.34.1
[PATCH v3 07/21] parallels: Recreate used bitmap in parallels_check_leak()
In parallels_check_leak() file can be truncated. In this case the used
bitmap would not comply to the file. Recreate the bitmap after file
truncation.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index d6dbb6757f..2ce3e40ce3 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -809,6 +809,14 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
return ret;
}
s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+res->check_errors++;
+return ret;
+}
+
if (explicit) {
res->leaks_fixed += count;
}
--
2.34.1
[PATCH v3 11/21] parallels: Mark parallels_inactivate GRAPH_RDLOCK, guard parallels_close
Add GRAPH_RDLOCK annotation to declare parallels_inactivate() have to hold
a reader lock for the graph because it calls
bdrv_get_device_or_node_name(), which accesses the parents list of a node.
Assert we are in the main thread in parallels_close() and guard the code
with GRAPH_RDLOCK_GUARD_MAINLOOP().
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 2d82e8ff6a..4c2cb09e43 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1465,7 +1465,7 @@ fail:
return ret;
}
-static int parallels_inactivate(BlockDriverState *bs)
+static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
Error *err = NULL;
@@ -1491,10 +1491,13 @@ static int parallels_inactivate(BlockDriverState *bs)
return ret;
}
-static void parallels_close(BlockDriverState *bs)
+static void GRAPH_UNLOCKED parallels_close(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
+GLOBAL_STATE_CODE();
+GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE))
{
parallels_inactivate(bs);
}
--
2.34.1
[PATCH v3 17/21] parallels: Check unused clusters in parallels_check_leak()
Since we have used bitmap, leak check is useless. Transform
parallels_truncate_unused_clusters() to parallels_check_unused_clusters()
helper and use it in leak check.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 121 +-
1 file changed, 67 insertions(+), 54 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 472311e2e6..d497cdbe41 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -770,57 +770,87 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
return 0;
}
+static int64_t parallels_check_unused_clusters(BlockDriverState *bs,
+ bool truncate)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t leak, file_size, end_off = 0;
+int ret;
+
+file_size = bdrv_getlength(bs->file->bs);
+if (file_size < 0) {
+return file_size;
+}
+
+if (s->used_bmap_size > 0) {
+end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+if (end_off == s->used_bmap_size) {
+end_off = 0;
+} else {
+end_off = (end_off + 1) * s->cluster_size;
+}
+}
+
+end_off += s->data_start * BDRV_SECTOR_SIZE;
+leak = file_size - end_off;
+if (leak < 0) {
+return -EINVAL;
+}
+if (!truncate || leak == 0) {
+return leak;
+}
+
+ret = bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+if (ret) {
+return ret;
+}
+
+s->data_end = end_off / BDRV_SECTOR_SIZE;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret < 0) {
+return ret;
+}
+
+return leak;
+}
+
static int coroutine_fn GRAPH_RDLOCK
parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
BdrvCheckMode fix, bool explicit)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size, count;
-int ret;
+int64_t leak, count, size;
+
+leak = parallels_check_unused_clusters(bs, fix & BDRV_FIX_LEAKS);
+if (leak < 0) {
+res->check_errors++;
+return leak;
+}
+if (leak == 0) {
+return 0;
+}
size = bdrv_co_getlength(bs->file->bs);
if (size < 0) {
res->check_errors++;
return size;
}
-if (size <= res->image_end_offset) {
+res->image_end_offset = size;
+
+if (!explicit) {
return 0;
}
-count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-if (explicit) {
-fprintf(stderr,
-"%s space leaked at the end of the image %" PRId64 "\n",
-fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-size - res->image_end_offset);
-res->leaks += count;
-}
+count = DIV_ROUND_UP(leak, s->cluster_size);
+fprintf(stderr,
+"%s space leaked at the end of the image %" PRId64 "\n",
+fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak);
+res->leaks += count;
+
if (fix & BDRV_FIX_LEAKS) {
-Error *local_err = NULL;
-
-/*
- * In order to really repair the image, we must shrink it.
- * That means we have to pass exact=true.
- */
-ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
- PREALLOC_MODE_OFF, 0, _err);
-if (ret < 0) {
-error_report_err(local_err);
-res->check_errors++;
-return ret;
-}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-parallels_free_used_bitmap(bs);
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-res->check_errors++;
-return ret;
-}
-
-if (explicit) {
-res->leaks_fixed += count;
-}
+res->leaks_fixed += count;
}
return 0;
@@ -1456,23 +1486,6 @@ fail:
return ret;
}
-static int parallels_truncate_unused_clusters(BlockDriverState *bs)
-{
-BDRVParallelsState *s = bs->opaque;
-uint64_t end_off = 0;
-if (s->used_bmap_size > 0) {
-end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
-if (end_off == s->used_bmap_size) {
-end_off = 0;
-} else {
-end_off = (end_off + 1) * s->cluster_size;
-}
-}
-end_off += s->data_start * BDRV_SECTOR_SIZE;
-s->data_end = end_off / BDRV_SECTOR_SIZE;
-return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
-}
-
static int GRAPH_RDLOCK parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -1490,7 +1503,7 @@ static int GRAPH_RDLOCK
parallels_inactivate(BlockDriverState
[PATCH v3 05/21] parallels: Move host clusters allocation to a separate function
For parallels images extensions we need to allocate host clusters
without any connection to BAT. Move host clusters allocation code to
allocate_host_clusters().
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 128 --
block/parallels.h | 4 ++
2 files changed, 72 insertions(+), 60 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a30bb5fe0d..33bb8f1084 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -266,58 +266,31 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
s->used_bmap = NULL;
}
-static int64_t coroutine_fn GRAPH_RDLOCK
-allocate_clusters(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, int *pnum)
+int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
+ int64_t *clusters)
{
-int ret = 0;
BDRVParallelsState *s = bs->opaque;
-int64_t i, pos, idx, to_allocate, first_free, host_off;
-
-pos = block_status(s, sector_num, nb_sectors, pnum);
-if (pos > 0) {
-return pos;
-}
-
-idx = sector_num / s->tracks;
-to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
-
-/*
- * This function is called only by parallels_co_writev(), which will never
- * pass a sector_num at or beyond the end of the image (because the block
- * layer never passes such a sector_num to that function). Therefore, idx
- * is always below s->bat_size.
- * block_status() will limit *pnum so that sector_num + *pnum will not
- * exceed the image end. Therefore, idx + to_allocate cannot exceed
- * s->bat_size.
- * Note that s->bat_size is an unsigned int, therefore idx + to_allocate
- * will always fit into a uint32_t.
- */
-assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
+int64_t first_free, next_used, host_off, prealloc_clusters;
+int64_t bytes, prealloc_bytes;
+uint32_t new_usedsize;
+int ret = 0;
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-uint32_t new_usedsize;
-int64_t bytes = to_allocate * s->cluster_size;
-bytes += s->prealloc_size * BDRV_SECTOR_SIZE;
-
host_off = s->data_end * BDRV_SECTOR_SIZE;
+prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
+bytes = *clusters * s->cluster_size;
+prealloc_bytes = prealloc_clusters * s->cluster_size;
-/*
- * We require the expanded size to read back as zero. If the
- * user permitted truncation, we try that; but if it fails, we
- * force the safer-but-slower fallocate.
- */
if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-ret = bdrv_co_truncate(bs->file, host_off + bytes,
- false, PREALLOC_MODE_OFF,
- BDRV_REQ_ZERO_WRITE, NULL);
+ret = bdrv_truncate(bs->file, host_off + prealloc_bytes, false,
+PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE, NULL);
if (ret == -ENOTSUP) {
s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
}
}
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0);
+ret = bdrv_pwrite_zeroes(bs->file, host_off, prealloc_bytes, 0);
}
if (ret < 0) {
return ret;
@@ -327,15 +300,15 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
+if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
+s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
+}
} else {
-int64_t next_used;
next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
/* Not enough continuous clusters in the middle, adjust the size */
-if (next_used - first_free < to_allocate) {
-to_allocate = next_used - first_free;
-*pnum = (idx + to_allocate) * s->tracks - sector_num;
-}
+*clusters = MIN(*clusters, next_used - first_free);
+bytes = *clusters * s->cluster_size;
host_off = s->data_start * BDRV_SECTOR_SIZE;
host_off += first_free * s->cluster_size;
@@ -347,14 +320,59 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
host_off < s->data_end * BDRV_SECTOR_SIZE) {
-ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
-
[PATCH v3 15/21] parallels: Reverse a conditional in parallels_check_leak() to reduce indents
Let the function return a success code if a file size is not bigger than
image_end_offset. Thus we can decrease indents in the next code block.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 72 +++
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 0a2956b45f..354fa0f2cc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -775,7 +775,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
BdrvCheckMode fix, bool explicit)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size;
+int64_t size, count;
int ret;
size = bdrv_co_getlength(bs->file->bs);
@@ -783,43 +783,43 @@ parallels_check_leak(BlockDriverState *bs,
BdrvCheckResult *res,
res->check_errors++;
return size;
}
+if (size <= res->image_end_offset) {
+return 0;
+}
+
+count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
+if (explicit) {
+fprintf(stderr,
+"%s space leaked at the end of the image %" PRId64 "\n",
+fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
+size - res->image_end_offset);
+res->leaks += count;
+}
+if (fix & BDRV_FIX_LEAKS) {
+Error *local_err = NULL;
+
+/*
+ * In order to really repair the image, we must shrink it.
+ * That means we have to pass exact=true.
+ */
+ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
+ PREALLOC_MODE_OFF, 0, _err);
+if (ret < 0) {
+error_report_err(local_err);
+res->check_errors++;
+return ret;
+}
+s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+res->check_errors++;
+return ret;
+}
-if (size > res->image_end_offset) {
-int64_t count;
-count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
if (explicit) {
-fprintf(stderr,
-"%s space leaked at the end of the image %" PRId64 "\n",
-fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-size - res->image_end_offset);
-res->leaks += count;
-}
-if (fix & BDRV_FIX_LEAKS) {
-Error *local_err = NULL;
-
-/*
- * In order to really repair the image, we must shrink it.
- * That means we have to pass exact=true.
- */
-ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
- PREALLOC_MODE_OFF, 0, _err);
-if (ret < 0) {
-error_report_err(local_err);
-res->check_errors++;
-return ret;
-}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-parallels_free_used_bitmap(bs);
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-res->check_errors++;
-return ret;
-}
-
-if (explicit) {
-res->leaks_fixed += count;
-}
+res->leaks_fixed += count;
}
}
--
2.34.1
[PATCH v3 20/21] tests: Turned on 256, 299, 304 and block-status-cache for parallels format
These tests pass with parallels format. Add parallels to supporting
formats for these tests.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/256 | 2 +-
tests/qemu-iotests/299 | 2 +-
tests/qemu-iotests/304 | 2 +-
tests/qemu-iotests/tests/block-status-cache | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/256 b/tests/qemu-iotests/256
index f34af6cef7..1a4c9c6885 100755
--- a/tests/qemu-iotests/256
+++ b/tests/qemu-iotests/256
@@ -26,7 +26,7 @@ from iotests import log
iotests.verify_virtio_scsi_pci_or_ccw()
-iotests.script_initialize(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2', 'parallels'])
size = 64 * 1024 * 1024
with iotests.FilePath('img0') as img0_path, \
diff --git a/tests/qemu-iotests/299 b/tests/qemu-iotests/299
index a7122941fd..d8c4399446 100755
--- a/tests/qemu-iotests/299
+++ b/tests/qemu-iotests/299
@@ -23,7 +23,7 @@ import iotests
# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
iotests.script_initialize(
-supported_fmts=['qcow2'],
+supported_fmts=['qcow2', 'parallels'],
)
nbd_sock = iotests.file_path('nbd.sock', base_dir=iotests.sock_dir)
diff --git a/tests/qemu-iotests/304 b/tests/qemu-iotests/304
index 198f282087..1ebf30 100755
--- a/tests/qemu-iotests/304
+++ b/tests/qemu-iotests/304
@@ -23,7 +23,7 @@
import iotests
from iotests import qemu_img_create, qemu_img_log, file_path
-iotests.script_initialize(supported_fmts=['qcow2'],
+iotests.script_initialize(supported_fmts=['qcow2', 'parallels'],
supported_protocols=['file'])
test_img = file_path('test.qcow2')
diff --git a/tests/qemu-iotests/tests/block-status-cache
b/tests/qemu-iotests/tests/block-status-cache
index 5a7bc2c149..ade3d5b169 100755
--- a/tests/qemu-iotests/tests/block-status-cache
+++ b/tests/qemu-iotests/tests/block-status-cache
@@ -131,5 +131,5 @@ class TestBscWithNbd(iotests.QMPTestCase):
if __name__ == '__main__':
# The block-status cache only works on the protocol layer, so to test it,
# we can only use the raw format
-iotests.main(supported_fmts=['raw'],
+iotests.main(supported_fmts=['raw', 'parallels'],
supported_protocols=['file'])
--
2.34.1
[PATCH v3 02/21] parallels: Move inactivation code to a separate function
We are going to add parallels image extensions storage and need a separate
function for inactivation code.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 01a61a4ebd..8962bc9fe5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1428,18 +1428,27 @@ fail:
return ret;
}
+static int parallels_inactivate(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int ret;
+
+s->header->inuse = 0;
+parallels_update_header(bs);
+
+/* errors are ignored, so we might as well pass exact=true */
+ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
+PREALLOC_MODE_OFF, 0, NULL);
+
+return ret;
+}
static void parallels_close(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE))
{
-s->header->inuse = 0;
-parallels_update_header(bs);
-
-/* errors are ignored, so we might as well pass exact=true */
-bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
- PREALLOC_MODE_OFF, 0, NULL);
+parallels_inactivate(bs);
}
parallels_free_used_bitmap(bs);
@@ -1478,6 +1487,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_check = parallels_co_check,
.bdrv_co_pdiscard = parallels_co_pdiscard,
.bdrv_co_pwrite_zeroes = parallels_co_pwrite_zeroes,
+.bdrv_inactivate= parallels_inactivate,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PATCH v3 00/21] parallels: Add full dirty bitmap support
Parallels format driver: * make some preparation * add dirty bitmap saving * make dirty bitmap RW * fix broken checks * refactor leak check * add parallels format support to several tests You could find these patches in my repo: https://github.com/AlexanderIvanov-Virtuozzo/qemu/tree/parallels-v3 v3: 1: Fixed the order of g_free() and s->used_bmap = NULL. 3,4: Made mark_used() a global function before mark_unused() addition. In this way we can avoid compilation warnings. 5-9: Patches shifted. 11: Added GRAPH_RDLOCK annotation to parallels_inactivate(). Guard parallels_close() with GRAPH_RDLOCK_GUARD_MAINLOOP(). 12-21: Patches shifted. v2: 1: New patch to fix double free error. 4: Fixed clusters leaks. 15: Fixed (end_off != s->used_bmap_size) handling in parallels_truncate_unused_clusters(). 16,17: Changed the sequence of the patches - in this way we have correct leaks check. Alexander Ivanov (21): parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap() parallels: Move inactivation code to a separate function parallels: Make mark_used() a global function parallels: Add parallels_mark_unused() helper parallels: Move host clusters allocation to a separate function parallels: Set data_end value in parallels_check_leak() parallels: Recreate used bitmap in parallels_check_leak() parallels: Add a note about used bitmap in parallels_check_duplicate() parallels: Create used bitmap even if checks needed parallels: Add dirty bitmaps saving parallels: Mark parallels_inactivate GRAPH_RDLOCK, guard parallels_close parallels: Let image extensions work in RW mode parallels: Handle L1 entries equal to one parallels: Make a loaded dirty bitmap persistent parallels: Reverse a conditional in parallels_check_leak() to reduce indents parallels: Truncate images on the last used cluster parallels: Check unused clusters in parallels_check_leak() parallels: Remove unnecessary data_end field tests: Add parallels images support to test 165 tests: Turned on 256, 299, 304 and block-status-cache for parallels format tests: Add parallels format support to image-fleecing block/parallels-ext.c | 182 +- block/parallels.c | 366 block/parallels.h | 15 +- tests/qemu-iotests/165 | 40 ++- tests/qemu-iotests/256 | 2 +- tests/qemu-iotests/299 | 2 +- tests/qemu-iotests/304 | 2 +- tests/qemu-iotests/tests/block-status-cache | 2 +- tests/qemu-iotests/tests/image-fleecing | 13 +- 9 files changed, 451 insertions(+), 173 deletions(-) -- 2.34.1
[PATCH v3 10/21] parallels: Add dirty bitmaps saving
Now dirty bitmaps can be loaded but there is no their saving. Add code for
dirty bitmap storage.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 167 ++
block/parallels.c | 16 +++-
block/parallels.h | 5 ++
3 files changed, 186 insertions(+), 2 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 8a109f005a..0a632a2331 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "qapi/error.h"
#include "block/block-io.h"
#include "block/block_int.h"
@@ -301,3 +302,169 @@ out:
return ret;
}
+
+static void parallels_save_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
*bitmap,
+ uint8_t **buf, int *buf_size)
+{
+BDRVParallelsState *s = bs->opaque;
+ParallelsFeatureHeader *fh;
+ParallelsDirtyBitmapFeature *bh;
+uint64_t *l1_table, l1_size, granularity, limit;
+int64_t bm_size, ser_size, offset, buf_used;
+int64_t alloc_size = 1;
+const char *name;
+uint8_t *bm_buf;
+QemuUUID uuid;
+int ret = 0;
+
+if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
+bdrv_dirty_bitmap_inconsistent(bitmap)) {
+return;
+}
+
+bm_size = bdrv_dirty_bitmap_size(bitmap);
+granularity = bdrv_dirty_bitmap_granularity(bitmap);
+limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
+ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size);
+l1_size = DIV_ROUND_UP(ser_size, s->cluster_size);
+
+buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh);
+/* Check if there is enough space for the final section */
+if (*buf_size - buf_used < sizeof(*fh)) {
+return;
+}
+
+name = bdrv_dirty_bitmap_name(bitmap);
+ret = qemu_uuid_parse(name, );
+if (ret < 0) {
+error_report("Can't save dirty bitmap: ID parsing error: '%s'", name);
+return;
+}
+
+fh = (ParallelsFeatureHeader *)*buf;
+bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh));
+l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
+
+fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
+fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
+
+bh->l1_size = cpu_to_le32(l1_size);
+bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS);
+bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS);
+memcpy(bh->id, , sizeof(uuid));
+
+bm_buf = qemu_blockalign(bs, s->cluster_size);
+
+offset = 0;
+while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >=
0) {
+uint64_t idx = offset / limit;
+int64_t cluster_off, end, write_size;
+
+offset = QEMU_ALIGN_DOWN(offset, limit);
+end = MIN(bm_size, offset + limit);
+write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
+ end - offset);
+assert(write_size <= s->cluster_size);
+
+bdrv_dirty_bitmap_serialize_part(bitmap, bm_buf, offset, end - offset);
+if (write_size < s->cluster_size) {
+memset(bm_buf + write_size, 0, s->cluster_size - write_size);
+}
+
+cluster_off = parallels_allocate_host_clusters(bs, _size);
+if (cluster_off <= 0) {
+goto end;
+}
+
+ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, bm_buf, 0);
+if (ret < 0) {
+memset(>magic, 0, sizeof(fh->magic));
+parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+ cluster_off, 1);
+goto end;
+}
+
+l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS);
+offset = end;
+}
+
+*buf_size -= buf_used;
+*buf += buf_used;
+
+end:
+qemu_vfree(bm_buf);
+}
+
+void parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+ Error **errp)
+{
+BDRVParallelsState *s = bs->opaque;
+BdrvDirtyBitmap *bitmap;
+ParallelsFormatExtensionHeader *eh;
+int remaining = s->cluster_size;
+uint8_t *buf, *pos;
+int64_t header_off, alloc_size = 1;
+g_autofree uint8_t *hash = NULL;
+size_t hash_len = 0;
+int ret;
+
+s->header->ext_off = 0;
+
+if (!bdrv_has_named_bitmaps(bs)) {
+return;
+}
+
+buf = qemu_blockalign0(bs, s->cluster_size);
+
+eh = (ParallelsFormatExtensionHeader *)buf;
+pos = buf + sizeof(*eh);
+
+eh->magic = cpu_to_le64(PARALLELS_FORMAT_EXTENSION_MAGIC);
+
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+parallels_save_bitmap(bs, bitmap, , );
+}
+
+header_off = parallels_allocate_host_clusters(bs, _size);
+if (head
[PATCH v3 19/21] tests: Add parallels images support to test 165
Use a different bitmap name for parallels images because their has own ID
format, and can't contain an arbitrary string.
Replace image reopen by shutdown/launch VM because parallels images doesn't
support reopen.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/165 | 40 +---
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index b24907a62f..f732db257c 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -38,6 +38,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+if iotests.imgfmt == 'parallels':
+self.bitmap_name = '----'
+else:
+self.bitmap_name = 'bitmap0'
def tearDown(self):
os.remove(disk)
@@ -50,12 +54,12 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def getSha256(self):
result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
- node='drive0', name='bitmap0')
+ node='drive0', name=self.bitmap_name)
return result['return']['sha256']
def checkBitmap(self, sha256):
result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
- node='drive0', name='bitmap0')
+ node='drive0', name=self.bitmap_name)
self.assert_qmp(result, 'return/sha256', sha256);
def writeRegions(self, regions):
@@ -65,7 +69,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def qmpAddBitmap(self):
self.vm.qmp('block-dirty-bitmap-add', node='drive0',
-name='bitmap0', persistent=True)
+name=self.bitmap_name, persistent=True)
def test_persistent(self):
self.vm = self.mkVm()
@@ -117,7 +121,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
assert sha256_1 != sha256_2 # Otherwise, it's not very interesting.
self.vm.cmd('block-dirty-bitmap-clear', node='drive0',
-name='bitmap0')
+name=self.bitmap_name)
# Start with regions1
@@ -135,16 +139,22 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
self.writeRegions(regions2)
assert sha256_1 == self.getSha256()
-# Reopen to RW
-self.vm.cmd('blockdev-reopen', options=[{
-'node-name': 'node0',
-'driver': iotests.imgfmt,
-'file': {
-'driver': 'file',
-'filename': disk
-},
-'read-only': False
-}])
+if iotests.imgfmt == 'parallels':
+# parallels doesn't support reopen
+self.vm.shutdown()
+self.vm = self.mkVm()
+self.vm.launch()
+else:
+# Reopen to RW
+self.vm.cmd('blockdev-reopen', options=[{
+'node-name': 'node0',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': disk
+},
+'read-only': False
+}])
# Check that bitmap is reopened to RW and we can write to it.
self.writeRegions(regions2)
@@ -154,6 +164,6 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
if __name__ == '__main__':
-iotests.main(supported_fmts=['qcow2'],
+iotests.main(supported_fmts=['qcow2', 'parallels'],
supported_protocols=['file'],
unsupported_imgopts=['compat'])
--
2.34.1
[PATCH v3 04/21] parallels: Add parallels_mark_unused() helper
Add a helper to set unused areas in the used bitmap.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 17 +
block/parallels.h | 2 ++
2 files changed, 19 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index e9a8cbe430..a30bb5fe0d 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -195,6 +195,23 @@ int parallels_mark_used(BlockDriverState *bs, unsigned
long *bitmap,
return 0;
}
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_index = host_cluster_index(s, off);
+unsigned long next_unused;
+if (cluster_index + count > bitmap_size) {
+return -E2BIG;
+}
+next_unused = find_next_zero_bit(bitmap, bitmap_size, cluster_index);
+if (next_unused < cluster_index + count) {
+return -EINVAL;
+}
+bitmap_clear(bitmap, cluster_index, count);
+return 0;
+}
+
/*
* Collect used bitmap. The image can contain errors, we should fill the
* bitmap anyway, as much as we can. This information will be used for
diff --git a/block/parallels.h b/block/parallels.h
index bb18ee0527..31ebbd6846 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -92,6 +92,8 @@ typedef struct BDRVParallelsState {
int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
uint32_t bitmap_size, int64_t off, uint32_t count);
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count);
int parallels_read_format_extension(BlockDriverState *bs,
int64_t ext_off, Error **errp);
--
2.34.1
[PATCH v3 06/21] parallels: Set data_end value in parallels_check_leak()
In parallels_check_leak() we change file size but don't correct data_end
field of BDRVParallelsState structure. Fix it.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index 33bb8f1084..d6dbb6757f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -808,6 +808,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
res->check_errors++;
return ret;
}
+s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
if (explicit) {
res->leaks_fixed += count;
}
--
2.34.1
[PATCH v3 13/21] parallels: Handle L1 entries equal to one
If all the bits in a dirty bitmap cluster are ones, the cluster shouldn't
be written. Instead the corresponding L1 entry should be set to 1.
Check if all bits in a memory region are ones and set 1 to L1 entries
corresponding clusters filled with ones.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index a0de4f1b07..ebda6b0a01 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void parallels_save_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
offset = 0;
while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >=
0) {
uint64_t idx = offset / limit;
-int64_t cluster_off, end, write_size;
+int64_t cluster_off, end, write_size, first_zero;
offset = QEMU_ALIGN_DOWN(offset, limit);
end = MIN(bm_size, offset + limit);
@@ -367,6 +367,16 @@ static void parallels_save_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
memset(bm_buf + write_size, 0, s->cluster_size - write_size);
}
+first_zero = bdrv_dirty_bitmap_next_zero(bitmap, offset, bm_size);
+if (first_zero < 0) {
+goto end;
+}
+if (first_zero - offset >= s->cluster_size) {
+l1_table[idx] = 1;
+offset = end;
+continue;
+}
+
cluster_off = parallels_allocate_host_clusters(bs, _size);
if (cluster_off <= 0) {
goto end;
--
2.34.1
[PATCH v3 14/21] parallels: Make a loaded dirty bitmap persistent
After bitmap loading the bitmap is not persistent and is removed on image
saving. Set bitmap persistence to true.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index ebda6b0a01..bb4478c350 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -256,6 +256,7 @@ static int
parallels_parse_format_extension(BlockDriverState *bs,
if (!bitmap) {
goto fail;
}
+bdrv_dirty_bitmap_set_persistence(bitmap, true);
bitmaps = g_slist_append(bitmaps, bitmap);
break;
--
2.34.1
Re: [PATCH 5/6] qga: Add timeout for fsfreeze
On 10/26/23 11:16, Konstantin Kostiuk wrote:
I think it is better to check that timeout <= 10 sec in the case of
Windows.
Anyway this is a VSS limitation and FS will be unfrozen earlier if
timeout > 10 sec,
this can cause some misunderstanding from a user.
Thank you, will pay attention.
timeout option sounds good in the guest-fsfreeze-freeze command.
In guest-fsfreeze-freeze-list, it looks strange to me. Command returns
list but takes timeout option. Can you explain timeout usage in this
command?
The second command doesn't return a list, it takes a list of mountpoints.
Both of the commands freeze local guest filesystems. The first one
freezes all the FS,
the second one freeze only FS from the given list.
On Wed, Oct 25, 2023 at 5:01 PM Alexander Ivanov
wrote:
In some cases it would be useful to thaw a filesystem by timeout after
freezing this filesystem by guest-fsfreeze-freeze-list. Add an
optional
argument "timeout" to the command.
Signed-off-by: Alexander Ivanov
---
qga/commands-posix.c | 21 ++---
qga/commands-win32.c | 16 ++--
qga/guest-agent-core.h | 3 ++-
qga/main.c | 19 ++-
qga/qapi-schema.json | 9 -
5 files changed, 60 insertions(+), 8 deletions(-)
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 26711a1a72..e8a79e0a41 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -707,13 +707,17 @@ GuestFsfreezeStatus
qmp_guest_fsfreeze_status(Error **errp)
return GUEST_FSFREEZE_STATUS_THAWED;
}
-int64_t qmp_guest_fsfreeze_freeze(Error **errp)
+int64_t qmp_guest_fsfreeze_freeze(bool has_timeout, int64_t timeout,
+ Error **errp)
{
- return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
+ return qmp_guest_fsfreeze_freeze_list(false, NULL,
has_timeout, timeout,
+ errp);
}
int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
strList *mountpoints,
+ bool has_timeout,
+ int64_t timeout,
Error **errp)
{
int ret;
@@ -734,8 +738,11 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
has_mountpoints,
return -1;
}
+ if (!has_timeout || timeout < 0) {
+ timeout = 0;
+ }
/* cannot risk guest agent blocking itself on a write in this
state */
- ga_set_frozen(ga_state);
+ ga_set_frozen(ga_state, timeout);
ret = qmp_guest_fsfreeze_do_freeze_list(has_mountpoints,
mountpoints,
mounts, errp);
@@ -780,6 +787,12 @@ static void guest_fsfreeze_cleanup(void)
}
}
}
+
+gboolean ga_frozen_timeout_cb(gpointer data)
+{
+ guest_fsfreeze_cleanup();
+ return G_SOURCE_REMOVE;
+}
#endif
/* linux-specific implementations. avoid this if at all possible. */
@@ -3119,6 +3132,8 @@ int64_t qmp_guest_fsfreeze_freeze(Error **errp)
int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
strList *mountpoints,
+ bool has_timeout,
+ int64_t timeout,
Error **errp)
{
error_setg(errp, QERR_UNSUPPORTED);
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 618d862c00..51fd6dcd58 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1221,13 +1221,16 @@ GuestFsfreezeStatus
qmp_guest_fsfreeze_status(Error **errp)
* Freeze local file systems using Volume Shadow-copy Service.
* The frozen state is limited for up to 10 seconds by VSS.
*/
-int64_t qmp_guest_fsfreeze_freeze(Error **errp)
+int64_t qmp_guest_fsfreeze_freeze(bool has_timeout, int64_t timeout,
+ Error **errp)
{
return qmp_guest_fsfreeze_freeze_list(false, NULL, errp);
}
int64_t qmp_guest_fsfreeze_freeze_list(bool has_mountpoints,
strList *mountpoints,
+ bool has_timeout,
+ int64_t timeout,
Error **errp)
{
int i;
@@ -1240,8 +1243,11 @@ int64_t qmp_guest_fsfreeze_freeze_list(bool
has_mountpoints,
slog("guest-fsfreeze called");
+ if (!has_timeout || timeout < 0) {
+ timeout = 0;
+ }
/* cannot risk guest agent blocking itself on a write in this
state */
Re: [PATCH 09/21] parallels: fix broken parallels_check_data_off()
On 9/15/23 20:41, Denis V. Lunev wrote:
Once we have repaired data_off field in the header we should update
s->data_start which is calculated on the base of it.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index 60ad41b49b..bdc4dd081b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -531,6 +531,7 @@ parallels_check_data_off(BlockDriverState *bs,
BdrvCheckResult *res,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
s->header->data_off = cpu_to_le32(data_off);
+s->data_start = data_off;
res->corruptions_fixed++;
}
Reviewed-by: Alexander Ivanov
Re: [PATCH 11/21] parallels: collect bitmap of used clusters at open
On 9/15/23 20:41, Denis V. Lunev wrote:
If the operation is failed, we need to check image consistency if the
problem is not about memory allocation.
Bitmap adjustments in allocate_cluster are not performed yet.
They worth to be separate. This was proven useful during debug of this
series. Kept as is for future bissecting.
It should be specifically noted that used bitmap must be recalculated
if data_off has been fixed during image consistency check.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 73 +++
block/parallels.h | 3 ++
2 files changed, 76 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index bdc4dd081b..2517f35581 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -193,6 +193,58 @@ static int mark_used(BlockDriverState *bs,
return 0;
}
+/*
+ * Collect used bitmap. The image can contain errors, we should fill the
+ * bitmap anyway, as much as we can. This information will be used for
+ * error resolution.
+ */
+static int parallels_fill_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t payload_bytes;
+uint32_t i;
+int err = 0;
+
+payload_bytes = bdrv_co_getlength(bs->file->bs);
+if (payload_bytes < 0) {
+return payload_bytes;
+}
+payload_bytes -= s->data_start * BDRV_SECTOR_SIZE;
+if (payload_bytes < 0) {
+return -EINVAL;
+}
+
+s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
+if (s->used_bmap_size == 0) {
+return 0;
+}
+s->used_bmap = bitmap_try_new(s->used_bmap_size);
+if (s->used_bmap == NULL) {
+return -ENOMEM;
+}
+
+for (i = 0; i < s->bat_size; i++) {
+int err2;
+int64_t host_off = bat2sect(s, i) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+if (err2 < 0 && err == 0) {
+err = err2;
+}
+}
+return err;
+}
+
+static void parallels_free_used_bitmap(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+s->used_bmap_size = 0;
+g_free(s->used_bmap);
+}
+
static int64_t coroutine_fn GRAPH_RDLOCK
allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
@@ -530,8 +582,17 @@ parallels_check_data_off(BlockDriverState *bs,
BdrvCheckResult *res,
res->corruptions++;
if (fix & BDRV_FIX_ERRORS) {
+int err;
s->header->data_off = cpu_to_le32(data_off);
s->data_start = data_off;
+
+parallels_free_used_bitmap(bs);
+err = parallels_fill_used_bitmap(bs);
+if (err == -ENOMEM) {
+res->check_errors++;
+return err;
+}
+
res->corruptions_fixed++;
}
@@ -1214,6 +1275,14 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
}
need_check = need_check || s->data_end > file_nb_sectors;
+if (!need_check) {
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
+}
+need_check = need_check || ret < 0; /* These are correctable errors */
+}
+
/*
* We don't repair the image here if it's opened for checks. Also we don't
* want to change inactive images and can't change readonly images.
@@ -1243,6 +1312,8 @@ fail:
* "s" object was allocated by g_malloc0 so we can safely
* try to free its fields even they were not allocated.
*/
+parallels_free_used_bitmap(bs);
+
error_free(s->migration_blocker);
g_free(s->bat_dirty_bmap);
qemu_vfree(s->header);
@@ -1263,6 +1334,8 @@ static void parallels_close(BlockDriverState *bs)
PREALLOC_MODE_OFF, 0, NULL);
}
+parallels_free_used_bitmap(bs);
+
g_free(s->bat_dirty_bmap);
qemu_vfree(s->header);
diff --git a/block/parallels.h b/block/parallels.h
index 4e53e9572d..6b199443cf 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -72,6 +72,9 @@ typedef struct BDRVParallelsState {
unsigned long *bat_dirty_bmap;
unsigned int bat_dirty_block;
+unsigned long *used_bmap;
+unsigned long used_bmap_size;
+
uint32_t *bat_bitmap;
unsigned int bat_size;
Reviewed-by: Alexander Ivanov
Re: [PATCH 04/22] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts
On 9/18/23 20:00, Denis V. Lunev wrote:
This patch creates above mentioned helper and moves its usage to the
beginning of parallels_open(). This simplifies parallels_open() a bit.
The patch also ensures that we store prealloc_size on block driver state
always in sectors. This makes code cleaner and avoids wrong opinion at
the assignment that the value is in bytes.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 72 +--
1 file changed, 44 insertions(+), 28 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index af7be427c9..ae006e7fc7 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1025,6 +1025,44 @@ static int parallels_update_header(BlockDriverState *bs)
return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
}
+
+static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
+ Error **errp)
+{
+int err;
+char *buf;
+int64_t bytes;
+BDRVParallelsState *s = bs->opaque;
+Error *local_err = NULL;
+QemuOpts *opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
+if (!opts) {
+return -ENOMEM;
+}
+
+err = -EINVAL;
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
+goto done;
+}
+
+bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
+s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
+buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
+/* prealloc_mode can be downgraded later during allocate_clusters */
+s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
+ PRL_PREALLOC_MODE_FALLOCATE,
+ _err);
+g_free(buf);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+goto done;
+}
+err = 0;
+
+done:
+qemu_opts_del(opts);
+return err;
+}
+
static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -1033,11 +1071,13 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-QemuOpts *opts = NULL;
-Error *local_err = NULL;
-char *buf;
bool data_off_is_correct;
+ret = parallels_opts_prealloc(bs, options, errp);
+if (ret < 0) {
+return ret;
+}
+
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
@@ -1078,6 +1118,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = -EFBIG;
goto fail;
}
+s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
s->bat_size = le32_to_cpu(ph.bat_entries);
@@ -1117,29 +1158,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->header_size = size;
}
-opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
-if (!opts) {
-goto fail_options;
-}
-
-if (!qemu_opts_absorb_qdict(opts, options, errp)) {
-goto fail_options;
-}
-
-s->prealloc_size =
-qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
-s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
-buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
-/* prealloc_mode can be downgraded later during allocate_clusters */
-s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
- PRL_PREALLOC_MODE_FALLOCATE,
- _err);
-g_free(buf);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-goto fail_options;
-}
-
if (ph.ext_off) {
if (flags & BDRV_O_RDWR) {
/*
@@ -1214,10 +1232,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
fail_format:
error_setg(errp, "Image not in Parallels format");
-fail_options:
ret = -EINVAL;
fail:
-qemu_opts_del(opts);
/*
* "s" object was allocated by g_malloc0 so we can safely
* try to free its fields even they were not allocated.
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH 19/22] parallels: naive implementation of parallels_co_pdiscard
On 9/18/23 20:00, Denis V. Lunev wrote:
* Discarding with backing stores is not supported by the format.
* There is no buffering/queueing of the discard operation.
* Only operations aligned to the cluster are supported.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 46 ++
1 file changed, 46 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index d9d36c514b..1ef23f6669 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -537,6 +537,51 @@ parallels_co_readv(BlockDriverState *bs, int64_t
sector_num, int nb_sectors,
return ret;
}
+
+static int coroutine_fn
+parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+{
+int ret = 0;
+uint32_t cluster, count;
+BDRVParallelsState *s = bs->opaque;
+
+/*
+ * The image does not support ZERO mark inside the BAT, which means that
+ * stale data could be exposed from the backing file.
+ */
+if (bs->backing) {
+return -ENOTSUP;
+}
+
+if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) {
+return -ENOTSUP;
+} else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) {
+return -ENOTSUP;
+}
+
+cluster = offset / s->cluster_size;
+count = bytes / s->cluster_size;
+
+qemu_co_mutex_lock(>lock);
+for (; count > 0; cluster++, count--) {
+int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+ret = bdrv_co_pdiscard(bs->file, host_off, s->cluster_size);
+if (ret < 0) {
+goto done;
+}
+
+parallels_set_bat_entry(s, cluster, 0);
+bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1);
+}
+done:
+qemu_co_mutex_unlock(>lock);
+return ret;
+}
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1417,6 +1462,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create = parallels_co_create,
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
+.bdrv_co_pdiscard = parallels_co_pdiscard,
};
static void bdrv_parallels_init(void)
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH 20/22] tests: extend test 131 to cover availability of the discard operation
at offset 2097152 +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +discard 1048576/1048576 bytes at offset 2097152 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1048576/1048576 bytes at offset 524288 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0 0x10TEST_DIR/t.IMGFMT +0x100x10TEST_DIR/t.IMGFMT +0x300x10TEST_DIR/t.IMGFMT +read 1048576/1048576 bytes at offset 524288 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 524288/524288 bytes at offset 0 +512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 524288/524288 bytes at offset 1572864 +512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == allocate with backing == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 Reviewed-by: Alexander Ivanov -- Best regards, Alexander Ivanov
Re: [PATCH 03/22] parallels: fix memory leak in parallels_open()
On 9/18/23 20:00, Denis V. Lunev wrote: We should free opts allocated through qemu_opts_create() at the end. Signed-off-by: Denis V. Lunev --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 428f72de1c..af7be427c9 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1217,6 +1217,7 @@ fail_format: fail_options: ret = -EINVAL; fail: +qemu_opts_del(opts); /* * "s" object was allocated by g_malloc0 so we can safely * try to free its fields even they were not allocated. Reviewed-by: Alexander Ivanov -- Best regards, Alexander Ivanov
Re: [PATCH 03/21] parallels: invent parallels_opts_prealloc() helper to parse prealloc opts
On 9/15/23 20:41, Denis V. Lunev wrote:
This patch creates above mentioned helper and moves its usage to the
beginning of parallels_open(). This simplifies parallels_open() a bit.
The patch also ensures that we store prealloc_size on block driver state
always in sectors. This makes code cleaner and avoids wrong opinion at
the assignment that the value is in bytes.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 65 +++
1 file changed, 38 insertions(+), 27 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 428f72de1c..1d5409f2ba 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1025,6 +1025,38 @@ static int parallels_update_header(BlockDriverState *bs)
return bdrv_pwrite_sync(bs->file, 0, size, s->header, 0);
}
+
+static int parallels_opts_prealloc(BlockDriverState *bs, QDict *options,
+ Error **errp)
+{
+char *buf;
+int64_t bytes;
+BDRVParallelsState *s = bs->opaque;
+Error *local_err = NULL;
+QemuOpts *opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
+if (!opts) {
+return -ENOMEM;
+}
+
+if (!qemu_opts_absorb_qdict(opts, options, errp)) {
+return -EINVAL;
+}
+
+bytes = qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
+s->prealloc_size = bytes >> BDRV_SECTOR_BITS;
+buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
+/* prealloc_mode can be downgraded later during allocate_clusters */
+s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
+ PRL_PREALLOC_MODE_FALLOCATE,
+ _err);
+g_free(buf);
+if (local_err != NULL) {
+error_propagate(errp, local_err);
+return -EINVAL;
+}
+return 0;
+}
+
static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -1033,11 +1065,13 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-QemuOpts *opts = NULL;
-Error *local_err = NULL;
-char *buf;
bool data_off_is_correct;
+ret = parallels_opts_prealloc(bs, options, errp);
+if (ret < 0) {
+return ret;
+}
+
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
@@ -1078,6 +1112,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = -EFBIG;
goto fail;
}
+s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
s->bat_size = le32_to_cpu(ph.bat_entries);
@@ -1117,29 +1152,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->header_size = size;
}
-opts = qemu_opts_create(_runtime_opts, NULL, 0, errp);
-if (!opts) {
-goto fail_options;
-}
-
-if (!qemu_opts_absorb_qdict(opts, options, errp)) {
-goto fail_options;
-}
-
-s->prealloc_size =
-qemu_opt_get_size_del(opts, PARALLELS_OPT_PREALLOC_SIZE, 0);
-s->prealloc_size = MAX(s->tracks, s->prealloc_size >> BDRV_SECTOR_BITS);
-buf = qemu_opt_get_del(opts, PARALLELS_OPT_PREALLOC_MODE);
-/* prealloc_mode can be downgraded later during allocate_clusters */
-s->prealloc_mode = qapi_enum_parse(_mode_lookup, buf,
- PRL_PREALLOC_MODE_FALLOCATE,
- _err);
-g_free(buf);
-if (local_err != NULL) {
-error_propagate(errp, local_err);
-goto fail_options;
-}
-
if (ph.ext_off) {
if (flags & BDRV_O_RDWR) {
/*
@@ -1214,7 +1226,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
fail_format:
error_setg(errp, "Image not in Parallels format");
-fail_options:
ret = -EINVAL;
fail:
/*
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH 19/21] tests: extend test 131 to cover availability of the discard operation
fset 2097152 +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +discard 1048576/1048576 bytes at offset 2097152 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 1048576/1048576 bytes at offset 524288 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +Offset Length File +0 0x10TEST_DIR/t.IMGFMT +0x100x10TEST_DIR/t.IMGFMT +0x300x10TEST_DIR/t.IMGFMT +read 1048576/1048576 bytes at offset 524288 +1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 524288/524288 bytes at offset 0 +512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +read 2097152/2097152 bytes at offset 1572864 +2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) == allocate with backing == Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864 LGTM, but it didn't detect incorrect discarding. -- Best regards, Alexander Ivanov
Re: [PATCH 17/21] parallels: improve readability of allocate_clusters
On 9/15/23 20:41, Denis V. Lunev wrote:
Replace 'space' representing the amount of data to preallocate with
'bytes'.
Rationale:
* 'space' at each place is converted to bytes
* the unit is more close to the variable name
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 2efa578e21..76aedfd7c4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -279,7 +279,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
uint32_t new_usedsize;
-int64_t space = to_allocate * s->tracks + s->prealloc_size;
+int64_t bytes = to_allocate * s->cluster_size;
+bytes += s->prealloc_size * BDRV_SECTOR_SIZE;
host_off = s->data_end * BDRV_SECTOR_SIZE;
@@ -289,8 +290,7 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
* force the safer-but-slower fallocate.
*/
if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-ret = bdrv_co_truncate(bs->file,
- (s->data_end + space) << BDRV_SECTOR_BITS,
+ret = bdrv_co_truncate(bs->file, host_off + bytes,
false, PREALLOC_MODE_OFF,
BDRV_REQ_ZERO_WRITE, NULL);
if (ret == -ENOTSUP) {
@@ -298,16 +298,13 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
}
}
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-ret = bdrv_co_pwrite_zeroes(bs->file,
-s->data_end << BDRV_SECTOR_BITS,
-space << BDRV_SECTOR_BITS, 0);
+ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0);
}
if (ret < 0) {
return ret;
}
-new_usedsize = s->used_bmap_size +
- (space << BDRV_SECTOR_BITS) / s->cluster_size;
+new_usedsize = s->used_bmap_size + bytes / s->cluster_size;
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
Reviewed-by: Alexander Ivanov
Re: [PATCH 18/21] parallels: naive implementation of parallels_co_pdiscard
On 9/15/23 20:41, Denis V. Lunev wrote:
* Discarding with backing stores is not supported by the format.
* There is no buffering/queueing of the discard operation.
* Only operations aligned to the cluster are supported.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 47 +++
1 file changed, 47 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 76aedfd7c4..83cb8d6722 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -537,6 +537,52 @@ parallels_co_readv(BlockDriverState *bs, int64_t
sector_num, int nb_sectors,
return ret;
}
+
+static int coroutine_fn GRAPH_RDLOCK_PTR
+parallels_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
+{
+int ret = 0;
+uint32_t cluster, count;
+BDRVParallelsState *s = bs->opaque;
+
+/*
+ * The image does not support ZERO mark inside the BAT, which means that
+ * stale data could be exposed from the backing file.
+ */
+if (bs->backing) {
+return -ENOTSUP;
+}
+
+if (!QEMU_IS_ALIGNED(offset, s->cluster_size)) {
+return -ENOTSUP;
+} else if (!QEMU_IS_ALIGNED(bytes, s->cluster_size)) {
+return -ENOTSUP;
+}
+
+cluster = offset / s->cluster_size;
+count = bytes / s->cluster_size;
+
+qemu_co_mutex_lock(>lock);
+for (; count > 0; cluster++, count--) {
+int64_t host_off = bat2sect(s, cluster) << BDRV_SECTOR_BITS;
+if (host_off == 0) {
+continue;
+}
+
+ret = bdrv_co_pdiscard(bs->file, cluster * s->cluster_size,
+ s->cluster_size);
It seems, bdrv_co_pdiscard() should be called with a host offset, but
there is a guest one.
+if (ret < 0) {
+goto done;
+}
+
+parallels_set_bat_entry(s, cluster, 0);
+bitmap_clear(s->used_bmap, host_cluster_index(s, host_off), 1);
+}
+done:
+qemu_co_mutex_unlock(>lock);
+return ret;
+}
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1409,6 +1455,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create = parallels_co_create,
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
+.bdrv_co_pdiscard = parallels_co_pdiscard,
};
static void bdrv_parallels_init(void)
--
Best regards,
Alexander Ivanov
Re: [PATCH 16/21] parallels: naive implementation of allocate_clusters with used bitmap
On 9/15/23 20:41, Denis V. Lunev wrote:
The access to the bitmap is not optimized completely.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 51 ---
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a6d2f05863..2efa578e21 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -253,7 +253,7 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
{
int ret = 0;
BDRVParallelsState *s = bs->opaque;
-int64_t pos, space, idx, to_allocate, i, len;
+int64_t i, pos, idx, to_allocate, first_free, host_off;
pos = block_status(s, sector_num, nb_sectors, pnum);
if (pos > 0) {
@@ -276,15 +276,13 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
-space = to_allocate * s->tracks;
-len = bdrv_co_getlength(bs->file->bs);
-if (len < 0) {
-return len;
-}
-if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
+if (first_free == s->used_bmap_size) {
uint32_t new_usedsize;
+int64_t space = to_allocate * s->tracks + s->prealloc_size;
+
+host_off = s->data_end * BDRV_SECTOR_SIZE;
-space += s->prealloc_size;
/*
* We require the expanded size to read back as zero. If the
* user permitted truncation, we try that; but if it fails, we
@@ -313,6 +311,32 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
+} else {
+int64_t next_used;
+next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
+
+/* Not enough continuous clusters in the middle, adjust the size */
+if (next_used - first_free < to_allocate) {
+to_allocate = next_used - first_free;
+*pnum = (idx + to_allocate) * s->tracks - sector_num;
It looks, we can write this simplier:
*pnum = to_allocate * s->tracks;
because idx and sector_num aren't changed from idx calculation:
idx = sector_num / s->tracks;
+}
+
+host_off = s->data_start * BDRV_SECTOR_SIZE;
+host_off += first_free * s->cluster_size;
+
+/*
+ * No need to preallocate if we are using tail area from the above
+ * branch. In the other case we are likely re-using hole. Preallocate
+ * the space if required by the prealloc_mode.
+ */
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
+host_off < s->data_end * BDRV_SECTOR_SIZE) {
+ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
+s->cluster_size * to_allocate, 0);
+if (ret < 0) {
+return ret;
+}
+}
}
/*
@@ -344,15 +368,18 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
}
}
-ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
-s->data_end << BDRV_SECTOR_BITS, to_allocate);
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off,
to_allocate);
if (ret < 0) {
/* Image consistency is broken. Alarm! */
return ret;
}
for (i = 0; i < to_allocate; i++) {
-parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
-s->data_end += s->tracks;
+parallels_set_bat_entry(s, idx + i,
+host_off / BDRV_SECTOR_SIZE / s->off_multiplier);
+host_off += s->cluster_size;
+}
+if (host_off > s->data_end * BDRV_SECTOR_SIZE) {
+s->data_end = host_off / BDRV_SECTOR_SIZE;
}
return bat2sect(s, idx) + sector_num % s->tracks;
--
Best regards,
Alexander Ivanov
Re: [PATCH 16/21] parallels: naive implementation of allocate_clusters with used bitmap
On 9/18/23 15:14, Denis V. Lunev wrote:
On 9/18/23 15:09, Alexander Ivanov wrote:
On 9/15/23 20:41, Denis V. Lunev wrote:
The access to the bitmap is not optimized completely.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 51
---
1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index a6d2f05863..2efa578e21 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -253,7 +253,7 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
{
int ret = 0;
BDRVParallelsState *s = bs->opaque;
- int64_t pos, space, idx, to_allocate, i, len;
+ int64_t i, pos, idx, to_allocate, first_free, host_off;
pos = block_status(s, sector_num, nb_sectors, pnum);
if (pos > 0) {
@@ -276,15 +276,13 @@ allocate_clusters(BlockDriverState *bs,
int64_t sector_num,
*/
assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
- space = to_allocate * s->tracks;
- len = bdrv_co_getlength(bs->file->bs);
- if (len < 0) {
- return len;
- }
- if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+ first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
+ if (first_free == s->used_bmap_size) {
uint32_t new_usedsize;
+ int64_t space = to_allocate * s->tracks + s->prealloc_size;
+
+ host_off = s->data_end * BDRV_SECTOR_SIZE;
- space += s->prealloc_size;
/*
* We require the expanded size to read back as zero. If the
* user permitted truncation, we try that; but if it
fails, we
@@ -313,6 +311,32 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
s->used_bmap = bitmap_zero_extend(s->used_bmap,
s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
+ } else {
+ int64_t next_used;
+ next_used = find_next_bit(s->used_bmap, s->used_bmap_size,
first_free);
+
+ /* Not enough continuous clusters in the middle, adjust the
size */
+ if (next_used - first_free < to_allocate) {
+ to_allocate = next_used - first_free;
+ *pnum = (idx + to_allocate) * s->tracks - sector_num;
It looks, we can write this simplier:
*pnum = to_allocate * s->tracks;
because idx and sector_num aren't changed from idx calculation:
idx = sector_num / s->tracks;
absolutely NO! sector_num can be unaligned. Here we get to the
situation when the end of the operation is aligned to cluster
and is calculated above.
Den
OK, now I got the idea.
Reviewed-by: Alexander Ivanov
Re: [PATCH 20/21] parallels: naive implementation of parallels_co_pwrite_zeroes
On 9/15/23 20:41, Denis V. Lunev wrote:
The zero flag is missed in the Parallels format specification. We can
resort to discard if we have no backing file.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 83cb8d6722..a098e2cbc2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -583,6 +583,19 @@ done:
return ret;
}
+static int coroutine_fn GRAPH_RDLOCK
+parallels_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ BdrvRequestFlags flags)
+{
+/*
+ * The zero flag is missed in the Parallels format specification. We can
+ * resort to discard if we have no backing file (this condition is checked
+ * inside parallels_co_pdiscard().
+ */
+return parallels_co_pdiscard(bs, offset, bytes);
+}
+
+
static void parallels_check_unclean(BlockDriverState *bs,
BdrvCheckResult *res,
BdrvCheckMode fix)
@@ -1456,6 +1469,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_create_opts= parallels_co_create_opts,
.bdrv_co_check = parallels_co_check,
.bdrv_co_pdiscard = parallels_co_pdiscard,
+.bdrv_co_pwrite_zeroes = parallels_co_pwrite_zeroes,
};
static void bdrv_parallels_init(void)
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH 21/21] tests: extend test 131 to cover availability of the write-zeroes
On 9/15/23 20:41, Denis V. Lunev wrote:
This patch contains test which minimally tests write-zeroes on top of
working discard.
The following checks are added:
* write 2 clusters, write-zero to the first allocated cluster
* write 2 cluster, write-zero to the half the first allocated cluster
Signed-off-by: Denis V. Lunev
---
tests/qemu-iotests/131 | 20
tests/qemu-iotests/131.out | 20
2 files changed, 40 insertions(+)
diff --git a/tests/qemu-iotests/131 b/tests/qemu-iotests/131
index e50a658f22..308732d84b 100755
--- a/tests/qemu-iotests/131
+++ b/tests/qemu-iotests/131
@@ -105,6 +105,26 @@ _make_test_img $size
{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
{ $QEMU_IO -c "read -P 0 $((CLUSTER_SIZE + CLUSTER_HALF_SIZE)) $CLUSTER_DBL_SIZE"
"$TEST_IMG"; } 2>&1 | _filter_qemu_io | _filter_testdir
+echo "== check write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_DBL_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io
| _filter_testdir
+{ $QEMU_IMG map "$TEST_IMG"; } 2>&1 | _filter_qemu_img_map
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check cluster-partial write-zeroes =="
+
+# Clear image
+_make_test_img $size
+
+{ $QEMU_IO -c "write -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "write -z 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0 0 $CLUSTER_HALF_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -c "read -P 0x11 $CLUSTER_HALF_SIZE $CLUSTER_HALF_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== allocate with backing =="
# Verify that allocating clusters works fine even when there is a backing
image.
# Regression test for a bug where we would pass a buffer read from the backing
diff --git a/tests/qemu-iotests/131.out b/tests/qemu-iotests/131.out
index 9882f9df6c..8493561bab 100644
--- a/tests/qemu-iotests/131.out
+++ b/tests/qemu-iotests/131.out
@@ -64,6 +64,26 @@ read 524288/524288 bytes at offset 0
512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
read 2097152/2097152 bytes at offset 1572864
2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Offset Length File
+0x100x10TEST_DIR/t.IMGFMT
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check cluster-partial write-zeroes ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== allocate with backing ==
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH 04/21] parallels: return earler in fail_format branch in parallels_open()
On 9/15/23 20:41, Denis V. Lunev wrote: We do not need to perform any deallocation/cleanup if wrong format is detected. Signed-off-by: Denis V. Lunev --- block/parallels.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index 1d5409f2ba..0f127427bf 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1226,7 +1226,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, fail_format: error_setg(errp, "Image not in Parallels format"); -ret = -EINVAL; +return -EINVAL; + fail: /* * "s" object was allocated by g_malloc0 so we can safely Reviewed-by: Alexander Ivanov
Re: [PATCH 06/21] parallels: refactor path when we need to re-check image in parallels_open
On 9/15/23 20:41, Denis V. Lunev wrote:
More conditions follows thus the check should be more scalable.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 8f223bfd89..aa29df9f77 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1065,7 +1065,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
int ret, size, i;
int64_t file_nb_sectors, sector;
uint32_t data_start;
-bool data_off_is_correct;
+bool need_check = false;
ret = parallels_opts_prealloc(bs, options, errp);
if (ret < 0) {
@@ -1133,11 +1133,12 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_bitmap = (uint32_t *)(s->header + 1);
if (le32_to_cpu(ph.inuse) == HEADER_INUSE_MAGIC) {
-s->header_unclean = true;
+need_check = s->header_unclean = true;
}
-data_off_is_correct = parallels_test_data_off(s, file_nb_sectors,
- _start);
+need_check = need_check ||
+ !parallels_test_data_off(s, file_nb_sectors, _start);
+
s->data_start = data_start;
s->data_end = s->data_start;
if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
@@ -1194,6 +1195,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->data_end = sector + s->tracks;
}
}
+need_check = need_check || s->data_end > file_nb_sectors;
/*
* We don't repair the image here if it's opened for checks. Also we don't
@@ -1203,12 +1205,8 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
return 0;
}
-/*
- * Repair the image if it's dirty or
- * out-of-image corruption was detected.
- */
-if (s->data_end > file_nb_sectors || s->header_unclean
-|| !data_off_is_correct) {
+/* Repair the image if corruption was detected. */
+if (need_check) {
BdrvCheckResult res;
ret = bdrv_check(bs, , BDRV_FIX_ERRORS | BDRV_FIX_LEAKS);
if (ret < 0) {
@@ -1217,7 +1215,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
goto fail;
}
}
-
return 0;
fail_format:
Reviewed-by: Alexander Ivanov
Re: [PATCH 13/21] tests: test self-cure of parallels image with duplicated clusters
On 9/15/23 20:41, Denis V. Lunev wrote:
The test is quite similar with the original one for duplicated clusters.
There is the only difference in the operation which should fix the
image.
Signed-off-by: Denis V. Lunev
---
tests/qemu-iotests/tests/parallels-checks | 36 +++
tests/qemu-iotests/tests/parallels-checks.out | 31
2 files changed, 67 insertions(+)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index df99558486..b281246a42 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -135,6 +135,42 @@ echo "== check the second cluster (deduplicated) =="
# Clear image
_make_test_img $SIZE
+echo "== TEST DUPLICATION SELF-CURE =="
+
+echo "== write pattern to whole image =="
+{ $QEMU_IO -c "write -P 0x11 0 $SIZE" "$TEST_IMG"; } 2>&1 | _filter_qemu_io |
_filter_testdir
+
+echo "== write another pattern to second cluster =="
+{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1
| _filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+
+echo "== corrupt image =="
+poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster with self-repair =="
+{ $QEMU_IO -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check second cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
+# Clear image
+_make_test_img $SIZE
+
echo "== TEST DATA_OFF CHECK =="
echo "== write pattern to first cluster =="
diff --git a/tests/qemu-iotests/tests/parallels-checks.out
b/tests/qemu-iotests/tests/parallels-checks.out
index 1325d2b611..9793423111 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -71,6 +71,37 @@ read 1048576/1048576 bytes at offset 0
read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
+== TEST DUPLICATION SELF-CURE ==
+== write pattern to whole image ==
+wrote 4194304/4194304 bytes at offset 0
+4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to second cluster ==
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== corrupt image ==
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster with self-repair ==
+Repairing duplicate offset in BAT entry 1
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check second cluster ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
== TEST DATA_OFF CHECK ==
== write pattern to first cluster ==
wrote 1048576/1048576 bytes at offset 0
Reviewed-by: Alexander Ivanov
Re: [PATCH 04/21] parallels: return earler in fail_format branch in parallels_open()
This is not the case with this patch, but it seems that the 5 first "goto fail;" could be replaced by returns. The first allocation, freeing at the "fail" label, is at 1127 line. The next error handling and all the previous ones can make return instead goto fail. On 9/15/23 20:41, Denis V. Lunev wrote: We do not need to perform any deallocation/cleanup if wrong format is detected. Signed-off-by: Denis V. Lunev --- block/parallels.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index 1d5409f2ba..0f127427bf 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1226,7 +1226,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, fail_format: error_setg(errp, "Image not in Parallels format"); -ret = -EINVAL; +return -EINVAL; + fail: /* * "s" object was allocated by g_malloc0 so we can safely -- Best regards, Alexander Ivanov
Re: [PATCH 12/21] tests: fix broken deduplication check in parallels format test
On 9/15/23 20:41, Denis V. Lunev wrote:
Original check is broken as supposed reading from 2 different clusters
results in read from the same file offset twice. This is definitely
wrong.
We should be sure that
* the content of both clusters is correct after repair
* clusters are at the different offsets after repair
In order to check the latter we write some content into the first one
and validate that fact.
Signed-off-by: Denis V. Lunev
---
tests/qemu-iotests/tests/parallels-checks | 14 ++
tests/qemu-iotests/tests/parallels-checks.out | 16
2 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index f4ca50295e..df99558486 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -117,14 +117,20 @@ echo "== check second cluster =="
echo "== repair image =="
_check_test_img -r all
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x11 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
echo "== check second cluster =="
{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
-echo "== check first cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== write another pattern to the first clusters =="
+{ $QEMU_IO -c "write -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== check the first cluster =="
+{ $QEMU_IO -r -c "read -P 0x66 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
-echo "== check second cluster on host =="
-printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
+echo "== check the second cluster (deduplicated) =="
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
# Clear image
_make_test_img $SIZE
diff --git a/tests/qemu-iotests/tests/parallels-checks.out
b/tests/qemu-iotests/tests/parallels-checks.out
index 74a5e29260..1325d2b611 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -55,13 +55,21 @@ The following inconsistencies were found and repaired:
Double checking the fixed image now...
No errors were found on the image.
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
== check second cluster ==
read 1048576/1048576 bytes at offset 1048576
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-== check first cluster on host ==
-content: 0x11
-== check second cluster on host ==
-content: 0x11
+== write another pattern to the first clusters ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== check the second cluster (deduplicated) ==
+read 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
== TEST DATA_OFF CHECK ==
== write pattern to first cluster ==
Reviewed-by: Alexander Ivanov
Re: [PATCH 15/21] parallels: update used bitmap in allocate_cluster
On 9/15/23 20:41, Denis V. Lunev wrote:
We should extend the bitmap ff the file is extended and set the bit in
Typo: ff -> if.
the image used bitmap once the cluster is allocated. Sanity check at
that moment also looks like a good idea.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index a2ba5a9353..a6d2f05863 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -282,6 +282,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
return len;
}
if (s->data_end + space > (len >> BDRV_SECTOR_BITS)) {
+uint32_t new_usedsize;
+
space += s->prealloc_size;
/*
* We require the expanded size to read back as zero. If the
@@ -305,6 +307,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
if (ret < 0) {
return ret;
}
+
+new_usedsize = s->used_bmap_size +
+ (space << BDRV_SECTOR_BITS) / s->cluster_size;
+s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
+ new_usedsize);
+s->used_bmap_size = new_usedsize;
}
/*
@@ -336,6 +344,12 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
}
}
+ret = mark_used(bs, s->used_bmap, s->used_bmap_size,
+s->data_end << BDRV_SECTOR_BITS, to_allocate);
+if (ret < 0) {
+/* Image consistency is broken. Alarm! */
+return ret;
+}
for (i = 0; i < to_allocate; i++) {
parallels_set_bat_entry(s, idx + i, s->data_end / s->off_multiplier);
s->data_end += s->tracks;
Otherwise the typo, LGTM.
Reviewed-by: Alexander Ivanov
Re: [PATCH 02/21] parallels: mark driver as supporting CBT
On 9/15/23 20:41, Denis V. Lunev wrote:
Parallels driver indeed support Parallels Dirty Bitmap Feature in
read-only mode. The patch adds bdrv_supports_persistent_dirty_bitmap()
callback which always return 1 to indicate that.
This will allow to copy CBT from Parallels image with qemu-img.
Note: read-write support is signalled through
bdrv_co_can_store_new_dirty_bitmap() and is different.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 2ebd8e1301..428f72de1c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1248,6 +1248,11 @@ static void parallels_close(BlockDriverState *bs)
error_free(s->migration_blocker);
}
+static bool parallels_is_support_dirty_bitmaps(BlockDriverState *bs)
+{
+return 1;
+}
+
static BlockDriver bdrv_parallels = {
.format_name= "parallels",
.instance_size = sizeof(BDRVParallelsState),
@@ -1256,6 +1261,7 @@ static BlockDriver bdrv_parallels = {
.supports_backing = true,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
+.bdrv_supports_persistent_dirty_bitmap =
parallels_is_support_dirty_bitmaps,
.bdrv_probe = parallels_probe,
.bdrv_open = parallels_open,
Reviewed-by: Alexander Ivanov
Re: [PATCH 05/21] parallels: return earlier from parallels_open() function on error
On 9/15/23 20:41, Denis V. Lunev wrote:
At the beginning of the function we can return immediately until we
really allocate s->header.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 0f127427bf..8f223bfd89 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1084,7 +1084,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
ret = bdrv_pread(bs->file, 0, sizeof(ph), , 0);
if (ret < 0) {
-goto fail;
+return ret;
}
bs->total_sectors = le64_to_cpu(ph.nb_sectors);
@@ -1104,13 +1104,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->tracks = le32_to_cpu(ph.tracks);
if (s->tracks == 0) {
error_setg(errp, "Invalid image: Zero sectors per track");
-ret = -EINVAL;
-goto fail;
+return -EINVAL;
}
if (s->tracks > INT32_MAX/513) {
error_setg(errp, "Invalid image: Too big cluster");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
}
s->prealloc_size = MAX(s->tracks, s->prealloc_size);
s->cluster_size = s->tracks << BDRV_SECTOR_BITS;
@@ -1118,16 +1116,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->bat_size = le32_to_cpu(ph.bat_entries);
if (s->bat_size > INT_MAX / sizeof(uint32_t)) {
error_setg(errp, "Catalog too large");
-ret = -EFBIG;
-goto fail;
+return -EFBIG;
}
size = bat_entry_off(s->bat_size);
s->header_size = ROUND_UP(size, bdrv_opt_mem_align(bs->file->bs));
s->header = qemu_try_blockalign(bs->file->bs, s->header_size);
if (s->header == NULL) {
-ret = -ENOMEM;
-goto fail;
+ return -ENOMEM;
}
ret = bdrv_pread(bs->file, 0, s->header_size, s->header, 0);
Reviewed-by: Alexander Ivanov
Re: [PATCH 07/21] parallels: create mark_used() helper which sets bit in used bitmap
On 9/15/23 20:41, Denis V. Lunev wrote:
This functionality is used twice already and next patch will add more
code with it.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index aa29df9f77..60ad41b49b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,6 +178,21 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
+static int mark_used(BlockDriverState *bs,
+ unsigned long *bitmap, uint32_t bitmap_size, int64_t off)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_index = host_cluster_index(s, off);
+if (cluster_index >= bitmap_size) {
+return -E2BIG;
+}
+if (test_bit(cluster_index, bitmap)) {
+return -EBUSY;
+}
+bitmap_set(bitmap, cluster_index, 1);
+return 0;
+}
+
static int64_t coroutine_fn GRAPH_RDLOCK
allocate_clusters(BlockDriverState *bs, int64_t sector_num,
int nb_sectors, int *pnum)
@@ -621,7 +636,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
BDRVParallelsState *s = bs->opaque;
int64_t host_off, host_sector, guest_sector;
unsigned long *bitmap;
-uint32_t i, bitmap_size, cluster_index, bat_entry;
+uint32_t i, bitmap_size, bat_entry;
int n, ret = 0;
uint64_t *buf = NULL;
bool fixed = false;
@@ -655,10 +670,9 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-cluster_index = host_cluster_index(s, host_off);
-assert(cluster_index < bitmap_size);
-if (!test_bit(cluster_index, bitmap)) {
-bitmap_set(bitmap, cluster_index, 1);
+ret = mark_used(bs, bitmap, bitmap_size, host_off);
+assert(ret != -E2BIG);
+if (ret == 0) {
continue;
}
@@ -713,11 +727,13 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res,
* consistent for the new allocated clusters too.
*
* Note, clusters allocated outside the current image are not
- * considered, and the bitmap size doesn't change.
+ * considered, and the bitmap size doesn't change. This specifically
+ * means that -E2BIG is OK.
*/
-cluster_index = host_cluster_index(s, host_off);
-if (cluster_index < bitmap_size) {
-bitmap_set(bitmap, cluster_index, 1);
+ret = mark_used(bs, bitmap, bitmap_size, host_off);
+if (ret == -EBUSY) {
+res->check_errors++;
+goto out_repair_bat;
}
fixed = true;
Reviewed-by: Alexander Ivanov
Re: [PATCH 08/21] tests: ensure that image validation will not cure the corruption
On 9/15/23 20:41, Denis V. Lunev wrote:
Since
commit cfce1091d55322789582480798a891cbaf66924e
Author: Alexander Ivanov
Date: Tue Jul 18 12:44:29 2023 +0200
parallels: Image repairing in parallels_open()
there is a potential pit fall with calling
qemu-io -c "read"
The image is opened in read-write mode and thus could be potentially
repaired. This could ruin testing process.
The patch forces read-only opening for reads. In that case repairing
is impossible.
Signed-off-by: Denis V. Lunev
---
tests/qemu-iotests/tests/parallels-checks | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index a7a1b357b5..5917ee079d 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -91,7 +91,7 @@ file_size=`stat --printf="%s" "$TEST_IMG"`
echo "file size: $file_size"
echo "== check last cluster =="
-{ $QEMU_IO -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $LAST_CLUSTER_OFF $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
# Clear image
_make_test_img $SIZE
@@ -105,19 +105,20 @@ echo "== write another pattern to second cluster =="
{ $QEMU_IO -c "write -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1
| _filter_qemu_io | _filter_testdir
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x55 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
+
echo "== corrupt image =="
poke_file "$TEST_IMG" "$(($BAT_OFFSET + 4))" "\x01\x00\x00\x00"
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
echo "== repair image =="
_check_test_img -r all
echo "== check second cluster =="
-{ $QEMU_IO -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+{ $QEMU_IO -r -c "read -P 0x11 $CLUSTER_SIZE $CLUSTER_SIZE" "$TEST_IMG"; }
2>&1 | _filter_qemu_io | _filter_testdir
echo "== check first cluster on host =="
printf "content: 0x%02x\n" `peek_file_le $TEST_IMG $(($CLUSTER_SIZE)) 1`
Reviewed-by: Alexander Ivanov
Re: [PATCH 14/21] parallels: accept multiple clusters in mark_used()
On 9/15/23 20:41, Denis V. Lunev wrote:
This would be useful in the next patch in allocate_clusters(). This
change would not imply serious performance drawbacks as usually image
is full of data or are at the end of the bitmap.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 2517f35581..a2ba5a9353 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,18 +178,20 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
-static int mark_used(BlockDriverState *bs,
- unsigned long *bitmap, uint32_t bitmap_size, int64_t off)
+static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
{
BDRVParallelsState *s = bs->opaque;
uint32_t cluster_index = host_cluster_index(s, off);
-if (cluster_index >= bitmap_size) {
+unsigned long next_used;
+if (cluster_index + count > bitmap_size) {
return -E2BIG;
}
-if (test_bit(cluster_index, bitmap)) {
+next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
+if (next_used < cluster_index + count) {
return -EBUSY;
}
-bitmap_set(bitmap, cluster_index, 1);
+bitmap_set(bitmap, cluster_index, count);
return 0;
}
@@ -230,7 +232,7 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
continue;
}
-err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off);
+err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
if (err2 < 0 && err == 0) {
err = err2;
}
@@ -732,7 +734,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-ret = mark_used(bs, bitmap, bitmap_size, host_off);
+ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
assert(ret != -E2BIG);
if (ret == 0) {
continue;
@@ -792,7 +794,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
* considered, and the bitmap size doesn't change. This specifically
* means that -E2BIG is OK.
*/
-ret = mark_used(bs, bitmap, bitmap_size, host_off);
+ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
if (ret == -EBUSY) {
res->check_errors++;
goto out_repair_bat;
Reviewed-by: Alexander Ivanov
Re: [PATCH 01/21] parallels: fix formatting in bdrv_parallels initialization
On 9/15/23 20:41, Denis V. Lunev wrote:
Old code is ugly and contains tabulations. There are no functional
changes in this patch.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48c32d6821..2ebd8e1301 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1249,23 +1249,25 @@ static void parallels_close(BlockDriverState *bs)
}
static BlockDriver bdrv_parallels = {
-.format_name = "parallels",
-.instance_size = sizeof(BDRVParallelsState),
-.bdrv_probe= parallels_probe,
-.bdrv_open = parallels_open,
-.bdrv_close= parallels_close,
-.bdrv_child_perm = bdrv_default_perms,
-.bdrv_co_block_status = parallels_co_block_status,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_co_flush_to_os = parallels_co_flush_to_os,
-.bdrv_co_readv = parallels_co_readv,
-.bdrv_co_writev = parallels_co_writev,
-.is_format = true,
-.supports_backing = true,
-.bdrv_co_create = parallels_co_create,
-.bdrv_co_create_opts = parallels_co_create_opts,
-.bdrv_co_check = parallels_co_check,
-.create_opts= _create_opts,
+.format_name= "parallels",
+.instance_size = sizeof(BDRVParallelsState),
+.create_opts= _create_opts,
+.is_format = true,
+.supports_backing = true,
+
+.bdrv_has_zero_init = bdrv_has_zero_init_1,
+
+.bdrv_probe = parallels_probe,
+.bdrv_open = parallels_open,
+.bdrv_close = parallels_close,
+.bdrv_child_perm= bdrv_default_perms,
+.bdrv_co_block_status = parallels_co_block_status,
+.bdrv_co_flush_to_os= parallels_co_flush_to_os,
+.bdrv_co_readv = parallels_co_readv,
+.bdrv_co_writev = parallels_co_writev,
+.bdrv_co_create = parallels_co_create,
+.bdrv_co_create_opts= parallels_co_create_opts,
+.bdrv_co_check = parallels_co_check,
};
static void bdrv_parallels_init(void)
Reviewed-by: Alexander Ivanov
Re: [PATCH 01/21] parallels: fix formatting in bdrv_parallels initialization
On 9/15/23 20:41, Denis V. Lunev wrote:
Old code is ugly and contains tabulations. There are no functional
changes in this patch.
Signed-off-by: Denis V. Lunev
---
block/parallels.c | 36 +++-
1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48c32d6821..2ebd8e1301 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1249,23 +1249,25 @@ static void parallels_close(BlockDriverState *bs)
}
static BlockDriver bdrv_parallels = {
-.format_name = "parallels",
-.instance_size = sizeof(BDRVParallelsState),
-.bdrv_probe= parallels_probe,
-.bdrv_open = parallels_open,
-.bdrv_close= parallels_close,
-.bdrv_child_perm = bdrv_default_perms,
-.bdrv_co_block_status = parallels_co_block_status,
-.bdrv_has_zero_init = bdrv_has_zero_init_1,
-.bdrv_co_flush_to_os = parallels_co_flush_to_os,
-.bdrv_co_readv = parallels_co_readv,
-.bdrv_co_writev = parallels_co_writev,
-.is_format = true,
-.supports_backing = true,
-.bdrv_co_create = parallels_co_create,
-.bdrv_co_create_opts = parallels_co_create_opts,
-.bdrv_co_check = parallels_co_check,
-.create_opts= _create_opts,
+.format_name= "parallels",
+.instance_size = sizeof(BDRVParallelsState),
+.create_opts= _create_opts,
+.is_format = true,
+.supports_backing = true,
+
+.bdrv_has_zero_init = bdrv_has_zero_init_1,
+
+.bdrv_probe = parallels_probe,
+.bdrv_open = parallels_open,
+.bdrv_close = parallels_close,
+.bdrv_child_perm= bdrv_default_perms,
+.bdrv_co_block_status = parallels_co_block_status,
+.bdrv_co_flush_to_os= parallels_co_flush_to_os,
+.bdrv_co_readv = parallels_co_readv,
+.bdrv_co_writev = parallels_co_writev,
+.bdrv_co_create = parallels_co_create,
+.bdrv_co_create_opts= parallels_co_create_opts,
+.bdrv_co_check = parallels_co_check,
};
static void bdrv_parallels_init(void)
Reviewed-by: Alexander Ivanov
Re: [PATCH 04/21] parallels: return earler in fail_format branch in parallels_open()
Oh, sorry, I see it in the next patch. =) On 9/18/23 10:14, Alexander Ivanov wrote: This is not the case with this patch, but it seems that the 5 first "goto fail;" could be replaced by returns. The first allocation, freeing at the "fail" label, is at 1127 line. The next error handling and all the previous ones can make return instead goto fail. On 9/15/23 20:41, Denis V. Lunev wrote: We do not need to perform any deallocation/cleanup if wrong format is detected. Signed-off-by: Denis V. Lunev --- block/parallels.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index 1d5409f2ba..0f127427bf 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -1226,7 +1226,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags, fail_format: error_setg(errp, "Image not in Parallels format"); - ret = -EINVAL; + return -EINVAL; + fail: /* * "s" object was allocated by g_malloc0 so we can safely -- Best regards, Alexander Ivanov
Re: [PATCH 10/21] parallels: add test which will validate data_off fixes through repair
On 9/15/23 20:41, Denis V. Lunev wrote:
We have only check through self-repair and that proven to be not enough.
Signed-off-by: Denis V. Lunev
---
tests/qemu-iotests/tests/parallels-checks | 17 +
tests/qemu-iotests/tests/parallels-checks.out | 18 ++
2 files changed, 35 insertions(+)
diff --git a/tests/qemu-iotests/tests/parallels-checks
b/tests/qemu-iotests/tests/parallels-checks
index 5917ee079d..f4ca50295e 100755
--- a/tests/qemu-iotests/tests/parallels-checks
+++ b/tests/qemu-iotests/tests/parallels-checks
@@ -140,6 +140,23 @@ poke_file "$TEST_IMG" "$DATA_OFF_OFFSET" "\xff\xff\xff\xff"
echo "== check first cluster =="
{ $QEMU_IO -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+# Clear image
+_make_test_img $SIZE
+
+echo "== TEST DATA_OFF THROUGH REPAIR =="
+
+echo "== write pattern to first cluster =="
+{ $QEMU_IO -c "write -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
+echo "== spoil data_off field =="
+poke_file "$TEST_IMG" "$DATA_OFF_OFFSET" "\xff\xff\xff\xff"
+
+echo "== repair image =="
+_check_test_img -r all
+
+echo "== check first cluster =="
+{ $QEMU_IO -r -c "read -P 0x55 0 $CLUSTER_SIZE" "$TEST_IMG"; } 2>&1 |
_filter_qemu_io | _filter_testdir
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/tests/parallels-checks.out
b/tests/qemu-iotests/tests/parallels-checks.out
index 98a3a7f55e..74a5e29260 100644
--- a/tests/qemu-iotests/tests/parallels-checks.out
+++ b/tests/qemu-iotests/tests/parallels-checks.out
@@ -72,4 +72,22 @@ wrote 1048576/1048576 bytes at offset 0
Repairing data_off field has incorrect value
read 1048576/1048576 bytes at offset 0
1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
+== TEST DATA_OFF THROUGH REPAIR ==
+== write pattern to first cluster ==
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+== spoil data_off field ==
+== repair image ==
+Repairing data_off field has incorrect value
+The following inconsistencies were found and repaired:
+
+0 leaked clusters
+1 corruptions
+
+Double checking the fixed image now...
+No errors were found on the image.
+== check first cluster ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
*** done
Reviewed-by: Alexander Ivanov
[PATCH 08/19] parallels: Make mark_used() and mark_unused() global functions
We will need these functions in parallels-ext.c too. Let them be global
functions parallels_mark_used() and parallels_mark_unused().
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 22 --
block/parallels.h | 5 +
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 8208c0bc1a..adb43a7069 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -178,8 +178,8 @@ static void parallels_set_bat_entry(BDRVParallelsState *s,
bitmap_set(s->bat_dirty_bmap, bat_entry_off(index) / s->bat_dirty_block,
1);
}
-static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
- uint32_t bitmap_size, int64_t off, uint32_t count)
+int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
+uint32_t bitmap_size, int64_t off, uint32_t count)
{
BDRVParallelsState *s = bs->opaque;
uint32_t cluster_index = host_cluster_index(s, off);
@@ -195,8 +195,8 @@ static int mark_used(BlockDriverState *bs, unsigned long
*bitmap,
return 0;
}
-static int mark_unused(BlockDriverState *bs, unsigned long *bitmap,
- uint32_t bitmap_size, int64_t off, uint32_t count)
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
{
BDRVParallelsState *s = bs->opaque;
uint32_t cluster_index = host_cluster_index(s, off);
@@ -249,7 +249,8 @@ static int parallels_fill_used_bitmap(BlockDriverState *bs)
continue;
}
-err2 = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, 1);
+err2 = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+ host_off, 1);
if (err2 < 0 && err == 0) {
err = err2;
}
@@ -323,7 +324,8 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
}
}
-ret = mark_used(bs, s->used_bmap, s->used_bmap_size, host_off, *clusters);
+ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+ host_off, *clusters);
if (ret < 0) {
/* Image consistency is broken. Alarm! */
return ret;
@@ -390,8 +392,8 @@ allocate_clusters(BlockDriverState *bs, int64_t sector_num,
qemu_vfree(buf);
if (ret < 0) {
-mark_unused(bs, s->used_bmap, s->used_bmap_size,
-host_off, to_allocate);
+parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+ host_off, to_allocate);
return ret;
}
}
@@ -865,7 +867,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
continue;
}
-ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
+ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
assert(ret != -E2BIG);
if (ret == 0) {
continue;
@@ -925,7 +927,7 @@ parallels_check_duplicate(BlockDriverState *bs,
BdrvCheckResult *res,
* considered, and the bitmap size doesn't change. This specifically
* means that -E2BIG is OK.
*/
-ret = mark_used(bs, bitmap, bitmap_size, host_off, 1);
+ret = parallels_mark_used(bs, bitmap, bitmap_size, host_off, 1);
if (ret == -EBUSY) {
res->check_errors++;
goto out_repair_bat;
diff --git a/block/parallels.h b/block/parallels.h
index 3e4f397502..4e7aa6b80f 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -90,6 +90,11 @@ typedef struct BDRVParallelsState {
Error *migration_blocker;
} BDRVParallelsState;
+int parallels_mark_used(BlockDriverState *bs, unsigned long *bitmap,
+uint32_t bitmap_size, int64_t off, uint32_t count);
+int parallels_mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count);
+
int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters);
--
2.34.1
[PATCH 03/19] parallels: Move host clusters allocation to a separate function
For parallels images extensions we need to allocate host clusters
without any connection to BAT. Move host clusters allocation code to
allocate_host_clusters().
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 124 --
block/parallels.h | 4 ++
2 files changed, 69 insertions(+), 59 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index b5e19ff921..3c69afa04b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -264,58 +264,29 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
g_free(s->used_bmap);
}
-static int64_t coroutine_fn GRAPH_RDLOCK
-allocate_clusters(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, int *pnum)
+int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
+ int64_t *clusters)
{
-int ret = 0;
BDRVParallelsState *s = bs->opaque;
-int64_t i, pos, idx, to_allocate, first_free, host_off;
-
-pos = block_status(s, sector_num, nb_sectors, pnum);
-if (pos > 0) {
-return pos;
-}
-
-idx = sector_num / s->tracks;
-to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
-
-/*
- * This function is called only by parallels_co_writev(), which will never
- * pass a sector_num at or beyond the end of the image (because the block
- * layer never passes such a sector_num to that function). Therefore, idx
- * is always below s->bat_size.
- * block_status() will limit *pnum so that sector_num + *pnum will not
- * exceed the image end. Therefore, idx + to_allocate cannot exceed
- * s->bat_size.
- * Note that s->bat_size is an unsigned int, therefore idx + to_allocate
- * will always fit into a uint32_t.
- */
-assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
+int64_t first_free, next_used, host_off, bytes, prealloc_clusters;
+uint32_t new_usedsize;
+int ret = 0;
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-uint32_t new_usedsize;
-int64_t bytes = to_allocate * s->cluster_size;
-bytes += s->prealloc_size * BDRV_SECTOR_SIZE;
-
host_off = s->data_end * BDRV_SECTOR_SIZE;
+prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
+bytes = prealloc_clusters * s->cluster_size;
-/*
- * We require the expanded size to read back as zero. If the
- * user permitted truncation, we try that; but if it fails, we
- * force the safer-but-slower fallocate.
- */
if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-ret = bdrv_co_truncate(bs->file, host_off + bytes,
- false, PREALLOC_MODE_OFF,
- BDRV_REQ_ZERO_WRITE, NULL);
+ret = bdrv_truncate(bs->file, host_off + bytes, false,
+PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE, NULL);
if (ret == -ENOTSUP) {
s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
}
}
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0);
+ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
}
if (ret < 0) {
return ret;
@@ -325,15 +296,15 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
+if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
+s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
+}
} else {
-int64_t next_used;
next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
/* Not enough continuous clusters in the middle, adjust the size */
-if (next_used - first_free < to_allocate) {
-to_allocate = next_used - first_free;
-*pnum = (idx + to_allocate) * s->tracks - sector_num;
-}
+*clusters = MIN(*clusters, next_used - first_free);
+bytes = *clusters * s->cluster_size;
host_off = s->data_start * BDRV_SECTOR_SIZE;
host_off += first_free * s->cluster_size;
@@ -345,14 +316,58 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
host_off < s->data_end * BDRV_SECTOR_SIZE) {
-ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
-s->cluster_size * to_allocate, 0);
+ret = bdrv_pwrite_zeroes(bs-
[PATCH 16/19] parallels: Check unused clusters in parallels_check_leak()
Since we have used bitmap, leak check is useless. Transform
parallels_truncate_unused_clusters() to parallels_check_unused_clusters()
helper and use it in leak check.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 112 ++
1 file changed, 63 insertions(+), 49 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 80a7171b84..053ec0f8a1 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -764,56 +764,82 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
return 0;
}
+static int64_t parallels_check_unused_clusters(BlockDriverState *bs,
+ bool truncate)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t leak, file_size, end_off = 0;
+int ret;
+
+file_size = bdrv_getlength(bs->file->bs);
+if (file_size < 0) {
+return file_size;
+}
+
+if (s->used_bmap_size > 0) {
+end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+if (end_off == s->used_bmap_size) {
+return 0;
+}
+end_off = (end_off + 1) * s->cluster_size;
+}
+
+end_off += s->data_start * BDRV_SECTOR_SIZE;
+leak = file_size - end_off;
+if (leak < 0) {
+return -EINVAL;
+}
+if (!truncate || leak == 0) {
+return leak;
+}
+
+ret = bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+if (ret) {
+return ret;
+}
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret < 0) {
+return ret;
+}
+
+return leak;
+}
+
static int coroutine_fn GRAPH_RDLOCK
parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
BdrvCheckMode fix, bool explicit)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size, count;
-int ret;
+int64_t leak, count, size;
+
+leak = parallels_check_unused_clusters(bs, fix & BDRV_FIX_LEAKS);
+if (leak < 0) {
+res->check_errors++;
+return leak;
+}
+if (leak == 0) {
+return 0;
+}
size = bdrv_co_getlength(bs->file->bs);
if (size < 0) {
-res->check_errors++;
return size;
}
-if (size <= res->image_end_offset) {
+res->image_end_offset = size;
+
+if (!explicit) {
return 0;
}
-count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-if (explicit) {
-fprintf(stderr,
-"%s space leaked at the end of the image %" PRId64 "\n",
-fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-size - res->image_end_offset);
-res->leaks += count;
-}
+count = DIV_ROUND_UP(leak, s->cluster_size);
+fprintf(stderr,
+"%s space leaked at the end of the image %" PRId64 "\n",
+fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak);
+res->leaks += count;
if (fix & BDRV_FIX_LEAKS) {
-Error *local_err = NULL;
-
-/*
- * In order to really repair the image, we must shrink it.
- * That means we have to pass exact=true.
- */
-ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
- PREALLOC_MODE_OFF, 0, _err);
-if (ret < 0) {
-error_report_err(local_err);
-res->check_errors++;
-return ret;
-}
-
-parallels_free_used_bitmap(bs);
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-res->check_errors++;
-return ret;
-}
-
-if (explicit) {
-res->leaks_fixed += count;
-}
+res->leaks_fixed += count;
}
return 0;
@@ -1446,18 +1472,6 @@ fail:
return ret;
}
-static int parallels_truncate_unused_clusters(BlockDriverState *bs)
-{
-BDRVParallelsState *s = bs->opaque;
-uint64_t end_off = 0;
-if (s->used_bmap_size > 0) {
-end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
-end_off = (end_off + 1) * s->cluster_size;
-}
-end_off += s->data_start * BDRV_SECTOR_SIZE;
-return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
-}
-
static int parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -1475,7 +1489,7 @@ static int parallels_inactivate(BlockDriverState *bs)
parallels_update_header(bs);
/* errors are ignored, so we might as well pass exact=true */
-ret = parallels_truncate_unused_clusters(bs);
+ret = parallels_check_unused_clusters(bs, true);
if (ret < 0) {
error_report("Failed to truncate image: %s", strerror(-ret));
}
--
2.34.1
[PATCH 14/19] parallels: Truncate images on the last used cluster
On an image closing there can be unused clusters in the end of the image.
Truncate these clusters and update data_end field.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 7b16ca2ab2..48ea5b3f03 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1452,6 +1452,19 @@ fail:
return ret;
}
+static int parallels_truncate_unused_clusters(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+uint64_t end_off = 0;
+if (s->used_bmap_size > 0) {
+end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+end_off = (end_off + 1) * s->cluster_size;
+}
+end_off += s->data_start * BDRV_SECTOR_SIZE;
+s->data_end = end_off / BDRV_SECTOR_SIZE;
+return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+}
+
static int parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -1469,8 +1482,7 @@ static int parallels_inactivate(BlockDriverState *bs)
parallels_update_header(bs);
/* errors are ignored, so we might as well pass exact=true */
-ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
-true, PREALLOC_MODE_OFF, 0, NULL);
+ret = parallels_truncate_unused_clusters(bs);
if (ret < 0) {
error_report("Failed to truncate image: %s", strerror(-ret));
}
--
2.34.1
[PATCH 19/19] tests: Add parallels format support to image-fleecing
Use a different bitmap name for parallels images because their has own ID
format, and can't contain an arbitrary string.
Replace hardcoded 'qcow2' format to iotests.imgfmt.
Add 'parallels' to supported formats.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/tests/image-fleecing | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/image-fleecing
b/tests/qemu-iotests/tests/image-fleecing
index f6e449d071..00d7f37af4 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -28,7 +28,7 @@ import iotests
from iotests import log, qemu_img, qemu_io
iotests.script_initialize(
-supported_fmts=['qcow2'],
+supported_fmts=['qcow2', 'parallels'],
supported_platforms=['linux'],
required_fmts=['copy-before-write'],
unsupported_imgopts=['compat']
@@ -61,12 +61,17 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
if push_backup:
assert use_cbw
+if iotests.imgfmt == 'parallels':
+bitmap_name = '----'
+else:
+bitmap_name = 'bitmap0'
+
log('--- Setting up images ---')
log('')
qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
if bitmap:
-qemu_img('bitmap', '--add', base_img_path, 'bitmap0')
+qemu_img('bitmap', '--add', base_img_path, bitmap_name)
if use_snapshot_access_filter:
assert use_cbw
@@ -75,7 +80,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
qemu_img('create', '-f', 'qcow2', fleece_img_path, '64M')
if push_backup:
-qemu_img('create', '-f', 'qcow2', target_img_path, '64M')
+qemu_img('create', '-f', iotests.imgfmt, target_img_path, '64M')
for p in patterns:
qemu_io('-f', iotests.imgfmt,
@@ -130,7 +135,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
}
if bitmap:
-fl_cbw['bitmap'] = {'node': src_node, 'name': 'bitmap0'}
+fl_cbw['bitmap'] = {'node': src_node, 'name': bitmap_name}
log(vm.qmp('blockdev-add', fl_cbw))
--
2.34.1
[PATCH 11/19] parallels: Handle L1 entries equal to one
If all the bits in a dirty bitmap cluster are ones, the cluster shouldn't
be written. Instead the corresponding L1 entry should be set to 1.
Check if all bits in a memory region are ones and set 1 to L1 entries
corresponding clusters filled with ones.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index a0de4f1b07..ebda6b0a01 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void parallels_save_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
offset = 0;
while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >=
0) {
uint64_t idx = offset / limit;
-int64_t cluster_off, end, write_size;
+int64_t cluster_off, end, write_size, first_zero;
offset = QEMU_ALIGN_DOWN(offset, limit);
end = MIN(bm_size, offset + limit);
@@ -367,6 +367,16 @@ static void parallels_save_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
memset(bm_buf + write_size, 0, s->cluster_size - write_size);
}
+first_zero = bdrv_dirty_bitmap_next_zero(bitmap, offset, bm_size);
+if (first_zero < 0) {
+goto end;
+}
+if (first_zero - offset >= s->cluster_size) {
+l1_table[idx] = 1;
+offset = end;
+continue;
+}
+
cluster_off = parallels_allocate_host_clusters(bs, _size);
if (cluster_off <= 0) {
goto end;
--
2.34.1
[PATCH 17/19] tests: Add parallels images support to test 165
Use a different bitmap name for parallels images because their has own ID
format, and can't contain an arbitrary string.
Replace image reopen by shutdown/launch VM because parallels images doesn't
support reopen.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/165 | 42 ++
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index e3ef28e2ee..3181cccb89 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -38,6 +38,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+if iotests.imgfmt == 'parallels':
+self.bitmap_name = '----'
+else:
+self.bitmap_name = 'bitmap0'
def tearDown(self):
os.remove(disk)
@@ -50,12 +54,12 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def getSha256(self):
result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
- node='drive0', name='bitmap0')
+ node='drive0', name=self.bitmap_name)
return result['return']['sha256']
def checkBitmap(self, sha256):
result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
- node='drive0', name='bitmap0')
+ node='drive0', name=self.bitmap_name)
self.assert_qmp(result, 'return/sha256', sha256);
def writeRegions(self, regions):
@@ -65,7 +69,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def qmpAddBitmap(self):
self.vm.qmp('block-dirty-bitmap-add', node='drive0',
-name='bitmap0', persistent=True)
+name=self.bitmap_name, persistent=True)
def test_persistent(self):
self.vm = self.mkVm()
@@ -117,7 +121,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
assert sha256_1 != sha256_2 # Otherwise, it's not very interesting.
result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
- name='bitmap0')
+ name=self.bitmap_name)
self.assert_qmp(result, 'return', {})
# Start with regions1
@@ -136,17 +140,23 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
self.writeRegions(regions2)
assert sha256_1 == self.getSha256()
-# Reopen to RW
-result = self.vm.qmp('blockdev-reopen', options=[{
-'node-name': 'node0',
-'driver': iotests.imgfmt,
-'file': {
-'driver': 'file',
-'filename': disk
-},
-'read-only': False
-}])
-self.assert_qmp(result, 'return', {})
+if iotests.imgfmt == 'parallels':
+# parallels doesn't support reopen
+self.vm.shutdown()
+self.vm = self.mkVm()
+self.vm.launch()
+else:
+# Reopen to RW
+result = self.vm.qmp('blockdev-reopen', options=[{
+'node-name': 'node0',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': disk
+},
+'read-only': False
+}])
+self.assert_qmp(result, 'return', {})
# Check that bitmap is reopened to RW and we can write to it.
self.writeRegions(regions2)
@@ -156,6 +166,6 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
if __name__ == '__main__':
-iotests.main(supported_fmts=['qcow2'],
+iotests.main(supported_fmts=['qcow2', 'parallels'],
supported_protocols=['file'],
unsupported_imgopts=['compat'])
--
2.34.1
[PATCH 00/19] parallels: Add full dirty bitmap support
Parallels format driver: * make some preparation * add dirty bitmap saving * make dirty bitmap RW * fix broken checks * refactor leak check * add parallels format support to several tests Alexander Ivanov (19): parallels: Move inactivation code to a separate function parallels: Add mark_unused() helper parallels: Move host clusters allocation to a separate function parallels: Set data_end value in parallels_check_leak() parallels: Recreate used bitmap in parallels_check_leak() parallels: Add a note about used bitmap in parallels_check_duplicate() parallels: Create used bitmap even if checks needed parallels: Make mark_used() and mark_unused() global functions parallels: Add dirty bitmaps saving parallels: Let image extensions work in RW mode parallels: Handle L1 entries equal to one parallels: Make a loaded dirty bitmap persistent parallels: Reverse a conditional in parallels_check_leak() to reduce indents parallels: Truncate images on the last used cluster parallels: Remove unnecessary data_end field parallels: Check unused clusters in parallels_check_leak() tests: Add parallels images support to test 165 tests: Turned on 256, 299, 304 and block-status-cache for parallels format tests: Add parallels format support to image-fleecing block/parallels-ext.c | 182 +- block/parallels.c | 357 block/parallels.h | 15 +- tests/qemu-iotests/165 | 42 ++- tests/qemu-iotests/256 | 2 +- tests/qemu-iotests/299 | 2 +- tests/qemu-iotests/304 | 2 +- tests/qemu-iotests/tests/block-status-cache | 2 +- tests/qemu-iotests/tests/image-fleecing | 13 +- 9 files changed, 443 insertions(+), 174 deletions(-) -- 2.34.1
[PATCH 02/19] parallels: Add mark_unused() helper
Add a helper to set unused areas in used bitmap.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index d5b333d5a4..b5e19ff921 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -195,6 +195,23 @@ static int mark_used(BlockDriverState *bs, unsigned long
*bitmap,
return 0;
}
+static int mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_index = host_cluster_index(s, off);
+unsigned long next_unused;
+if (cluster_index + count > bitmap_size) {
+return -E2BIG;
+}
+next_unused = find_next_zero_bit(bitmap, bitmap_size, cluster_index);
+if (next_unused < cluster_index + count) {
+return -EINVAL;
+}
+bitmap_clear(bitmap, cluster_index, count);
+return 0;
+}
+
/*
* Collect used bitmap. The image can contain errors, we should fill the
* bitmap anyway, as much as we can. This information will be used for
--
2.34.1
[PATCH 06/19] parallels: Add a note about used bitmap in parallels_check_duplicate()
In parallels_check_duplicate() We use a bitmap for duplication detection. This bitmap is not related to used_bmap field in BDRVParallelsState. Add a comment about it to avoid confusion. Signed-off-by: Alexander Ivanov --- block/parallels.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index 1a4f8d1eb2..b735ba8390 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -834,7 +834,10 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res, bool fixed = false; /* - * Create a bitmap of used clusters. + * Create a bitmap of used clusters. Please note that this bitmap is not + * related to used_bmap field in BDRVParallelsState and is created only for + * local usage. + * * If a bit is set, there is a BAT entry pointing to this cluster. * Loop through the BAT entries, check bits relevant to an entry offset. * If bit is set, this entry is duplicated. Otherwise set the bit. -- 2.34.1
[PATCH 05/19] parallels: Recreate used bitmap in parallels_check_leak()
In parallels_check_leak() file can be truncated. In this case the used
bitmap would not comply to the file. Recreate the bitmap after file
truncation.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index e8d6d30fa5..1a4f8d1eb2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -804,6 +804,14 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
return ret;
}
s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+res->check_errors++;
+return ret;
+}
+
if (explicit) {
res->leaks_fixed += count;
}
--
2.34.1
[PATCH 07/19] parallels: Create used bitmap even if checks needed
All the checks were fixed to work with used bitmap. Create used bitmap in
parallels_open() even if need_check is true.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index b735ba8390..8208c0bc1a 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1416,13 +1416,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
need_check = need_check || s->data_end > file_nb_sectors;
-if (!need_check) {
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-goto fail;
-}
-need_check = need_check || ret < 0; /* These are correctable errors */
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
}
+need_check = need_check || ret < 0; /* These are correctable errors */
/*
* We don't repair the image here if it's opened for checks. Also we don't
--
2.34.1
[PATCH 09/19] parallels: Add dirty bitmaps saving
Now dirty bitmaps can be loaded but there is no their saving. Add code for
dirty bitmap storage.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 167 ++
block/parallels.c | 16 +++-
block/parallels.h | 5 ++
3 files changed, 186 insertions(+), 2 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 8a109f005a..0a632a2331 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "qapi/error.h"
#include "block/block-io.h"
#include "block/block_int.h"
@@ -301,3 +302,169 @@ out:
return ret;
}
+
+static void parallels_save_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
*bitmap,
+ uint8_t **buf, int *buf_size)
+{
+BDRVParallelsState *s = bs->opaque;
+ParallelsFeatureHeader *fh;
+ParallelsDirtyBitmapFeature *bh;
+uint64_t *l1_table, l1_size, granularity, limit;
+int64_t bm_size, ser_size, offset, buf_used;
+int64_t alloc_size = 1;
+const char *name;
+uint8_t *bm_buf;
+QemuUUID uuid;
+int ret = 0;
+
+if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
+bdrv_dirty_bitmap_inconsistent(bitmap)) {
+return;
+}
+
+bm_size = bdrv_dirty_bitmap_size(bitmap);
+granularity = bdrv_dirty_bitmap_granularity(bitmap);
+limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
+ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size);
+l1_size = DIV_ROUND_UP(ser_size, s->cluster_size);
+
+buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh);
+/* Check if there is enough space for the final section */
+if (*buf_size - buf_used < sizeof(*fh)) {
+return;
+}
+
+name = bdrv_dirty_bitmap_name(bitmap);
+ret = qemu_uuid_parse(name, );
+if (ret < 0) {
+error_report("Can't save dirty bitmap: ID parsing error: '%s'", name);
+return;
+}
+
+fh = (ParallelsFeatureHeader *)*buf;
+bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh));
+l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
+
+fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
+fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
+
+bh->l1_size = cpu_to_le32(l1_size);
+bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS);
+bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS);
+memcpy(bh->id, , sizeof(uuid));
+
+bm_buf = qemu_blockalign(bs, s->cluster_size);
+
+offset = 0;
+while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >=
0) {
+uint64_t idx = offset / limit;
+int64_t cluster_off, end, write_size;
+
+offset = QEMU_ALIGN_DOWN(offset, limit);
+end = MIN(bm_size, offset + limit);
+write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
+ end - offset);
+assert(write_size <= s->cluster_size);
+
+bdrv_dirty_bitmap_serialize_part(bitmap, bm_buf, offset, end - offset);
+if (write_size < s->cluster_size) {
+memset(bm_buf + write_size, 0, s->cluster_size - write_size);
+}
+
+cluster_off = parallels_allocate_host_clusters(bs, _size);
+if (cluster_off <= 0) {
+goto end;
+}
+
+ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, bm_buf, 0);
+if (ret < 0) {
+memset(>magic, 0, sizeof(fh->magic));
+parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+ cluster_off, 1);
+goto end;
+}
+
+l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS);
+offset = end;
+}
+
+*buf_size -= buf_used;
+*buf += buf_used;
+
+end:
+qemu_vfree(bm_buf);
+}
+
+void parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+ Error **errp)
+{
+BDRVParallelsState *s = bs->opaque;
+BdrvDirtyBitmap *bitmap;
+ParallelsFormatExtensionHeader *eh;
+int remaining = s->cluster_size;
+uint8_t *buf, *pos;
+int64_t header_off, alloc_size = 1;
+g_autofree uint8_t *hash = NULL;
+size_t hash_len = 0;
+int ret;
+
+s->header->ext_off = 0;
+
+if (!bdrv_has_named_bitmaps(bs)) {
+return;
+}
+
+buf = qemu_blockalign0(bs, s->cluster_size);
+
+eh = (ParallelsFormatExtensionHeader *)buf;
+pos = buf + sizeof(*eh);
+
+eh->magic = cpu_to_le64(PARALLELS_FORMAT_EXTENSION_MAGIC);
+
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+parallels_save_bitmap(bs, bitmap, , );
+}
+
+header_off = parallels_allocate_host_clusters(bs, _size);
+if (head
[PATCH 10/19] parallels: Let image extensions work in RW mode
Now we support extensions saving and can let to work with them in
read-write mode.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 4
block/parallels.c | 17 -
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 0a632a2331..a0de4f1b07 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -177,10 +177,6 @@ static BdrvDirtyBitmap
*parallels_load_bitmap(BlockDriverState *bs,
return NULL;
}
-/* We support format extension only for RO parallels images. */
-assert(!(bs->open_flags & BDRV_O_RDWR));
-bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
return bitmap;
}
diff --git a/block/parallels.c b/block/parallels.c
index d2a45e0c04..772a8ca1eb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1371,19 +1371,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
if (ph.ext_off) {
-if (flags & BDRV_O_RDWR) {
-/*
- * It's unsafe to open image RW if there is an extension (as we
- * don't support it). But parallels driver in QEMU historically
- * ignores the extension, so print warning and don't care.
- */
-warn_report("Format Extension ignored in RW mode");
-} else {
-ret = parallels_read_format_extension(
-bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
-if (ret < 0) {
-goto fail;
-}
+ret = parallels_read_format_extension(
+bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+if (ret < 0) {
+goto fail;
}
}
--
2.34.1
[PATCH 04/19] parallels: Set data_end value in parallels_check_leak()
In parallels_check_leak() we change file size but don't correct data_end
field of BDRVParallelsState structure. Fix it.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index 3c69afa04b..e8d6d30fa5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -803,6 +803,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
res->check_errors++;
return ret;
}
+s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
if (explicit) {
res->leaks_fixed += count;
}
--
2.34.1
[PATCH 12/19] parallels: Make a loaded dirty bitmap persistent
After bitmap loading the bitmap is not persistent and is removed on image
saving. Set bitmap persistence to true.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index ebda6b0a01..bb4478c350 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -256,6 +256,7 @@ static int
parallels_parse_format_extension(BlockDriverState *bs,
if (!bitmap) {
goto fail;
}
+bdrv_dirty_bitmap_set_persistence(bitmap, true);
bitmaps = g_slist_append(bitmaps, bitmap);
break;
--
2.34.1
[PATCH 01/19] parallels: Move inactivation code to a separate function
We are going to add parallels image extensions storage and need a separate
function for inactivation code.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index d026ce9e2f..d5b333d5a4 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1426,18 +1426,27 @@ fail:
return ret;
}
+static int parallels_inactivate(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int ret;
+
+s->header->inuse = 0;
+parallels_update_header(bs);
+
+/* errors are ignored, so we might as well pass exact=true */
+ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
+PREALLOC_MODE_OFF, 0, NULL);
+
+return ret;
+}
static void parallels_close(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE))
{
-s->header->inuse = 0;
-parallels_update_header(bs);
-
-/* errors are ignored, so we might as well pass exact=true */
-bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
- PREALLOC_MODE_OFF, 0, NULL);
+parallels_inactivate(bs);
}
parallels_free_used_bitmap(bs);
@@ -1477,6 +1486,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_check = parallels_co_check,
.bdrv_co_pdiscard = parallels_co_pdiscard,
.bdrv_co_pwrite_zeroes = parallels_co_pwrite_zeroes,
+.bdrv_inactivate= parallels_inactivate,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PATCH 15/19] parallels: Remove unnecessary data_end field
Since we have used bitmap, field data_end in BDRVParallelsState is
redundant and can be removed.
Add parallels_data_end() helper and remove data_end handling.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 33 +
block/parallels.h | 1 -
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48ea5b3f03..80a7171b84 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -265,6 +265,13 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
g_free(s->used_bmap);
}
+static int64_t parallels_data_end(BDRVParallelsState *s)
+{
+int64_t data_end = s->data_start * BDRV_SECTOR_SIZE;
+data_end += s->used_bmap_size * s->cluster_size;
+return data_end;
+}
+
int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -275,7 +282,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-host_off = s->data_end * BDRV_SECTOR_SIZE;
+host_off = parallels_data_end(s);
prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
bytes = prealloc_clusters * s->cluster_size;
@@ -297,9 +304,6 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
-if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
-s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
-}
} else {
next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
@@ -315,8 +319,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
* branch. In the other case we are likely re-using hole. Preallocate
* the space if required by the prealloc_mode.
*/
-if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
-host_off < s->data_end * BDRV_SECTOR_SIZE) {
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
if (ret < 0) {
return ret;
@@ -757,13 +760,7 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
}
}
-if (high_off == 0) {
-res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
-} else {
-res->image_end_offset = high_off + s->cluster_size;
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-}
-
+res->image_end_offset = parallels_data_end(s);
return 0;
}
@@ -806,7 +803,6 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
res->check_errors++;
return ret;
}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
parallels_free_used_bitmap(bs);
ret = parallels_fill_used_bitmap(bs);
@@ -1361,8 +1357,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
s->data_start = data_start;
-s->data_end = s->data_start;
-if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
+if (s->data_start < (s->header_size >> BDRV_SECTOR_BITS)) {
/*
* There is not enough unused space to fit to block align between BAT
* and actual data. We can't avoid read-modify-write...
@@ -1403,11 +1398,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
for (i = 0; i < s->bat_size; i++) {
sector = bat2sect(s, i);
-if (sector + s->tracks > s->data_end) {
-s->data_end = sector + s->tracks;
+if (sector + s->tracks > file_nb_sectors) {
+need_check = true;
}
}
-need_check = need_check || s->data_end > file_nb_sectors;
ret = parallels_fill_used_bitmap(bs);
if (ret == -ENOMEM) {
@@ -1461,7 +1455,6 @@ static int
parallels_truncate_unused_clusters(BlockDriverState *bs)
end_off = (end_off + 1) * s->cluster_size;
}
end_off += s->data_start * BDRV_SECTOR_SIZE;
-s->data_end = end_off / BDRV_SECTOR_SIZE;
return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
}
diff --git a/block/parallels.h b/block/parallels.h
index 18b4f8068e..a6a048d890 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -79,7 +79,6 @@ typedef struct BDRVParallelsState {
unsigned int bat_size;
int64_t data_start;
-int64_t data_end;
uint64_t prealloc_size;
ParallelsPreallocMode prealloc_mode;
--
2.34.1
[PATCH 13/19] parallels: Reverse a conditional in parallels_check_leak() to reduce indents
Let the function return a success code if a file size is not bigger than
image_end_offset. Thus we can decrease indents in the next code block.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 72 +++
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 772a8ca1eb..7b16ca2ab2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -772,7 +772,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
BdrvCheckMode fix, bool explicit)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size;
+int64_t size, count;
int ret;
size = bdrv_co_getlength(bs->file->bs);
@@ -780,43 +780,43 @@ parallels_check_leak(BlockDriverState *bs,
BdrvCheckResult *res,
res->check_errors++;
return size;
}
+if (size <= res->image_end_offset) {
+return 0;
+}
+
+count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
+if (explicit) {
+fprintf(stderr,
+"%s space leaked at the end of the image %" PRId64 "\n",
+fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
+size - res->image_end_offset);
+res->leaks += count;
+}
+if (fix & BDRV_FIX_LEAKS) {
+Error *local_err = NULL;
+
+/*
+ * In order to really repair the image, we must shrink it.
+ * That means we have to pass exact=true.
+ */
+ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
+ PREALLOC_MODE_OFF, 0, _err);
+if (ret < 0) {
+error_report_err(local_err);
+res->check_errors++;
+return ret;
+}
+s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+res->check_errors++;
+return ret;
+}
-if (size > res->image_end_offset) {
-int64_t count;
-count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
if (explicit) {
-fprintf(stderr,
-"%s space leaked at the end of the image %" PRId64 "\n",
-fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-size - res->image_end_offset);
-res->leaks += count;
-}
-if (fix & BDRV_FIX_LEAKS) {
-Error *local_err = NULL;
-
-/*
- * In order to really repair the image, we must shrink it.
- * That means we have to pass exact=true.
- */
-ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
- PREALLOC_MODE_OFF, 0, _err);
-if (ret < 0) {
-error_report_err(local_err);
-res->check_errors++;
-return ret;
-}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-parallels_free_used_bitmap(bs);
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-res->check_errors++;
-return ret;
-}
-
-if (explicit) {
-res->leaks_fixed += count;
-}
+res->leaks_fixed += count;
}
}
--
2.34.1
[PATCH 18/19] tests: Turned on 256, 299, 304 and block-status-cache for parallels format
These tests pass with parallels format. Add parallels to supporting
formats for these tests.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/256 | 2 +-
tests/qemu-iotests/299 | 2 +-
tests/qemu-iotests/304 | 2 +-
tests/qemu-iotests/tests/block-status-cache | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/256 b/tests/qemu-iotests/256
index d7e67f4a05..1930bb017e 100755
--- a/tests/qemu-iotests/256
+++ b/tests/qemu-iotests/256
@@ -26,7 +26,7 @@ from iotests import log
iotests.verify_virtio_scsi_pci_or_ccw()
-iotests.script_initialize(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2', 'parallels'])
size = 64 * 1024 * 1024
with iotests.FilePath('img0') as img0_path, \
diff --git a/tests/qemu-iotests/299 b/tests/qemu-iotests/299
index a7122941fd..d8c4399446 100755
--- a/tests/qemu-iotests/299
+++ b/tests/qemu-iotests/299
@@ -23,7 +23,7 @@ import iotests
# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
iotests.script_initialize(
-supported_fmts=['qcow2'],
+supported_fmts=['qcow2', 'parallels'],
)
nbd_sock = iotests.file_path('nbd.sock', base_dir=iotests.sock_dir)
diff --git a/tests/qemu-iotests/304 b/tests/qemu-iotests/304
index 198f282087..1ebf30 100755
--- a/tests/qemu-iotests/304
+++ b/tests/qemu-iotests/304
@@ -23,7 +23,7 @@
import iotests
from iotests import qemu_img_create, qemu_img_log, file_path
-iotests.script_initialize(supported_fmts=['qcow2'],
+iotests.script_initialize(supported_fmts=['qcow2', 'parallels'],
supported_protocols=['file'])
test_img = file_path('test.qcow2')
diff --git a/tests/qemu-iotests/tests/block-status-cache
b/tests/qemu-iotests/tests/block-status-cache
index 5a7bc2c149..ade3d5b169 100755
--- a/tests/qemu-iotests/tests/block-status-cache
+++ b/tests/qemu-iotests/tests/block-status-cache
@@ -131,5 +131,5 @@ class TestBscWithNbd(iotests.QMPTestCase):
if __name__ == '__main__':
# The block-status cache only works on the protocol layer, so to test it,
# we can only use the raw format
-iotests.main(supported_fmts=['raw'],
+iotests.main(supported_fmts=['raw', 'parallels'],
supported_protocols=['file'])
--
2.34.1
Re: [PATCH 15/19] parallels: Remove unnecessary data_end field
On 10/6/23 21:43, Mike Maslenkin wrote:
On Mon, Oct 2, 2023 at 12:01 PM Alexander Ivanov
wrote:
Since we have used bitmap, field data_end in BDRVParallelsState is
redundant and can be removed.
Add parallels_data_end() helper and remove data_end handling.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 33 +
block/parallels.h | 1 -
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48ea5b3f03..80a7171b84 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -265,6 +265,13 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
g_free(s->used_bmap);
}
+static int64_t parallels_data_end(BDRVParallelsState *s)
+{
+int64_t data_end = s->data_start * BDRV_SECTOR_SIZE;
+data_end += s->used_bmap_size * s->cluster_size;
+return data_end;
+}
+
int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -275,7 +282,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-host_off = s->data_end * BDRV_SECTOR_SIZE;
+host_off = parallels_data_end(s);
prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
bytes = prealloc_clusters * s->cluster_size;
@@ -297,9 +304,6 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
-if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
-s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
-}
} else {
next_used = find_next_bit(s->used_bmap, s->used_bmap_size,
first_free);
@@ -315,8 +319,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
* branch. In the other case we are likely re-using hole. Preallocate
* the space if required by the prealloc_mode.
*/
-if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
-host_off < s->data_end * BDRV_SECTOR_SIZE) {
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
if (ret < 0) {
return ret;
@@ -757,13 +760,7 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
}
}
-if (high_off == 0) {
-res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
-} else {
-res->image_end_offset = high_off + s->cluster_size;
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-}
-
+res->image_end_offset = parallels_data_end(s);
return 0;
}
@@ -806,7 +803,6 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
res->check_errors++;
return ret;
}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
parallels_free_used_bitmap(bs);
ret = parallels_fill_used_bitmap(bs);
@@ -1361,8 +1357,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
s->data_start = data_start;
-s->data_end = s->data_start;
-if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
+if (s->data_start < (s->header_size >> BDRV_SECTOR_BITS)) {
/*
* There is not enough unused space to fit to block align between BAT
* and actual data. We can't avoid read-modify-write...
@@ -1403,11 +1398,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
for (i = 0; i < s->bat_size; i++) {
sector = bat2sect(s, i);
-if (sector + s->tracks > s->data_end) {
-s->data_end = sector + s->tracks;
+if (sector + s->tracks > file_nb_sectors) {
+need_check = true;
}
}
-need_check = need_check || s->data_end > file_nb_sectors;
ret = parallels_fill_used_bitmap(bs);
if (ret == -ENOMEM) {
@@ -1461,7 +1455,6 @@ static int
parallels_truncate_unused_clusters(BlockDriverState *bs)
end_off = (end_off + 1) * s->cluster_size;
}
end_off += s->data_start * BDRV_SECTOR_SIZE;
-s->data_end = end_off / BDRV_SECTOR_SIZE;
return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
}
diff --git a/block/parallels.h b/block/parallels.h
index 18b4f8068e..a6a048d890 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -79,7 +79,6 @@ typedef struct BDRVParallelsState {
unsigne
Re: [PATCH 15/19] parallels: Remove unnecessary data_end field
On 10/7/23 13:21, Mike Maslenkin wrote:
On Sat, Oct 7, 2023 at 1:18 PM Alexander Ivanov
wrote:
On 10/6/23 21:43, Mike Maslenkin wrote:
On Mon, Oct 2, 2023 at 12:01 PM Alexander Ivanov
wrote:
Since we have used bitmap, field data_end in BDRVParallelsState is
redundant and can be removed.
Add parallels_data_end() helper and remove data_end handling.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 33 +
block/parallels.h | 1 -
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 48ea5b3f03..80a7171b84 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -265,6 +265,13 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
g_free(s->used_bmap);
}
+static int64_t parallels_data_end(BDRVParallelsState *s)
+{
+int64_t data_end = s->data_start * BDRV_SECTOR_SIZE;
+data_end += s->used_bmap_size * s->cluster_size;
+return data_end;
+}
+
int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -275,7 +282,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-host_off = s->data_end * BDRV_SECTOR_SIZE;
+host_off = parallels_data_end(s);
prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
bytes = prealloc_clusters * s->cluster_size;
@@ -297,9 +304,6 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
-if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
-s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
-}
} else {
next_used = find_next_bit(s->used_bmap, s->used_bmap_size,
first_free);
@@ -315,8 +319,7 @@ int64_t parallels_allocate_host_clusters(BlockDriverState
*bs,
* branch. In the other case we are likely re-using hole. Preallocate
* the space if required by the prealloc_mode.
*/
-if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
-host_off < s->data_end * BDRV_SECTOR_SIZE) {
+if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
ret = bdrv_pwrite_zeroes(bs->file, host_off, bytes, 0);
if (ret < 0) {
return ret;
@@ -757,13 +760,7 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
}
}
-if (high_off == 0) {
-res->image_end_offset = s->data_end << BDRV_SECTOR_BITS;
-} else {
-res->image_end_offset = high_off + s->cluster_size;
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-}
-
+res->image_end_offset = parallels_data_end(s);
return 0;
}
@@ -806,7 +803,6 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
res->check_errors++;
return ret;
}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
parallels_free_used_bitmap(bs);
ret = parallels_fill_used_bitmap(bs);
@@ -1361,8 +1357,7 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
s->data_start = data_start;
-s->data_end = s->data_start;
-if (s->data_end < (s->header_size >> BDRV_SECTOR_BITS)) {
+if (s->data_start < (s->header_size >> BDRV_SECTOR_BITS)) {
/*
* There is not enough unused space to fit to block align between BAT
* and actual data. We can't avoid read-modify-write...
@@ -1403,11 +1398,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
for (i = 0; i < s->bat_size; i++) {
sector = bat2sect(s, i);
-if (sector + s->tracks > s->data_end) {
-s->data_end = sector + s->tracks;
+if (sector + s->tracks > file_nb_sectors) {
+need_check = true;
}
}
-need_check = need_check || s->data_end > file_nb_sectors;
ret = parallels_fill_used_bitmap(bs);
if (ret == -ENOMEM) {
@@ -1461,7 +1455,6 @@ static int
parallels_truncate_unused_clusters(BlockDriverState *bs)
end_off = (end_off + 1) * s->cluster_size;
}
end_off += s->data_start * BDRV_SECTOR_SIZE;
-s->data_end = end_off / BDRV_SECTOR_SIZE;
return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0,
NULL);
}
diff --git a/block/parallels.h b/block/parallels.h
i
[PATCH v2 19/20] tests: Turned on 256, 299, 304 and block-status-cache for parallels format
These tests pass with parallels format. Add parallels to supporting
formats for these tests.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/256 | 2 +-
tests/qemu-iotests/299 | 2 +-
tests/qemu-iotests/304 | 2 +-
tests/qemu-iotests/tests/block-status-cache | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/256 b/tests/qemu-iotests/256
index f34af6cef7..1a4c9c6885 100755
--- a/tests/qemu-iotests/256
+++ b/tests/qemu-iotests/256
@@ -26,7 +26,7 @@ from iotests import log
iotests.verify_virtio_scsi_pci_or_ccw()
-iotests.script_initialize(supported_fmts=['qcow2'])
+iotests.script_initialize(supported_fmts=['qcow2', 'parallels'])
size = 64 * 1024 * 1024
with iotests.FilePath('img0') as img0_path, \
diff --git a/tests/qemu-iotests/299 b/tests/qemu-iotests/299
index a7122941fd..d8c4399446 100755
--- a/tests/qemu-iotests/299
+++ b/tests/qemu-iotests/299
@@ -23,7 +23,7 @@ import iotests
# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
iotests.script_initialize(
-supported_fmts=['qcow2'],
+supported_fmts=['qcow2', 'parallels'],
)
nbd_sock = iotests.file_path('nbd.sock', base_dir=iotests.sock_dir)
diff --git a/tests/qemu-iotests/304 b/tests/qemu-iotests/304
index 198f282087..1ebf30 100755
--- a/tests/qemu-iotests/304
+++ b/tests/qemu-iotests/304
@@ -23,7 +23,7 @@
import iotests
from iotests import qemu_img_create, qemu_img_log, file_path
-iotests.script_initialize(supported_fmts=['qcow2'],
+iotests.script_initialize(supported_fmts=['qcow2', 'parallels'],
supported_protocols=['file'])
test_img = file_path('test.qcow2')
diff --git a/tests/qemu-iotests/tests/block-status-cache
b/tests/qemu-iotests/tests/block-status-cache
index 5a7bc2c149..ade3d5b169 100755
--- a/tests/qemu-iotests/tests/block-status-cache
+++ b/tests/qemu-iotests/tests/block-status-cache
@@ -131,5 +131,5 @@ class TestBscWithNbd(iotests.QMPTestCase):
if __name__ == '__main__':
# The block-status cache only works on the protocol layer, so to test it,
# we can only use the raw format
-iotests.main(supported_fmts=['raw'],
+iotests.main(supported_fmts=['raw', 'parallels'],
supported_protocols=['file'])
--
2.34.1
[PATCH v2 14/20] parallels: Reverse a conditional in parallels_check_leak() to reduce indents
Let the function return a success code if a file size is not bigger than
image_end_offset. Thus we can decrease indents in the next code block.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 72 +++
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 2198eff21e..2d54459087 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -775,7 +775,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
BdrvCheckMode fix, bool explicit)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size;
+int64_t size, count;
int ret;
size = bdrv_co_getlength(bs->file->bs);
@@ -783,43 +783,43 @@ parallels_check_leak(BlockDriverState *bs,
BdrvCheckResult *res,
res->check_errors++;
return size;
}
+if (size <= res->image_end_offset) {
+return 0;
+}
+
+count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
+if (explicit) {
+fprintf(stderr,
+"%s space leaked at the end of the image %" PRId64 "\n",
+fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
+size - res->image_end_offset);
+res->leaks += count;
+}
+if (fix & BDRV_FIX_LEAKS) {
+Error *local_err = NULL;
+
+/*
+ * In order to really repair the image, we must shrink it.
+ * That means we have to pass exact=true.
+ */
+ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
+ PREALLOC_MODE_OFF, 0, _err);
+if (ret < 0) {
+error_report_err(local_err);
+res->check_errors++;
+return ret;
+}
+s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+res->check_errors++;
+return ret;
+}
-if (size > res->image_end_offset) {
-int64_t count;
-count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
if (explicit) {
-fprintf(stderr,
-"%s space leaked at the end of the image %" PRId64 "\n",
-fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-size - res->image_end_offset);
-res->leaks += count;
-}
-if (fix & BDRV_FIX_LEAKS) {
-Error *local_err = NULL;
-
-/*
- * In order to really repair the image, we must shrink it.
- * That means we have to pass exact=true.
- */
-ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
- PREALLOC_MODE_OFF, 0, _err);
-if (ret < 0) {
-error_report_err(local_err);
-res->check_errors++;
-return ret;
-}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-parallels_free_used_bitmap(bs);
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-res->check_errors++;
-return ret;
-}
-
-if (explicit) {
-res->leaks_fixed += count;
-}
+res->leaks_fixed += count;
}
}
--
2.34.1
[PATCH v2 18/20] tests: Add parallels images support to test 165
Use a different bitmap name for parallels images because their has own ID
format, and can't contain an arbitrary string.
Replace image reopen by shutdown/launch VM because parallels images doesn't
support reopen.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/165 | 40 +---
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
index b24907a62f..f732db257c 100755
--- a/tests/qemu-iotests/165
+++ b/tests/qemu-iotests/165
@@ -38,6 +38,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def setUp(self):
qemu_img('create', '-f', iotests.imgfmt, disk, str(disk_size))
+if iotests.imgfmt == 'parallels':
+self.bitmap_name = '----'
+else:
+self.bitmap_name = 'bitmap0'
def tearDown(self):
os.remove(disk)
@@ -50,12 +54,12 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def getSha256(self):
result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
- node='drive0', name='bitmap0')
+ node='drive0', name=self.bitmap_name)
return result['return']['sha256']
def checkBitmap(self, sha256):
result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
- node='drive0', name='bitmap0')
+ node='drive0', name=self.bitmap_name)
self.assert_qmp(result, 'return/sha256', sha256);
def writeRegions(self, regions):
@@ -65,7 +69,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
def qmpAddBitmap(self):
self.vm.qmp('block-dirty-bitmap-add', node='drive0',
-name='bitmap0', persistent=True)
+name=self.bitmap_name, persistent=True)
def test_persistent(self):
self.vm = self.mkVm()
@@ -117,7 +121,7 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
assert sha256_1 != sha256_2 # Otherwise, it's not very interesting.
self.vm.cmd('block-dirty-bitmap-clear', node='drive0',
-name='bitmap0')
+name=self.bitmap_name)
# Start with regions1
@@ -135,16 +139,22 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
self.writeRegions(regions2)
assert sha256_1 == self.getSha256()
-# Reopen to RW
-self.vm.cmd('blockdev-reopen', options=[{
-'node-name': 'node0',
-'driver': iotests.imgfmt,
-'file': {
-'driver': 'file',
-'filename': disk
-},
-'read-only': False
-}])
+if iotests.imgfmt == 'parallels':
+# parallels doesn't support reopen
+self.vm.shutdown()
+self.vm = self.mkVm()
+self.vm.launch()
+else:
+# Reopen to RW
+self.vm.cmd('blockdev-reopen', options=[{
+'node-name': 'node0',
+'driver': iotests.imgfmt,
+'file': {
+'driver': 'file',
+'filename': disk
+},
+'read-only': False
+}])
# Check that bitmap is reopened to RW and we can write to it.
self.writeRegions(regions2)
@@ -154,6 +164,6 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
if __name__ == '__main__':
-iotests.main(supported_fmts=['qcow2'],
+iotests.main(supported_fmts=['qcow2', 'parallels'],
supported_protocols=['file'],
unsupported_imgopts=['compat'])
--
2.34.1
[PATCH v2 01/20] parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()
After used bitmap freeng s->used_bmap points to the freed memory. If we try
to free used bitmap one more time it leads to double free error.
Set s->used_bmap to NULL to exclude double free error.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index 6b46623241..ba1fdde259 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -244,6 +244,7 @@ static void parallels_free_used_bitmap(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
s->used_bmap_size = 0;
+s->used_bmap = NULL;
g_free(s->used_bmap);
}
--
2.34.1
[PATCH v2 20/20] tests: Add parallels format support to image-fleecing
Use a different bitmap name for parallels images because their has own ID
format, and can't contain an arbitrary string.
Replace hardcoded 'qcow2' format to iotests.imgfmt.
Add 'parallels' to supported formats.
Signed-off-by: Alexander Ivanov
---
tests/qemu-iotests/tests/image-fleecing | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tests/qemu-iotests/tests/image-fleecing
b/tests/qemu-iotests/tests/image-fleecing
index 5e3b2c7e46..dd940b7203 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -28,7 +28,7 @@ import iotests
from iotests import log, qemu_img, qemu_io
iotests.script_initialize(
-supported_fmts=['qcow2'],
+supported_fmts=['qcow2', 'parallels'],
supported_platforms=['linux'],
required_fmts=['copy-before-write'],
unsupported_imgopts=['compat']
@@ -61,12 +61,17 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
if push_backup:
assert use_cbw
+if iotests.imgfmt == 'parallels':
+bitmap_name = '----'
+else:
+bitmap_name = 'bitmap0'
+
log('--- Setting up images ---')
log('')
qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M')
if bitmap:
-qemu_img('bitmap', '--add', base_img_path, 'bitmap0')
+qemu_img('bitmap', '--add', base_img_path, bitmap_name)
if use_snapshot_access_filter:
assert use_cbw
@@ -75,7 +80,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
qemu_img('create', '-f', 'qcow2', fleece_img_path, '64M')
if push_backup:
-qemu_img('create', '-f', 'qcow2', target_img_path, '64M')
+qemu_img('create', '-f', iotests.imgfmt, target_img_path, '64M')
for p in patterns:
qemu_io('-f', iotests.imgfmt,
@@ -130,7 +135,7 @@ def do_test(vm, use_cbw, use_snapshot_access_filter,
base_img_path,
}
if bitmap:
-fl_cbw['bitmap'] = {'node': src_node, 'name': 'bitmap0'}
+fl_cbw['bitmap'] = {'node': src_node, 'name': bitmap_name}
log(vm.qmp('blockdev-add', fl_cbw))
--
2.34.1
[PATCH v2 08/20] parallels: Create used bitmap even if checks needed
All the checks were fixed to work with used bitmap. Create used bitmap in
parallels_open() even if need_check is true.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index dd8ec925f3..a22ab7f2fc 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1422,13 +1422,11 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
need_check = need_check || s->data_end > file_nb_sectors;
-if (!need_check) {
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-goto fail;
-}
-need_check = need_check || ret < 0; /* These are correctable errors */
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+goto fail;
}
+need_check = need_check || ret < 0; /* These are correctable errors */
/*
* We don't repair the image here if it's opened for checks. Also we don't
--
2.34.1
[PATCH v2 10/20] parallels: Add dirty bitmaps saving
Now dirty bitmaps can be loaded but there is no their saving. Add code for
dirty bitmap storage.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 167 ++
block/parallels.c | 16 +++-
block/parallels.h | 5 ++
3 files changed, 186 insertions(+), 2 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 8a109f005a..0a632a2331 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -24,6 +24,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/error-report.h"
#include "qapi/error.h"
#include "block/block-io.h"
#include "block/block_int.h"
@@ -301,3 +302,169 @@ out:
return ret;
}
+
+static void parallels_save_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
*bitmap,
+ uint8_t **buf, int *buf_size)
+{
+BDRVParallelsState *s = bs->opaque;
+ParallelsFeatureHeader *fh;
+ParallelsDirtyBitmapFeature *bh;
+uint64_t *l1_table, l1_size, granularity, limit;
+int64_t bm_size, ser_size, offset, buf_used;
+int64_t alloc_size = 1;
+const char *name;
+uint8_t *bm_buf;
+QemuUUID uuid;
+int ret = 0;
+
+if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
+bdrv_dirty_bitmap_inconsistent(bitmap)) {
+return;
+}
+
+bm_size = bdrv_dirty_bitmap_size(bitmap);
+granularity = bdrv_dirty_bitmap_granularity(bitmap);
+limit = bdrv_dirty_bitmap_serialization_coverage(s->cluster_size, bitmap);
+ser_size = bdrv_dirty_bitmap_serialization_size(bitmap, 0, bm_size);
+l1_size = DIV_ROUND_UP(ser_size, s->cluster_size);
+
+buf_used = l1_size * 8 + sizeof(*fh) + sizeof(*bh);
+/* Check if there is enough space for the final section */
+if (*buf_size - buf_used < sizeof(*fh)) {
+return;
+}
+
+name = bdrv_dirty_bitmap_name(bitmap);
+ret = qemu_uuid_parse(name, );
+if (ret < 0) {
+error_report("Can't save dirty bitmap: ID parsing error: '%s'", name);
+return;
+}
+
+fh = (ParallelsFeatureHeader *)*buf;
+bh = (ParallelsDirtyBitmapFeature *)(*buf + sizeof(*fh));
+l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
+
+fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
+fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
+
+bh->l1_size = cpu_to_le32(l1_size);
+bh->size = cpu_to_le64(bm_size >> BDRV_SECTOR_BITS);
+bh->granularity = cpu_to_le32(granularity >> BDRV_SECTOR_BITS);
+memcpy(bh->id, , sizeof(uuid));
+
+bm_buf = qemu_blockalign(bs, s->cluster_size);
+
+offset = 0;
+while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >=
0) {
+uint64_t idx = offset / limit;
+int64_t cluster_off, end, write_size;
+
+offset = QEMU_ALIGN_DOWN(offset, limit);
+end = MIN(bm_size, offset + limit);
+write_size = bdrv_dirty_bitmap_serialization_size(bitmap, offset,
+ end - offset);
+assert(write_size <= s->cluster_size);
+
+bdrv_dirty_bitmap_serialize_part(bitmap, bm_buf, offset, end - offset);
+if (write_size < s->cluster_size) {
+memset(bm_buf + write_size, 0, s->cluster_size - write_size);
+}
+
+cluster_off = parallels_allocate_host_clusters(bs, _size);
+if (cluster_off <= 0) {
+goto end;
+}
+
+ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, bm_buf, 0);
+if (ret < 0) {
+memset(>magic, 0, sizeof(fh->magic));
+parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+ cluster_off, 1);
+goto end;
+}
+
+l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS);
+offset = end;
+}
+
+*buf_size -= buf_used;
+*buf += buf_used;
+
+end:
+qemu_vfree(bm_buf);
+}
+
+void parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+ Error **errp)
+{
+BDRVParallelsState *s = bs->opaque;
+BdrvDirtyBitmap *bitmap;
+ParallelsFormatExtensionHeader *eh;
+int remaining = s->cluster_size;
+uint8_t *buf, *pos;
+int64_t header_off, alloc_size = 1;
+g_autofree uint8_t *hash = NULL;
+size_t hash_len = 0;
+int ret;
+
+s->header->ext_off = 0;
+
+if (!bdrv_has_named_bitmaps(bs)) {
+return;
+}
+
+buf = qemu_blockalign0(bs, s->cluster_size);
+
+eh = (ParallelsFormatExtensionHeader *)buf;
+pos = buf + sizeof(*eh);
+
+eh->magic = cpu_to_le64(PARALLELS_FORMAT_EXTENSION_MAGIC);
+
+FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+parallels_save_bitmap(bs, bitmap, , );
+}
+
+header_off = parallels_allocate_host_clusters(bs, _size);
+if (head
[PATCH v2 03/20] parallels: Add mark_unused() helper
Add a helper to set unused areas in used bitmap.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 7eb92f87ab..6a4e3945c6 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -195,6 +195,23 @@ static int mark_used(BlockDriverState *bs, unsigned long
*bitmap,
return 0;
}
+static int mark_unused(BlockDriverState *bs, unsigned long *bitmap,
+ uint32_t bitmap_size, int64_t off, uint32_t count)
+{
+BDRVParallelsState *s = bs->opaque;
+uint32_t cluster_index = host_cluster_index(s, off);
+unsigned long next_unused;
+if (cluster_index + count > bitmap_size) {
+return -E2BIG;
+}
+next_unused = find_next_zero_bit(bitmap, bitmap_size, cluster_index);
+if (next_unused < cluster_index + count) {
+return -EINVAL;
+}
+bitmap_clear(bitmap, cluster_index, count);
+return 0;
+}
+
/*
* Collect used bitmap. The image can contain errors, we should fill the
* bitmap anyway, as much as we can. This information will be used for
--
2.34.1
[PATCH v2 05/20] parallels: Set data_end value in parallels_check_leak()
In parallels_check_leak() we change file size but don't correct data_end
field of BDRVParallelsState structure. Fix it.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index ce97387e3e..ba9fc46279 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -806,6 +806,7 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
res->check_errors++;
return ret;
}
+s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
if (explicit) {
res->leaks_fixed += count;
}
--
2.34.1
[PATCH v2 11/20] parallels: Let image extensions work in RW mode
Now we support extensions saving and can let to work with them in
read-write mode.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 4
block/parallels.c | 17 -
2 files changed, 4 insertions(+), 17 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 0a632a2331..a0de4f1b07 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -177,10 +177,6 @@ static BdrvDirtyBitmap
*parallels_load_bitmap(BlockDriverState *bs,
return NULL;
}
-/* We support format extension only for RO parallels images. */
-assert(!(bs->open_flags & BDRV_O_RDWR));
-bdrv_dirty_bitmap_set_readonly(bitmap, true);
-
return bitmap;
}
diff --git a/block/parallels.c b/block/parallels.c
index bb1e765ec8..2198eff21e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1374,19 +1374,10 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
if (ph.ext_off) {
-if (flags & BDRV_O_RDWR) {
-/*
- * It's unsafe to open image RW if there is an extension (as we
- * don't support it). But parallels driver in QEMU historically
- * ignores the extension, so print warning and don't care.
- */
-warn_report("Format Extension ignored in RW mode");
-} else {
-ret = parallels_read_format_extension(
-bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
-if (ret < 0) {
-goto fail;
-}
+ret = parallels_read_format_extension(
+bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+if (ret < 0) {
+goto fail;
}
}
--
2.34.1
[PATCH v2 00/20] parallels: Add full dirty bitmap support
Parallels format driver: * make some preparation * add dirty bitmap saving * make dirty bitmap RW * fix broken checks * refactor leak check * add parallels format support to several tests You could find these patches in my repo: https://github.com/AlexanderIvanov-Virtuozzo/qemu/tree/parallels-v2 v2: 1: New patch to fix double free error. 4: Fixed clusters leaks. 15: Fixed (end_off != s->used_bmap_size) handling in parallels_truncate_unused_clusters(). 16,17: Changed the sequence of the patches - in this way we have correct leaks check. Alexander Ivanov (20): parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap() parallels: Move inactivation code to a separate function parallels: Add mark_unused() helper parallels: Move host clusters allocation to a separate function parallels: Set data_end value in parallels_check_leak() parallels: Recreate used bitmap in parallels_check_leak() parallels: Add a note about used bitmap in parallels_check_duplicate() parallels: Create used bitmap even if checks needed parallels: Make mark_used() and mark_unused() global functions parallels: Add dirty bitmaps saving parallels: Let image extensions work in RW mode parallels: Handle L1 entries equal to one parallels: Make a loaded dirty bitmap persistent parallels: Reverse a conditional in parallels_check_leak() to reduce indents parallels: Truncate images on the last used cluster parallels: Check unused clusters in parallels_check_leak() parallels: Remove unnecessary data_end field tests: Add parallels images support to test 165 tests: Turned on 256, 299, 304 and block-status-cache for parallels format tests: Add parallels format support to image-fleecing block/parallels-ext.c | 182 +- block/parallels.c | 361 block/parallels.h | 15 +- tests/qemu-iotests/165 | 40 ++- tests/qemu-iotests/256 | 2 +- tests/qemu-iotests/299 | 2 +- tests/qemu-iotests/304 | 2 +- tests/qemu-iotests/tests/block-status-cache | 2 +- tests/qemu-iotests/tests/image-fleecing | 13 +- 9 files changed, 447 insertions(+), 172 deletions(-) -- 2.34.1
[PATCH v2 04/20] parallels: Move host clusters allocation to a separate function
For parallels images extensions we need to allocate host clusters
without any connection to BAT. Move host clusters allocation code to
allocate_host_clusters().
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 126 --
block/parallels.h | 4 ++
2 files changed, 71 insertions(+), 59 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 6a4e3945c6..ce97387e3e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -265,58 +265,31 @@ static void parallels_free_used_bitmap(BlockDriverState
*bs)
g_free(s->used_bmap);
}
-static int64_t coroutine_fn GRAPH_RDLOCK
-allocate_clusters(BlockDriverState *bs, int64_t sector_num,
- int nb_sectors, int *pnum)
+int64_t parallels_allocate_host_clusters(BlockDriverState *bs,
+ int64_t *clusters)
{
-int ret = 0;
BDRVParallelsState *s = bs->opaque;
-int64_t i, pos, idx, to_allocate, first_free, host_off;
-
-pos = block_status(s, sector_num, nb_sectors, pnum);
-if (pos > 0) {
-return pos;
-}
-
-idx = sector_num / s->tracks;
-to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
-
-/*
- * This function is called only by parallels_co_writev(), which will never
- * pass a sector_num at or beyond the end of the image (because the block
- * layer never passes such a sector_num to that function). Therefore, idx
- * is always below s->bat_size.
- * block_status() will limit *pnum so that sector_num + *pnum will not
- * exceed the image end. Therefore, idx + to_allocate cannot exceed
- * s->bat_size.
- * Note that s->bat_size is an unsigned int, therefore idx + to_allocate
- * will always fit into a uint32_t.
- */
-assert(idx < s->bat_size && idx + to_allocate <= s->bat_size);
+int64_t first_free, next_used, host_off, prealloc_clusters;
+int64_t bytes, prealloc_bytes;
+uint32_t new_usedsize;
+int ret = 0;
first_free = find_first_zero_bit(s->used_bmap, s->used_bmap_size);
if (first_free == s->used_bmap_size) {
-uint32_t new_usedsize;
-int64_t bytes = to_allocate * s->cluster_size;
-bytes += s->prealloc_size * BDRV_SECTOR_SIZE;
-
host_off = s->data_end * BDRV_SECTOR_SIZE;
+prealloc_clusters = *clusters + s->prealloc_size / s->tracks;
+bytes = *clusters * s->cluster_size;
+prealloc_bytes = prealloc_clusters * s->cluster_size;
-/*
- * We require the expanded size to read back as zero. If the
- * user permitted truncation, we try that; but if it fails, we
- * force the safer-but-slower fallocate.
- */
if (s->prealloc_mode == PRL_PREALLOC_MODE_TRUNCATE) {
-ret = bdrv_co_truncate(bs->file, host_off + bytes,
- false, PREALLOC_MODE_OFF,
- BDRV_REQ_ZERO_WRITE, NULL);
+ret = bdrv_truncate(bs->file, host_off + prealloc_bytes, false,
+PREALLOC_MODE_OFF, BDRV_REQ_ZERO_WRITE, NULL);
if (ret == -ENOTSUP) {
s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
}
}
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE) {
-ret = bdrv_co_pwrite_zeroes(bs->file, host_off, bytes, 0);
+ret = bdrv_pwrite_zeroes(bs->file, host_off, prealloc_bytes, 0);
}
if (ret < 0) {
return ret;
@@ -326,15 +299,15 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
s->used_bmap = bitmap_zero_extend(s->used_bmap, s->used_bmap_size,
new_usedsize);
s->used_bmap_size = new_usedsize;
+if (host_off + bytes > s->data_end * BDRV_SECTOR_SIZE) {
+s->data_end = (host_off + bytes) / BDRV_SECTOR_SIZE;
+}
} else {
-int64_t next_used;
next_used = find_next_bit(s->used_bmap, s->used_bmap_size, first_free);
/* Not enough continuous clusters in the middle, adjust the size */
-if (next_used - first_free < to_allocate) {
-to_allocate = next_used - first_free;
-*pnum = (idx + to_allocate) * s->tracks - sector_num;
-}
+*clusters = MIN(*clusters, next_used - first_free);
+bytes = *clusters * s->cluster_size;
host_off = s->data_start * BDRV_SECTOR_SIZE;
host_off += first_free * s->cluster_size;
@@ -346,14 +319,58 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
if (s->prealloc_mode == PRL_PREALLOC_MODE_FALLOCATE &&
host_off < s->data_end * BDRV_SECTOR_SIZE) {
-ret = bdrv_co_pwrite_zeroes(bs->file, host_off,
-
[PATCH v2 13/20] parallels: Make a loaded dirty bitmap persistent
After bitmap loading the bitmap is not persistent and is removed on image
saving. Set bitmap persistence to true.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index ebda6b0a01..bb4478c350 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -256,6 +256,7 @@ static int
parallels_parse_format_extension(BlockDriverState *bs,
if (!bitmap) {
goto fail;
}
+bdrv_dirty_bitmap_set_persistence(bitmap, true);
bitmaps = g_slist_append(bitmaps, bitmap);
break;
--
2.34.1
[PATCH v2 12/20] parallels: Handle L1 entries equal to one
If all the bits in a dirty bitmap cluster are ones, the cluster shouldn't
be written. Instead the corresponding L1 entry should be set to 1.
Check if all bits in a memory region are ones and set 1 to L1 entries
corresponding clusters filled with ones.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index a0de4f1b07..ebda6b0a01 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void parallels_save_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
offset = 0;
while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >=
0) {
uint64_t idx = offset / limit;
-int64_t cluster_off, end, write_size;
+int64_t cluster_off, end, write_size, first_zero;
offset = QEMU_ALIGN_DOWN(offset, limit);
end = MIN(bm_size, offset + limit);
@@ -367,6 +367,16 @@ static void parallels_save_bitmap(BlockDriverState *bs,
BdrvDirtyBitmap *bitmap,
memset(bm_buf + write_size, 0, s->cluster_size - write_size);
}
+first_zero = bdrv_dirty_bitmap_next_zero(bitmap, offset, bm_size);
+if (first_zero < 0) {
+goto end;
+}
+if (first_zero - offset >= s->cluster_size) {
+l1_table[idx] = 1;
+offset = end;
+continue;
+}
+
cluster_off = parallels_allocate_host_clusters(bs, _size);
if (cluster_off <= 0) {
goto end;
--
2.34.1
[PATCH v2 06/20] parallels: Recreate used bitmap in parallels_check_leak()
In parallels_check_leak() file can be truncated. In this case the used
bitmap would not comply to the file. Recreate the bitmap after file
truncation.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index ba9fc46279..dad2bf67cf 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -807,6 +807,14 @@ parallels_check_leak(BlockDriverState *bs, BdrvCheckResult
*res,
return ret;
}
s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret == -ENOMEM) {
+res->check_errors++;
+return ret;
+}
+
if (explicit) {
res->leaks_fixed += count;
}
--
2.34.1
[PATCH v2 15/20] parallels: Truncate images on the last used cluster
On an image closing there can be unused clusters in the end of the image.
Truncate these clusters and update data_end field.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 2d54459087..c7db2a9562 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1458,6 +1458,23 @@ fail:
return ret;
}
+static int parallels_truncate_unused_clusters(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+uint64_t end_off = 0;
+if (s->used_bmap_size > 0) {
+end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+if (end_off == s->used_bmap_size) {
+end_off = 0;
+} else {
+end_off = (end_off + 1) * s->cluster_size;
+}
+}
+end_off += s->data_start * BDRV_SECTOR_SIZE;
+s->data_end = end_off / BDRV_SECTOR_SIZE;
+return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+}
+
static int parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -1475,8 +1492,7 @@ static int parallels_inactivate(BlockDriverState *bs)
parallels_update_header(bs);
/* errors are ignored, so we might as well pass exact=true */
-ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS,
-true, PREALLOC_MODE_OFF, 0, NULL);
+ret = parallels_truncate_unused_clusters(bs);
if (ret < 0) {
error_report("Failed to truncate image: %s", strerror(-ret));
}
--
2.34.1
[PATCH v2 02/20] parallels: Move inactivation code to a separate function
We are going to add parallels image extensions storage and need a separate
function for inactivation code.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index ba1fdde259..7eb92f87ab 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1430,18 +1430,27 @@ fail:
return ret;
}
+static int parallels_inactivate(BlockDriverState *bs)
+{
+BDRVParallelsState *s = bs->opaque;
+int ret;
+
+s->header->inuse = 0;
+parallels_update_header(bs);
+
+/* errors are ignored, so we might as well pass exact=true */
+ret = bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
+PREALLOC_MODE_OFF, 0, NULL);
+
+return ret;
+}
static void parallels_close(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE))
{
-s->header->inuse = 0;
-parallels_update_header(bs);
-
-/* errors are ignored, so we might as well pass exact=true */
-bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, true,
- PREALLOC_MODE_OFF, 0, NULL);
+parallels_inactivate(bs);
}
parallels_free_used_bitmap(bs);
@@ -1481,6 +1490,7 @@ static BlockDriver bdrv_parallels = {
.bdrv_co_check = parallels_co_check,
.bdrv_co_pdiscard = parallels_co_pdiscard,
.bdrv_co_pwrite_zeroes = parallels_co_pwrite_zeroes,
+.bdrv_inactivate= parallels_inactivate,
};
static void bdrv_parallels_init(void)
--
2.34.1
[PATCH v2 07/20] parallels: Add a note about used bitmap in parallels_check_duplicate()
In parallels_check_duplicate() We use a bitmap for duplication detection. This bitmap is not related to used_bmap field in BDRVParallelsState. Add a comment about it to avoid confusion. Signed-off-by: Alexander Ivanov --- block/parallels.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index dad2bf67cf..dd8ec925f3 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -837,7 +837,10 @@ parallels_check_duplicate(BlockDriverState *bs, BdrvCheckResult *res, bool fixed = false; /* - * Create a bitmap of used clusters. + * Create a bitmap of used clusters. Please note that this bitmap is not + * related to used_bmap field in BDRVParallelsState and is created only for + * local usage. + * * If a bit is set, there is a BAT entry pointing to this cluster. * Loop through the BAT entries, check bits relevant to an entry offset. * If bit is set, this entry is duplicated. Otherwise set the bit. -- 2.34.1
[PATCH v2 16/20] parallels: Check unused clusters in parallels_check_leak()
Since we have used bitmap, leak check is useless. Transform
parallels_truncate_unused_clusters() to parallels_check_unused_clusters()
helper and use it in leak check.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 121 +-
1 file changed, 67 insertions(+), 54 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index c7db2a9562..b1100dc65c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -770,57 +770,87 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
return 0;
}
+static int64_t parallels_check_unused_clusters(BlockDriverState *bs,
+ bool truncate)
+{
+BDRVParallelsState *s = bs->opaque;
+int64_t leak, file_size, end_off = 0;
+int ret;
+
+file_size = bdrv_getlength(bs->file->bs);
+if (file_size < 0) {
+return file_size;
+}
+
+if (s->used_bmap_size > 0) {
+end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
+if (end_off == s->used_bmap_size) {
+end_off = 0;
+} else {
+end_off = (end_off + 1) * s->cluster_size;
+}
+}
+
+end_off += s->data_start * BDRV_SECTOR_SIZE;
+leak = file_size - end_off;
+if (leak < 0) {
+return -EINVAL;
+}
+if (!truncate || leak == 0) {
+return leak;
+}
+
+ret = bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
+if (ret) {
+return ret;
+}
+
+s->data_end = end_off / BDRV_SECTOR_SIZE;
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret < 0) {
+return ret;
+}
+
+return leak;
+}
+
static int coroutine_fn GRAPH_RDLOCK
parallels_check_leak(BlockDriverState *bs, BdrvCheckResult *res,
BdrvCheckMode fix, bool explicit)
{
BDRVParallelsState *s = bs->opaque;
-int64_t size, count;
-int ret;
+int64_t leak, count, size;
+
+leak = parallels_check_unused_clusters(bs, fix & BDRV_FIX_LEAKS);
+if (leak < 0) {
+res->check_errors++;
+return leak;
+}
+if (leak == 0) {
+return 0;
+}
size = bdrv_co_getlength(bs->file->bs);
if (size < 0) {
res->check_errors++;
return size;
}
-if (size <= res->image_end_offset) {
+res->image_end_offset = size;
+
+if (!explicit) {
return 0;
}
-count = DIV_ROUND_UP(size - res->image_end_offset, s->cluster_size);
-if (explicit) {
-fprintf(stderr,
-"%s space leaked at the end of the image %" PRId64 "\n",
-fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR",
-size - res->image_end_offset);
-res->leaks += count;
-}
+count = DIV_ROUND_UP(leak, s->cluster_size);
+fprintf(stderr,
+"%s space leaked at the end of the image %" PRId64 "\n",
+fix & BDRV_FIX_LEAKS ? "Repairing" : "ERROR", leak);
+res->leaks += count;
+
if (fix & BDRV_FIX_LEAKS) {
-Error *local_err = NULL;
-
-/*
- * In order to really repair the image, we must shrink it.
- * That means we have to pass exact=true.
- */
-ret = bdrv_co_truncate(bs->file, res->image_end_offset, true,
- PREALLOC_MODE_OFF, 0, _err);
-if (ret < 0) {
-error_report_err(local_err);
-res->check_errors++;
-return ret;
-}
-s->data_end = res->image_end_offset >> BDRV_SECTOR_BITS;
-
-parallels_free_used_bitmap(bs);
-ret = parallels_fill_used_bitmap(bs);
-if (ret == -ENOMEM) {
-res->check_errors++;
-return ret;
-}
-
-if (explicit) {
-res->leaks_fixed += count;
-}
+res->leaks_fixed += count;
}
return 0;
@@ -1458,23 +1488,6 @@ fail:
return ret;
}
-static int parallels_truncate_unused_clusters(BlockDriverState *bs)
-{
-BDRVParallelsState *s = bs->opaque;
-uint64_t end_off = 0;
-if (s->used_bmap_size > 0) {
-end_off = find_last_bit(s->used_bmap, s->used_bmap_size);
-if (end_off == s->used_bmap_size) {
-end_off = 0;
-} else {
-end_off = (end_off + 1) * s->cluster_size;
-}
-}
-end_off += s->data_start * BDRV_SECTOR_SIZE;
-s->data_end = end_off / BDRV_SECTOR_SIZE;
-return bdrv_truncate(bs->file, end_off, true, PREALLOC_MODE_OFF, 0, NULL);
-}
-
static int parallels_inactivate(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
@@ -1492,7 +1505,7 @@ static int parallels_inactivate(BlockDriverState *bs)
parallels_update_he
