Re: [PATCH] qga: Add an interactive mode to guest-exec via VSOCK for Linux
On 5/22/24 18:10, Daniel P. Berrangé wrote: On Wed, May 22, 2024 at 05:06:57PM +0200, Alexander Ivanov wrote: Add an interactive mode to the guest-exec command in the QEMU Guest Agent using the VSOCK communication mechanism. It enables interactive sessions with the executed command in the guest, allowing real-time input/output. Introduce "interactive" mode in the GuestExecCaptureOutputMode enumeration and add optional "cid" and "port" fields to the guest-exec response. In such a way user can execute guest-exec command, get CID and port number from the response and connect to the guest server. After connection user can communicate with the started process. All the data transmitted to the server is redirected to stdin. Data from stdout and stderr is redirected to the client. All data blocks are preceded by 32-bit headers (network byte order): most significant bit contains a sign of stream (stdout - 0, stderr - 1), all the other bits contain the payload size. Every patch to 'guest-exec' takes us torwards re-inventing yet more SSH/telnet functionality, but a poor simulation of it. For exmaple this still lacks any separation of stdout/stderr streams, just There IS separation of stdout/stderr. Receiving data on the host you can see from which stream it is. interleaving all their data back to the host. There is also zero access control facilities beyond turning off the 'guest-exec' command entirely. IMHO we should really consider "arbitrary command execution" to be something to be handled by a separate process. Let the guest OS admin decide separately from running QEMU GA, whether they want to enable arbitrary host processes to have a trival privileged backdoor into their guest. systemd now supports exposing SSH over VSOCK, and provides an SSH proxy in the host to connect to VMs, while libvirt also has added its own host SSH proxy to allow SSH based on libvirt VM name. For windows guests, there is something called PowerShell Direct which exposes PowerShell over vmbus under HyperV. Possibly that can be enabled in QEMU too if someone understands windows & vmbus enough... ? With regards, Daniel Otherwise, you are right, it makes sense to use SSH over VSOCK. Thank you. -- Best regards, Alexander Ivanov
Test scripts
There are two python scripts in the attachment:
vsock_guest_exec_simple.py - simple example of a client;
vsock_guest_exec_test.py - tests with different payload size.
The last file should be copied to a guest VM. Edit SRV_PATH variable
in the host copy of the script - there should be path to the directory
containing a copy of the script in VM. Execute the host script with
net arguments:
./vsock_guest_exec_test.py srv
--
Best regards,
Alexander Ivanov
#!/usr/bin/python3
import sys, os, struct, subprocess, json, socket
TYPE_MASK = 0x8000
def parse_block_header(data):
res = struct.unpack('!I', data)
size = res[0]
if size & TYPE_MASK:
size -= TYPE_MASK
tp = 'err'
else:
tp = 'out'
return (size, tp)
def recv_block(sock):
hdr = sock.recv(4)
if not hdr:
print('ERROR: header receiving')
sys.exit(-1)
size, tp = parse_block_header(hdr)
res = b''
received = 0
while size > 0:
part = sock.recv(size)
res += part
size -= len(part)
return (res, tp)
def guest_exec(vm_name):
print('run guest-exec command...')
cmd = ['virsh',
'qemu-agent-command',
vm_name,
'{"execute":"guest-exec", "arguments":{"path": "bash", "capture-output": "interactive"}}']
p = subprocess.run(cmd, stdout=subprocess.PIPE)
response = p.stdout.decode('utf-8')
print('response: %s' % response.strip())
response = json.loads(response)['return']
cid = int(response['cid'])
port = int(response['port'])
return (cid, port)
def srv_conn(cid, port):
sock = socket.socket(socket.AF_VSOCK, socket.SOCK_STREAM)
print('connect...')
sock.connect((cid, port))
print('connected')
return sock
def main():
if len(sys.argv) != 2:
print('Usage:\n\t%s ' % sys.argv[0])
return
vm_name = sys.argv[1]
cid, port = guest_exec(vm_name)
sock = srv_conn(cid, port)
sock.send(b'echo "Hello world!"\n')
data, tp = recv_block(sock)
print('Received from %s: "%s"' % (tp, data.decode('utf-8')))
main()
#!/usr/bin/python3
import sys, os, time, struct, hashlib, subprocess, json, socket, random
SRV_PATH = '/home/user/'
PKT_HDR_FMT = '!Ib32s'
TYPE_MASK = 0x8000
TYPE_OUT = 1
TYPE_ERR = 2
def parse_block_header(data):
res = struct.unpack('!I', data)
size = res[0]
if size & TYPE_MASK:
size -= TYPE_MASK
tp = 'err'
else:
tp = 'out'
return (size, tp)
def make_pkt(size, tp):
payload = os.urandom(size)
digest = hashlib.sha256(payload).digest()
return struct.pack(PKT_HDR_FMT, size, tp, digest) + payload
def recv_block(sock):
hdr = b''
while len(hdr) < 4:
part = sock.recv(4 - len(hdr))
if not part:
return (None, None)
hdr += part
size, tp = parse_block_header(hdr)
res = b''
received = 0
while size > 0:
part = sock.recv(size)
res += part
size -= len(part)
return (res, tp)
def recv_pkt(sock, spkt):
hdr_size = struct.calcsize(PKT_HDR_FMT)
data = b''
size = None
tries = 0
while True:
block, btp = recv_block(sock)
if block == None:
if len(data) == 0:
time.sleep(0.01)
tries += 1
print('retry')
if tries > 10:
print('Connection closed')
return
else:
data += block
tries = 0
if len(data) < hdr_size:
continue
if size == None:
hdr = data[:hdr_size]
data = data[hdr_size:]
size, tp, digest = struct.unpack(PKT_HDR_FMT, hdr)
tp = 'err' if tp == TYPE_ERR else 'out'
if tp != btp:
print('ERR type')
return None
if len(data) < size:
continue
payload = data[:size]
data = data[size:]
if size != len(payload):
print('ERR size', size, len(payload))
return None
if digest != hashlib.sha256(payload).digest():
print('ERR digest', digest, hashlib.sha256(payload).digest())
for i in range(size):
if payload[i] != spkt[i + hdr_size]:
print('%d: %d != %d\n', i, payload[i], spkt[i + hdr_size])
return None
return payload
def run_command(vm_name, path):
print('run command...')
cmd = ['virsh',
'qemu-agent-command',
vm_name,
'{"execute":"guest-exec", "arguments":{"path": "%s", "arg": ["srv"], "capture-output": "interactive"}}' % path]
p = subprocess.run(cmd, stdout=subprocess.PIPE)
response = p.stdout.decode('utf-8'
[PATCH] qga: Add an interactive mode to guest-exec via VSOCK for Linux
Add an interactive mode to the guest-exec command in the QEMU Guest Agent
using the VSOCK communication mechanism. It enables interactive sessions
with the executed command in the guest, allowing real-time input/output.
Introduce "interactive" mode in the GuestExecCaptureOutputMode enumeration
and add optional "cid" and "port" fields to the guest-exec response. In
such a way user can execute guest-exec command, get CID and port number
from the response and connect to the guest server. After connection user
can communicate with the started process. All the data transmitted to the
server is redirected to stdin. Data from stdout and stderr is redirected
to the client. All data blocks are preceded by 32-bit headers (network
byte order): most significant bit contains a sign of stream (stdout - 0,
stderr - 1), all the other bits contain the payload size.
Signed-off-by: Alexander Ivanov
---
qga/commands.c | 272 +--
qga/qapi-schema.json | 11 +-
2 files changed, 273 insertions(+), 10 deletions(-)
diff --git a/qga/commands.c b/qga/commands.c
index 88c1c99fe5..377a79c816 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -20,6 +20,11 @@
#include "qemu/cutils.h"
#include "commands-common.h"
+#ifdef CONFIG_LINUX
+#include
+#include
+#endif
+
/* Maximum captured guest-exec out_data/err_data - 16MB */
#define GUEST_EXEC_MAX_OUTPUT (16 * 1024 * 1024)
/* Allocation and I/O buffer for reading guest-exec out_data/err_data - 4KB */
@@ -92,6 +97,27 @@ struct GuestExecIOData {
};
typedef struct GuestExecIOData GuestExecIOData;
+#define GE_INT_IO_SIZE (256 * 1024)
+#define GE_INT_STREAM_MASK 0x8000
+
+struct GEIntPacket {
+uint32_t header;
+gchar buf[GE_INT_IO_SIZE];
+} __attribute__((aligned(1)));
+typedef struct GEIntPacket GEIntPacket;
+
+struct GEIntData {
+unsigned int cid;
+unsigned int port;
+GIOChannel *ch_srv;
+GIOChannel *ch_clt;
+GIOChannel *ch_in;
+GIOChannel *ch_out;
+GIOChannel *ch_err;
+GEIntPacket packet;
+};
+typedef struct GEIntData GEIntData;
+
struct GuestExecInfo {
GPid pid;
int64_t pid_numeric;
@@ -101,6 +127,7 @@ struct GuestExecInfo {
GuestExecIOData in;
GuestExecIOData out;
GuestExecIOData err;
+GEIntData *int_data;
QTAILQ_ENTRY(GuestExecInfo) next;
};
typedef struct GuestExecInfo GuestExecInfo;
@@ -257,6 +284,194 @@ static char **guest_exec_get_args(const strList *entry,
bool log)
return args;
}
+#ifdef CONFIG_LINUX
+static void guest_exec_close_channel(GIOChannel *ch)
+{
+g_io_channel_shutdown(ch, true, NULL);
+g_io_channel_unref(ch);
+}
+
+static void guest_exec_interactive_cleanup(GuestExecInfo *gei)
+{
+GEIntData *data = gei->int_data;
+
+guest_exec_close_channel(data->ch_clt);
+guest_exec_close_channel(data->ch_srv);
+guest_exec_close_channel(data->ch_in);
+guest_exec_close_channel(data->ch_out);
+guest_exec_close_channel(data->ch_err);
+
+g_free(data);
+gei->int_data = NULL;
+}
+
+static gboolean guest_exec_interactive_watch(GIOChannel *ch, GIOCondition cond,
+ gpointer data_)
+{
+GuestExecInfo *gei = (GuestExecInfo *)data_;
+GEIntData *data = gei->int_data;
+gsize size, bytes_written;
+GIOStatus gstatus;
+GError *gerr = NULL;
+GIOChannel *dst_ch;
+gchar *p;
+
+if (data == NULL) {
+return false;
+}
+
+if (cond == G_IO_HUP || cond == G_IO_ERR) {
+goto close;
+}
+
+gstatus = g_io_channel_read_chars(ch, data->packet.buf,
+ sizeof(data->packet.buf), , NULL);
+
+if (gstatus == G_IO_STATUS_EOF || gstatus == G_IO_STATUS_ERROR) {
+if (gerr) {
+g_warning("qga: i/o error reading from a channel: %s",
+ gerr->message);
+g_error_free(gerr);
+}
+goto close;
+}
+
+if (ch == data->ch_clt) {
+dst_ch = data->ch_in;
+p = data->packet.buf;
+} else {
+assert(size < GE_INT_STREAM_MASK);
+
+dst_ch = data->ch_clt;
+p = (gchar *)&(data->packet);
+data->packet.header = htonl(size);
+if (ch == data->ch_err) {
+data->packet.header |= htonl(GE_INT_STREAM_MASK);
+}
+size += sizeof(data->packet.header);
+}
+
+do {
+gstatus = g_io_channel_write_chars(dst_ch, p, size,
+ _written, );
+
+if (gstatus == G_IO_STATUS_EOF || gstatus == G_IO_STATUS_ERROR) {
+if (gerr) {
+g_warning("qga: i/o error writing to a channel: %s",
+ gerr->message);
+g_error_free(gerr);
+}
+goto close;
+}
+size -= bytes_written;
+p += byt
Re: [PATCH v2 11/11] iotests/271: add test cases for subcluster-based discard/unmap
ff L2 entry #20: 0x @@ -351,6 +363,8 @@ L2 entry #24: 0x8009 L2 entry #25: 0x800e L2 entry #26: 0x800f write -q -z -u 1759k 128k +file_do_fallocate fd=N mode=0x03 offset=819200 len=32768 +file_do_fallocate fd=N mode=0x03 offset=1245184 len=65536 L2 entry #27: 0x800c L2 entry #28: 0x L2 entry #29: 0x8010 7fff8000 @@ -369,15 +383,20 @@ L2 entry #14: 0x800a L2 entry #15: 0x800b L2 entry #16: 0x800c L2 entry #17: 0x800d +write -q -z -u 512k 8k +L2 entry #8: 0x 000f write -q -z -u 576k 192k +file_do_fallocate fd=N mode=0x03 offset=327680 len=196608 L2 entry #9: 0x L2 entry #10: 0x L2 entry #11: 0x write -q -z -u 800k 128k +file_do_fallocate fd=N mode=0x03 offset=557056 len=131072 L2 entry #12: 0x8008 L2 entry #13: 0x L2 entry #14: 0x800a write -q -z -u 991k 128k +file_do_fallocate fd=N mode=0x03 offset=753664 len=129024 L2 entry #15: 0x800b L2 entry #16: 0x L2 entry #17: 0x800d 7fff8000 @@ -407,6 +426,7 @@ L2 entry #27: 0x4012 write -q -c -P PATTERN 1792k 64k L2 entry #28: 0x4013 write -q -z -u 1152k 192k +file_do_fallocate fd=N mode=0x03 offset=327680 len=196608 L2 entry #18: 0x L2 entry #19: 0x L2 entry #20: 0x @@ -419,28 +439,69 @@ L2 entry #24: 0x8009 L2 entry #25: 0x800e L2 entry #26: 0x800f write -q -z -u 1759k 128k +file_do_fallocate fd=N mode=0x03 offset=819200 len=32768 +file_do_fallocate fd=N mode=0x03 offset=1245184 len=65536 L2 entry #27: 0x800c L2 entry #28: 0x L2 entry #29: 0x ### Discarding clusters with non-zero bitmaps (backing file: yes) ### +Formatting 'TEST_DIR/t.IMGFMT.raw', fmt=raw size=1048576 +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=raw size=1048576 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=raw +write -q -P PATTERN 0 32k +L2 entry #0: 0x8005 +discard -q 32k 32k +file_do_fallocate fd=N mode=0x03 offset=360448 len=32768 +L2 entry #0: 0x8005 +write -q -P PATTERN 0 64k +L2 entry #0: 0x8005 +discard -q 0 8k +file_do_fallocate fd=N mode=0x03 offset=327680 len=8192 +L2 entry #0: 0x8005 0000 +discard -q 8k 56k +file_do_fallocate fd=N mode=0x03 offset=327680 len=65536 +L2 entry #0: 0x +write -q -P PATTERN 0 128k +L2 entry #0: 0x8005 +discard -q 0 128k +file_do_fallocate fd=N mode=0x03 offset=327680 len=131072 L2 entry #0: 0x L2 entry #1: 0x +discard -q 0 64k +L2 entry #0: 0x Image resized. Image resized. -L2 entry #0: 0x L2 entry #1: 0x ### Discarding clusters with non-zero bitmaps (backing file: no) ### +Formatting 'TEST_DIR/t.IMGFMT.raw', fmt=raw size=1048576 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 +write -q -P PATTERN 0 32k +L2 entry #0: 0x8005 +discard -q 32k 32k +file_do_fallocate fd=N mode=0x03 offset=360448 len=32768 +L2 entry #0: 0x8005 +write -q -P PATTERN 0 64k +L2 entry #0: 0x8005 +discard -q 0 8k +file_do_fallocate fd=N mode=0x03 offset=327680 len=8192 +L2 entry #0: 0x8005 0000 +discard -q 8k 56k +file_do_fallocate fd=N mode=0x03 offset=327680 len=65536 +L2 entry #0: 0x +write -q -P PATTERN 0 128k +L2 entry #0: 0x8005 +discard -q 0 128k +file_do_fallocate fd=N mode=0x03 offset=327680 len=131072 L2 entry #0: 0x L2 entry #1: 0x +discard -q 0 64k +L2 entry #0: 0x Image resized. Image resized. -L2 entry #0: 0x L2 entry #1: 0x ### Corrupted L2 entries - read test (allocated) ### Reviewed-by: Alexander Ivanov -- Best regards, Alexander Ivanov
Re: [PATCH v2 10/11] qcow2: zero_l2_subclusters: fall through to discard operation when requested
On 5/13/24 08:32, Andrey Drobyshev wrote:
When zeroizing subclusters within single cluster, detect usage of the
BDRV_REQ_MAY_UNMAP flag and fall through to the subcluster-based discard
operation, much like it's done with the cluster-based discards. That
way subcluster-aligned operations "qemu-io -c 'write -z -u ...'" will
lead to actual unmap.
Signed-off-by: Andrey Drobyshev
---
block/qcow2-cluster.c | 26 --
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3c134a7e80..53e04eff93 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2107,15 +2107,16 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t
offset, uint64_t nb_clusters,
static int coroutine_fn GRAPH_RDLOCK
discard_l2_subclusters(BlockDriverState *bs, uint64_t offset,
- uint64_t nb_subclusters,
- enum qcow2_discard_type type,
- bool full_discard)
+ uint64_t nb_subclusters, enum qcow2_discard_type type,
+ bool full_discard, bool uncond_zeroize)
{
BDRVQcow2State *s = bs->opaque;
uint64_t new_l2_bitmap, bitmap_alloc_mask, bitmap_zero_mask;
int ret, sc = offset_to_sc_index(s, offset);
g_auto(SubClusterRangeInfo) scri = { 0 };
+assert(!(full_discard && uncond_zeroize));
+
ret = get_sc_range_info(bs, offset, nb_subclusters, );
if (ret < 0) {
return ret;
@@ -2140,7 +2141,8 @@ discard_l2_subclusters(BlockDriverState *bs, uint64_t
offset,
*/
if (full_discard) {
new_l2_bitmap &= ~bitmap_zero_mask;
-} else if (bs->backing || scri.l2_bitmap & bitmap_alloc_mask) {
+} else if (uncond_zeroize || bs->backing ||
+ scri.l2_bitmap & bitmap_alloc_mask) {
new_l2_bitmap |= bitmap_zero_mask;
}
@@ -2197,7 +2199,7 @@ int qcow2_subcluster_discard(BlockDriverState *bs, uint64_t offset,
if (head) {
ret = discard_l2_subclusters(bs, offset - head,
size_to_subclusters(s, head), type,
- full_discard);
+ full_discard, false);
if (ret < 0) {
goto fail;
}
@@ -2221,7 +2223,7 @@ int qcow2_subcluster_discard(BlockDriverState *bs,
uint64_t offset,
if (tail) {
ret = discard_l2_subclusters(bs, end_offset,
size_to_subclusters(s, tail), type,
- full_discard);
+ full_discard, false);
if (ret < 0) {
goto fail;
}
@@ -2318,6 +2320,18 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t
offset,
int ret, sc = offset_to_sc_index(s, offset);
g_auto(SubClusterRangeInfo) scri = { 0 };
+/*
+ * If the request allows discarding subclusters we go down the discard
+ * path regardless of their allocation status. After the discard
+ * operation with full_discard=false subclusters are going to be read as
+ * zeroes anyway. But we make sure that subclusters are explicitly
+ * zeroed anyway with uncond_zeroize=true.
+ */
+if (flags & BDRV_REQ_MAY_UNMAP) {
+return discard_l2_subclusters(bs, offset, nb_subclusters,
+ QCOW2_DISCARD_REQUEST, false, true);
+}
+
ret = get_sc_range_info(bs, offset, nb_subclusters, );
if (ret < 0) {
return ret;
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 09/11] qcow2: make subclusters discardable
cropped
clusters");
goto fail;
@@ -5032,8 +5032,9 @@ static int GRAPH_RDLOCK qcow2_make_empty(BlockDriverState
*bs)
* default action for this kind of discard is to pass the discard,
* which will ideally result in an actually smaller image file, as
* is probably desired. */
-ret = qcow2_cluster_discard(bs, offset, MIN(step, end_offset - offset),
-QCOW2_DISCARD_SNAPSHOT, true);
+ret = qcow2_subcluster_discard(bs, offset,
+ MIN(step, end_offset - offset),
+ QCOW2_DISCARD_SNAPSHOT, true);
if (ret < 0) {
break;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index 197bdcdf53..a65c185b51 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -953,8 +953,8 @@ void coroutine_fn GRAPH_RDLOCK
qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m);
int GRAPH_RDLOCK
-qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
- enum qcow2_discard_type type, bool full_discard);
+qcow2_subcluster_discard(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+ enum qcow2_discard_type type, bool full_discard);
int coroutine_fn GRAPH_RDLOCK
qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset, uint64_t
bytes,
diff --git a/tests/qemu-iotests/271 b/tests/qemu-iotests/271
index 59a6fafa2f..04c57813c2 100755
--- a/tests/qemu-iotests/271
+++ b/tests/qemu-iotests/271
@@ -518,7 +518,7 @@ done
-# Test qcow2_cluster_discard() with full and normal discards
+# Test qcow2_subcluster_discard() with full and normal discards
for use_backing_file in yes no; do
echo
echo "### Discarding clusters with non-zero bitmaps (backing file:
$use_backing_file) ###"
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 08/11] qcow2: zeroize the entire cluster when there're no non-zero subclusters
On 5/13/24 08:32, Andrey Drobyshev wrote:
When zeroizing the last non-zero subclusters within single cluster, it
makes sense to go zeroize the entire cluster and go down zero_in_l2_slice()
path right away. That way we'd also update the corresponding refcount
table.
Signed-off-by: Andrey Drobyshev
Reviewed-by: Hanna Czenczek
---
block/qcow2-cluster.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 475f167035..8d39e2f960 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -2221,7 +2221,7 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
static int coroutine_fn GRAPH_RDLOCK
zero_l2_subclusters(BlockDriverState *bs, uint64_t offset,
-unsigned nb_subclusters)
+unsigned nb_subclusters, int flags)
{
BDRVQcow2State *s = bs->opaque;
uint64_t new_l2_bitmap;
@@ -2237,6 +2237,16 @@ zero_l2_subclusters(BlockDriverState *bs, uint64_t
offset,
new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+/*
+ * If there're no non-zero subclusters left, we might as well zeroize
+ * the entire cluster. That way we'd also update the refcount table.
+ */
+if ((new_l2_bitmap & QCOW_L2_BITMAP_ALL_ZEROES) ==
+QCOW_L2_BITMAP_ALL_ZEROES) {
+return zero_in_l2_slice(bs, QEMU_ALIGN_DOWN(offset, s->cluster_size),
+1, flags);
+}
+
if (new_l2_bitmap != scri.l2_bitmap) {
set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap);
qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
@@ -2293,7 +2303,7 @@ int coroutine_fn
qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
if (head) {
ret = zero_l2_subclusters(bs, offset - head,
- size_to_subclusters(s, head));
+ size_to_subclusters(s, head), flags);
if (ret < 0) {
goto fail;
}
@@ -2314,7 +2324,8 @@ int coroutine_fn
qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
}
if (tail) {
-ret = zero_l2_subclusters(bs, end_offset, size_to_subclusters(s,
tail));
+ret = zero_l2_subclusters(bs, end_offset,
+ size_to_subclusters(s, tail), flags);
if (ret < 0) {
goto fail;
}
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 07/11] qcow2: add get_sc_range_info() helper for working with subcluster ranges
ssert(offset_into_subcluster(s, offset) == 0);
+uint64_t new_l2_bitmap;
+int ret, sc = offset_to_sc_index(s, offset);
+g_auto(SubClusterRangeInfo) scri = { 0 };
-ret = get_cluster_table(bs, offset, _slice, _index);
+ret = get_sc_range_info(bs, offset, nb_subclusters, );
if (ret < 0) {
return ret;
}
-switch (qcow2_get_cluster_type(bs, get_l2_entry(s, l2_slice, l2_index))) {
-case QCOW2_CLUSTER_COMPRESSED:
-ret = -ENOTSUP; /* We cannot partially zeroize compressed clusters */
-goto out;
-case QCOW2_CLUSTER_NORMAL:
-case QCOW2_CLUSTER_UNALLOCATED:
-break;
-default:
-g_assert_not_reached();
-}
-
-old_l2_bitmap = l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index);
-
-l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
-l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
+new_l2_bitmap = scri.l2_bitmap;
+new_l2_bitmap |= QCOW_OFLAG_SUB_ZERO_RANGE(sc, sc + nb_subclusters);
+new_l2_bitmap &= ~QCOW_OFLAG_SUB_ALLOC_RANGE(sc, sc + nb_subclusters);
-if (old_l2_bitmap != l2_bitmap) {
-set_l2_bitmap(s, l2_slice, l2_index, l2_bitmap);
-qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
+if (new_l2_bitmap != scri.l2_bitmap) {
+set_l2_bitmap(s, scri.l2_slice, scri.l2_index, new_l2_bitmap);
+qcow2_cache_entry_mark_dirty(s->l2_table_cache, scri.l2_slice);
}
-ret = 0;
-out:
-qcow2_cache_put(s->l2_table_cache, (void **) _slice);
-
- return ret;
+return 0;
}
int coroutine_fn qcow2_subcluster_zeroize(BlockDriverState *bs, uint64_t offset,
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 06/11] iotests/290: add test case to check 'discard-no-unref' option behavior
On 5/13/24 08:31, Andrey Drobyshev wrote:
We basically fill 2 images with identical data and perform discard
operations with and without 'discard-no-unref' enabled. Then we check
that images still read identically, that their disk usage is the same
(i.e. fallocate(FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE) is called for
both) and that with the option enabled cluster is still marked as
allocated in "qemu-img map" output. We also check that the option
doesn't work with qcow2 v2 images.
Signed-off-by: Andrey Drobyshev
---
tests/qemu-iotests/290 | 34 ++
tests/qemu-iotests/290.out | 28
2 files changed, 62 insertions(+)
diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
index 776b59de1b..4eb929d15f 100755
--- a/tests/qemu-iotests/290
+++ b/tests/qemu-iotests/290
@@ -92,6 +92,40 @@ for qcow2_compat in 0.10 1.1; do
$QEMU_IMG map "$TEST_IMG" | _filter_testdir
done
+echo
+echo "### Test 'qemu-io -c discard' with 'discard-no-unref' option enabled"
+echo
+
+echo "# Check that qcow2 v2 images don't support 'discard-no-unref' option"
+NOUNREF_IMG="$TEST_IMG.nounref"
+TEST_IMG="$NOUNREF_IMG" _make_test_img -o "compat=0.10" 128k
+# This should immediately fail with an error
+$QEMU_IO -c 'reopen -o discard-no-unref=on' "$NOUNREF_IMG" | _filter_qemu_io
+
+echo "# Create two compat=1.1 images and fill them with identical data"
+_make_test_img -o "compat=1.1" 128k
+TEST_IMG="$NOUNREF_IMG" _make_test_img -o "compat=1.1" 128k
+$QEMU_IO -c 'write -P 0xaa 0 128k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c 'write -P 0xaa 0 128k' "$NOUNREF_IMG" | _filter_qemu_io
+
+echo "# Enable 'discard-no-unref' in one of them, discard 2nd cluster in both"
+$QEMU_IO -c 'discard 64k 64k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c 'reopen -o discard-no-unref=on' \
+ -c 'discard 64k 64k' "$NOUNREF_IMG" | _filter_qemu_io
+
+echo "# Compare disk usage of the 2 images"
+# Don't check the exact disk usage values but rather that they're equal
+echo "disk_usage(`basename $TEST_IMG`) - disk_usage(`basename $NOUNREF_IMG`)" \
+ "= $(( `disk_usage $TEST_IMG` - `disk_usage $NOUNREF_IMG`))"
+
+echo "# Check that images are still identical"
+$QEMU_IMG compare "$TEST_IMG" "$NOUNREF_IMG"
+
+echo "# Output of qemu-img map for the image with dropped reference"
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+echo "# Output of qemu-img map for the image with kept reference"
+$QEMU_IMG map --output=json "$NOUNREF_IMG" | _filter_qemu_img_map
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/290.out b/tests/qemu-iotests/290.out
index 22b476594f..f790feae81 100644
--- a/tests/qemu-iotests/290.out
+++ b/tests/qemu-iotests/290.out
@@ -58,4 +58,32 @@ read 131072/131072 bytes at offset 0
128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
# Output of qemu-img map
Offset Length Mapped to File
+
+### Test 'qemu-io -c discard' with 'discard-no-unref' option enabled
+
+# Check that qcow2 v2 images don't support 'discard-no-unref' option
+Formatting 'TEST_DIR/t.IMGFMT.nounref', fmt=IMGFMT size=131072
+qemu-io: discard-no-unref is only supported since qcow2 version 3
+# Create two compat=1.1 images and fill them with identical data
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+Formatting 'TEST_DIR/t.IMGFMT.nounref', fmt=IMGFMT size=131072
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Enable 'discard-no-unref' in one of them, discard 2nd cluster in both
+discard 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Compare disk usage of the 2 images
+disk_usage(t.qcow2) - disk_usage(t.qcow2.nounref) = 0
+# Check that images are still identical
+Images are identical.
+# Output of qemu-img map for the image with dropped reference
+[{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true,
"compressed": false, "offset": OFFSET},
+{ "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data":
false, "compressed": false}]
+# Output of qemu-img map for the image with kept reference
+[{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true,
"compressed": false, "offset": OFFSET},
+{ "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false,
"compressed": false, "offset": OFFSET}]
*** done
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 05/11] iotests/common.rc: add disk_usage function
On 5/13/24 08:31, Andrey Drobyshev wrote:
Move the definition from iotests/250 to common.rc. This is used to
detect real disk usage of sparse files. In particular, we want to use
it for checking subclusters-based discards.
Signed-off-by: Andrey Drobyshev
---
tests/qemu-iotests/250 | 5 -
tests/qemu-iotests/common.rc | 6 ++
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/tests/qemu-iotests/250 b/tests/qemu-iotests/250
index af48f83aba..c0a0dbc0ff 100755
--- a/tests/qemu-iotests/250
+++ b/tests/qemu-iotests/250
@@ -52,11 +52,6 @@ _unsupported_imgopts data_file
# bdrv_co_truncate(bs->file) call in qcow2_co_truncate(), which might succeed
# anyway.
-disk_usage()
-{
-du --block-size=1 $1 | awk '{print $1}'
-}
-
size=2100M
_make_test_img -o "cluster_size=1M,preallocation=metadata" $size
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 95c12577dd..237f746af8 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -140,6 +140,12 @@ _optstr_add()
fi
}
+# report real disk usage for sparse files
+disk_usage()
+{
+du --block-size=1 "$1" | awk '{print $1}'
+}
+
# Set the variables to the empty string to turn Valgrind off
# for specific processes, e.g.
# $ VALGRIND_QEMU_IO= ./check -qcow2 -valgrind 015
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 04/11] block/file-posix: add trace event for fallocate() calls
On 5/13/24 08:31, Andrey Drobyshev wrote:
This would ease debugging of write zeroes and discard operations.
Signed-off-by: Andrey Drobyshev
---
block/file-posix.c | 1 +
block/trace-events | 1 +
2 files changed, 2 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..45134f0eef 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1859,6 +1859,7 @@ static int translate_err(int err)
static int do_fallocate(int fd, int mode, off_t offset, off_t len)
{
do {
+trace_file_do_fallocate(fd, mode, offset, len);
if (fallocate(fd, mode, offset, len) == 0) {
return 0;
}
diff --git a/block/trace-events b/block/trace-events
index 8e789e1f12..2f7ad28996 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -203,6 +203,7 @@ curl_setup_preadv(uint64_t bytes, uint64_t start, const char *range)
"reading %"
curl_close(void) "close"
# file-posix.c
+file_do_fallocate(int fd, int mode, int64_t offset, int64_t len) "fd=%d mode=0x%02x
offset=%" PRIi64 " len=%" PRIi64
file_copy_file_range(void *bs, int src, int64_t src_off, int dst, int64_t dst_off, int64_t bytes, int flags, int64_t
ret) "bs %p src_fd %d offset %"PRIu64" dst_fd %d offset %"PRIu64" bytes %"PRIu64"
flags %d ret %"PRId64
file_FindEjectableOpticalMedia(const char *media) "Matching using %s"
file_setup_cdrom(const char *partition) "Using %s as optical disc"
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 03/11] qcow2: put discard requests in the common queue when discard-no-unref enabled
On 5/13/24 08:31, Andrey Drobyshev wrote:
Normally discard requests are stored in the queue attached to BDRVQcow2State
to be processed later at once. Currently discard-no-unref option handling
causes these requests to be processed straight away. Let's fix that.
Note that when doing regular discards qcow2_free_any_cluster() would check
for the presence of external data files for us and redirect request to
underlying data_file. Here we want to do the same but avoid refcount updates,
thus we perform the same checks.
Suggested-by: Hanna Czenczek
Signed-off-by: Andrey Drobyshev
---
block/qcow2-cluster.c | 39 +--
1 file changed, 29 insertions(+), 10 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 5f057ba2fd..7dff0bd5a1 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1893,6 +1893,28 @@ again:
return 0;
}
+/*
+ * Helper for adding a discard request to the queue without any refcount
+ * modifications. If external data file is used redirects the request to
+ * the corresponding BdrvChild.
+ */
+static inline void
+discard_no_unref_any_file(BlockDriverState *bs, uint64_t offset,
+ uint64_t length, QCow2ClusterType ctype,
+ enum qcow2_discard_type dtype)
+{
+BDRVQcow2State *s = bs->opaque;
+
+if (s->discard_passthrough[dtype] &&
+(ctype == QCOW2_CLUSTER_NORMAL || ctype == QCOW2_CLUSTER_ZERO_ALLOC)) {
+if (has_data_file(bs)) {
+bdrv_pdiscard(s->data_file, offset, length);
+} else {
+qcow2_queue_discard(bs, offset, length);
+}
+}
+}
+
/*
* This discards as many clusters of nb_clusters as possible at once (i.e.
* all clusters in the same L2 slice) and returns the number of discarded
@@ -1974,12 +1996,10 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t
offset, uint64_t nb_clusters,
if (!keep_reference) {
/* Then decrease the refcount */
qcow2_free_any_cluster(bs, old_l2_entry, type);
-} else if (s->discard_passthrough[type] &&
- (cluster_type == QCOW2_CLUSTER_NORMAL ||
-cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) {
+} else {
/* If we keep the reference, pass on the discard still */
-bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
- s->cluster_size);
+discard_no_unref_any_file(bs, old_l2_entry & L2E_OFFSET_MASK,
+ s->cluster_size, cluster_type, type);
}
}
@@ -2088,12 +2108,11 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
if (!keep_reference) {
/* Then decrease the refcount */
qcow2_free_any_cluster(bs, old_l2_entry,
QCOW2_DISCARD_REQUEST);
-} else if (s->discard_passthrough[QCOW2_DISCARD_REQUEST] &&
- (type == QCOW2_CLUSTER_NORMAL ||
-type == QCOW2_CLUSTER_ZERO_ALLOC)) {
+} else {
/* If we keep the reference, pass on the discard still */
-bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
-s->cluster_size);
+discard_no_unref_any_file(bs, old_l2_entry & L2E_OFFSET_MASK,
+ s->cluster_size, type,
+ QCOW2_DISCARD_REQUEST);
}
}
}
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 02/11] qcow2: simplify L2 entries accounting for discard-no-unref
On 5/13/24 08:31, Andrey Drobyshev wrote:
Commits 42a2890a and b2b10904 introduce handling of discard-no-unref
option in discard_in_l2_slice() and zero_in_l2_slice(). They add even
more if's when chosing the right l2 entry. What we really need for this
option is the new entry simply to contain the same host cluster offset,
no matter whether we unmap or zeroize the cluster. For that OR'ing with
the old entry is enough.
This patch doesn't change the logic and is pure refactoring.
Signed-off-by: Andrey Drobyshev
---
block/qcow2-cluster.c | 34 +++---
1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ce8c0076b3..5f057ba2fd 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1946,25 +1946,21 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t
offset, uint64_t nb_clusters,
new_l2_entry = new_l2_bitmap = 0;
} else if (bs->backing || qcow2_cluster_is_allocated(cluster_type)) {
if (has_subclusters(s)) {
-if (keep_reference) {
-new_l2_entry = old_l2_entry;
-} else {
-new_l2_entry = 0;
-}
+new_l2_entry = 0;
new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
} else {
-if (s->qcow_version >= 3) {
-if (keep_reference) {
-new_l2_entry |= QCOW_OFLAG_ZERO;
-} else {
-new_l2_entry = QCOW_OFLAG_ZERO;
-}
-} else {
-new_l2_entry = 0;
-}
+new_l2_entry = s->qcow_version >= 3 ? QCOW_OFLAG_ZERO : 0;
}
}
+/*
+ * No need to check for the QCOW version since discard-no-unref is
+ * only allowed since version 3.
+ */
+if (keep_reference) {
+new_l2_entry |= old_l2_entry;
+}
+
if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
continue;
}
@@ -2064,19 +2060,19 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
((flags & BDRV_REQ_MAY_UNMAP) &&
qcow2_cluster_is_allocated(type));
bool keep_reference =
(s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED);
-uint64_t new_l2_entry = old_l2_entry;
+uint64_t new_l2_entry = unmap ? 0 : old_l2_entry;
uint64_t new_l2_bitmap = old_l2_bitmap;
-if (unmap && !keep_reference) {
-new_l2_entry = 0;
-}
-
if (has_subclusters(s)) {
new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
} else {
new_l2_entry |= QCOW_OFLAG_ZERO;
}
+if (keep_reference) {
+new_l2_entry |= old_l2_entry;
+}
+
if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
continue;
}
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v2 01/11] qcow2: make function update_refcount_discard() global
On 5/13/24 08:31, Andrey Drobyshev wrote:
We are going to need it for discarding separate subclusters. The
function itself doesn't do anything with the refcount tables, it simply
adds a discard request to the queue, so rename it to qcow2_queue_discard().
Signed-off-by: Andrey Drobyshev
Reviewed-by: Hanna Czenczek
---
block/qcow2-refcount.c | 8
block/qcow2.h | 2 ++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 0266542cee..2026f5fa21 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -754,8 +754,8 @@ void qcow2_process_discards(BlockDriverState *bs, int ret)
}
}
-static void update_refcount_discard(BlockDriverState *bs,
-uint64_t offset, uint64_t length)
+void qcow2_queue_discard(BlockDriverState *bs, uint64_t offset,
+ uint64_t length)
{
BDRVQcow2State *s = bs->opaque;
Qcow2DiscardRegion *d, *p, *next;
@@ -902,7 +902,7 @@ update_refcount(BlockDriverState *bs, int64_t offset,
int64_t length,
}
if (s->discard_passthrough[type]) {
-update_refcount_discard(bs, cluster_offset, s->cluster_size);
+qcow2_queue_discard(bs, cluster_offset, s->cluster_size);
}
}
}
@@ -3619,7 +3619,7 @@ qcow2_discard_refcount_block(BlockDriverState *bs,
uint64_t discard_block_offs)
/* discard refblock from the cache if refblock is cached */
qcow2_cache_discard(s->refcount_block_cache, refblock);
}
-update_refcount_discard(bs, discard_block_offs, s->cluster_size);
+qcow2_queue_discard(bs, discard_block_offs, s->cluster_size);
return 0;
}
diff --git a/block/qcow2.h b/block/qcow2.h
index a9e3481c6e..197bdcdf53 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -891,6 +891,8 @@ int coroutine_fn qcow2_check_refcounts(BlockDriverState
*bs, BdrvCheckResult *re
BdrvCheckMode fix);
void GRAPH_RDLOCK qcow2_process_discards(BlockDriverState *bs, int ret);
+void qcow2_queue_discard(BlockDriverState *bs, uint64_t offset,
+ uint64_t length);
int GRAPH_RDLOCK
qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
Reviewed-by: Alexander Ivanov
--
Best regards,
Alexander Ivanov
Re: [PATCH v3] block: Use LVM tools for LV block device truncation
ping
Is there any update of the patch status?
Thank you.
On 3/15/24 09:58, Alexander Ivanov wrote:
If a block device is an LVM logical volume we can resize it using
standard LVM tools.
Add a helper to detect if a device is a DM device. In raw_co_truncate()
check if the block device is DM and resize it executing lvresize.
Signed-off-by: Alexander Ivanov
---
block/file-posix.c | 61 ++
1 file changed, 61 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..af17a43fe9 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd,
int64_t offset,
return raw_thread_pool_submit(handle_aiocb_truncate, );
}
+static bool device_is_dm(struct stat *st)
+{
+unsigned int maj, maj2;
+char line[32], devname[16];
+bool ret = false;
+FILE *f;
+
+if (!S_ISBLK(st->st_mode)) {
+return false;
+}
+
+f = fopen("/proc/devices", "r");
+if (!f) {
+return false;
+}
+
+maj = major(st->st_rdev);
+
+while (fgets(line, sizeof(line), f)) {
+if (sscanf(line, "%u %15s", , devname) != 2) {
+continue;
+}
+if (strcmp(devname, "device-mapper") == 0) {
+ret = (maj == maj2);
+break;
+}
+}
+
+fclose(f);
+return ret;
+}
+
static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
bool exact, PreallocMode prealloc,
BdrvRequestFlags flags, Error **errp)
@@ -2670,6 +2702,35 @@ static int coroutine_fn raw_co_truncate(BlockDriverState
*bs, int64_t offset,
if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
int64_t cur_length = raw_getlength(bs);
+/*
+ * Try to resize an LVM device using LVM tools.
+ */
+if (device_is_dm() && offset > 0) {
+int spawn_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL;
+int status;
+bool success;
+char *err;
+GError *gerr = NULL, *gerr_exit = NULL;
+g_autofree char *size_str = g_strdup_printf("%" PRId64 "B",
offset);
+const char *cmd[] = {"lvresize", "-f", "-L",
+ size_str, bs->filename, NULL};
+
+success = g_spawn_sync(NULL, (gchar **)cmd, NULL, spawn_flags,
+ NULL, NULL, NULL, , , );
+
+if (success && g_spawn_check_exit_status(status, _exit)) {
+return 0;
+}
+
+if (success) {
+error_setg(errp, "%s: %s", gerr_exit->message, err);
+} else {
+error_setg(errp, "lvresize execution error: %s",
gerr->message);
+}
+
+return -EINVAL;
+}
+
if (offset != cur_length && exact) {
error_setg(errp, "Cannot resize device files");
return -ENOTSUP;
--
Best regards,
Alexander Ivanov
[PATCH v5] blockcommit: Reopen base image as RO after abort
If a blockcommit is aborted the base image remains in RW mode, that leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk inside the guest ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda --abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos:0
flags: 02140002 < The last 2 means RW mode
If the base image is in RW mode at the end of blockcommit and was in RO
mode before blockcommit, reopen the base BDS in RO.
Signed-off-by: Alexander Ivanov
Reviewed-by: Vladimir Sementsov-Ogievskiy
---
block/mirror.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 1bdce3b657..61f0a717b7 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -93,6 +93,7 @@ typedef struct MirrorBlockJob {
int64_t active_write_bytes_in_flight;
bool prepared;
bool in_drain;
+bool base_ro;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -794,6 +795,10 @@ static int mirror_exit_common(Job *job)
bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, _abort);
bdrv_graph_wrunlock();
+if (abort && s->base_ro && !bdrv_is_read_only(target_bs)) {
+bdrv_reopen_set_read_only(target_bs, true, NULL);
+}
+
bdrv_drained_end(target_bs);
bdrv_unref(target_bs);
@@ -1717,6 +1722,7 @@ static BlockJob *mirror_start_job(
bool is_none_mode, BlockDriverState *base,
bool auto_complete, const char *filter_node_name,
bool is_mirror, MirrorCopyMode copy_mode,
+ bool base_ro,
Error **errp)
{
MirrorBlockJob *s;
@@ -1800,6 +1806,7 @@ static BlockJob *mirror_start_job(
bdrv_unref(mirror_top_bs);
s->mirror_top_bs = mirror_top_bs;
+s->base_ro = base_ro;
/* No resize for the target either; while the mirror is still running, a
* consistent read isn't necessarily possible. We could possibly allow
@@ -2029,7 +2036,7 @@ void mirror_start(const char *job_id, BlockDriverState
*bs,
speed, granularity, buf_size, backing_mode, zero_target,
on_source_error, on_target_error, unmap, NULL, NULL,
_job_driver, is_none_mode, base, false,
- filter_node_name, true, copy_mode, errp);
+ filter_node_name, true, copy_mode, false, errp);
}
BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -2058,7 +2065,7 @@ BlockJob *commit_active_start(const char *job_id,
BlockDriverState *bs,
on_error, on_error, true, cb, opaque,
_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- errp);
+ base_read_only, errp);
if (!job) {
goto error_restore_flags;
}
--
2.40.1
[PATCH v4] blockcommit: Reopen base image as RO after abort
If a blockcommit is aborted the base image remains in RW mode, that leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk inside the guest ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda --abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos:0
flags: 02140002 < The last 2 means RW mode
If the base image is in RW mode at the end of blockcommit and was in RO
mode before blockcommit, reopen the base BDS in RO.
Signed-off-by: Alexander Ivanov
---
block/mirror.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 1bdce3b657..d23be57255 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -93,6 +93,7 @@ typedef struct MirrorBlockJob {
int64_t active_write_bytes_in_flight;
bool prepared;
bool in_drain;
+bool base_ro;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -797,6 +798,10 @@ static int mirror_exit_common(Job *job)
bdrv_drained_end(target_bs);
bdrv_unref(target_bs);
+if (abort && s->base_ro && !bdrv_is_read_only(target_bs)) {
+bdrv_reopen_set_read_only(target_bs, true, NULL);
+}
+
bs_opaque->job = NULL;
bdrv_drained_end(src);
@@ -1717,6 +1722,7 @@ static BlockJob *mirror_start_job(
bool is_none_mode, BlockDriverState *base,
bool auto_complete, const char *filter_node_name,
bool is_mirror, MirrorCopyMode copy_mode,
+ bool base_ro,
Error **errp)
{
MirrorBlockJob *s;
@@ -1800,6 +1806,7 @@ static BlockJob *mirror_start_job(
bdrv_unref(mirror_top_bs);
s->mirror_top_bs = mirror_top_bs;
+s->base_ro = base_ro;
/* No resize for the target either; while the mirror is still running, a
* consistent read isn't necessarily possible. We could possibly allow
@@ -2029,7 +2036,7 @@ void mirror_start(const char *job_id, BlockDriverState
*bs,
speed, granularity, buf_size, backing_mode, zero_target,
on_source_error, on_target_error, unmap, NULL, NULL,
_job_driver, is_none_mode, base, false,
- filter_node_name, true, copy_mode, errp);
+ filter_node_name, true, copy_mode, false, errp);
}
BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -2058,7 +2065,7 @@ BlockJob *commit_active_start(const char *job_id,
BlockDriverState *bs,
on_error, on_error, true, cb, opaque,
_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- errp);
+ base_read_only, errp);
if (!job) {
goto error_restore_flags;
}
--
2.40.1
Re: [PATCH v3] blockcommit: Reopen base image as RO after abort
On 2/28/24 17:48, Vladimir Sementsov-Ogievskiy wrote:
On 09.02.24 15:29, Alexander Ivanov wrote:
Could you please review the patch?
Sorry for long delay.
Honestly, I don't like refcnt in block-driver. It violate
incapsulation, refcnt is interal thing of common block layer. And
actually, you can't make any assumptions from value of refcnt, as you
don't know which additional parents were created and why, and when
they are going unref their children.
Hmmm... Maybe I can just exclude refcnt check from the condition, can't
I. If BDS will be removed it doesn't matter if we make it RO. What do
you think?
What was wrong with v2?
My bad, it seems, I didn't send v2 before I decided to change the patch.
On 1/30/24 10:14, Alexander Ivanov wrote:
If a blockcommit is aborted the base image remains in RW mode, that
leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk inside the guest ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob
vm vda --abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133
/vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos: 0
flags: 02140002 < The last 2 means RW mode
If the base image is in RW mode at the end of blockcommit and was in RO
mode before blockcommit, check if src BDS has refcnt > 1. If so, the
BDS
will not be removed after blockcommit, and we should make the base
image
RO. Otherwise check recursively if there is a parent BDS of src BDS and
reopen the base BDS in RO in this case.
Signed-off-by: Alexander Ivanov
---
block/mirror.c | 38 --
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 5145eb53e1..52a7fee75e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -93,6 +93,7 @@ typedef struct MirrorBlockJob {
int64_t active_write_bytes_in_flight;
bool prepared;
bool in_drain;
+ bool base_ro;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -652,6 +653,32 @@ static void coroutine_fn
mirror_wait_for_all_io(MirrorBlockJob *s)
}
}
+/*
+ * Check recursively if there is a parent BDS referenced more than
+ * min_refcnt times. This argument is needed because at the first
+ * call there is a bds referenced in blockcommit.
+ */
+static bool bdrv_chain_has_significant_parent(BlockDriverState *bs)
+{
+ BdrvChild *parent;
+ BlockDriverState *parent_bs;
+
+ QLIST_FOREACH(parent, >parents, next) {
+ if (!(parent->klass->parent_is_bds)) {
+ continue;
+ }
+ parent_bs = parent->opaque;
+ if (parent_bs->drv && !parent_bs->drv->is_filter) {
+ return true;
+ }
+ if (bdrv_chain_has_significant_parent(parent_bs)) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
/**
* mirror_exit_common: handle both abort() and prepare() cases.
* for .prepare, returns 0 on success and -errno on failure.
@@ -793,6 +820,11 @@ static int mirror_exit_common(Job *job)
bdrv_drained_end(target_bs);
bdrv_unref(target_bs);
+ if (s->base_ro && !bdrv_is_read_only(target_bs) &&
+ (src->refcnt > 1 || bdrv_chain_has_significant_parent(src))) {
+ bdrv_reopen_set_read_only(target_bs, true, NULL);
+ }
+
bs_opaque->job = NULL;
bdrv_drained_end(src);
@@ -1715,6 +1747,7 @@ static BlockJob *mirror_start_job(
bool is_none_mode, BlockDriverState
*base,
bool auto_complete, const char
*filter_node_name,
bool is_mirror, MirrorCopyMode
copy_mode,
+ bool base_ro,
Error **errp)
{
MirrorBlockJob *s;
@@ -1798,6 +1831,7 @@ static BlockJob *mirror_start_job(
bdrv_unref(mirror_top_bs);
s->mirror_top_bs = mirror_top_bs;
+ s->base_ro = base_ro;
/* No resize for the target either; while the mirror is still
running, a
* consistent read isn't necessarily possible. We could
possibly allow
@@ -2027,7 +2061,7 @@ void mirror_start(const char *job_id,
BlockDriverState *bs,
speed, granularity, buf_size, backing_mode,
zero_target,
on_source_error, on_target_error, unmap,
NULL, NULL,
_job_driver, is_none_mode, base, false,
- filter_node_name, true, copy_mode, errp);
+ filter_node_name, true, copy_mode, false, errp);
}
BlockJob *commit_active_start(const char *job_id, BlockDriverState
*bs,
@@ -2056,7 +2090,7 @@ BlockJob *commit_active_start(const char
*job_id, BlockDriverState *bs,
on_error, on_error, true, cb, o
[PATCH v3] block: Use LVM tools for LV block device truncation
If a block device is an LVM logical volume we can resize it using
standard LVM tools.
Add a helper to detect if a device is a DM device. In raw_co_truncate()
check if the block device is DM and resize it executing lvresize.
Signed-off-by: Alexander Ivanov
---
block/file-posix.c | 61 ++
1 file changed, 61 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..af17a43fe9 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd,
int64_t offset,
return raw_thread_pool_submit(handle_aiocb_truncate, );
}
+static bool device_is_dm(struct stat *st)
+{
+unsigned int maj, maj2;
+char line[32], devname[16];
+bool ret = false;
+FILE *f;
+
+if (!S_ISBLK(st->st_mode)) {
+return false;
+}
+
+f = fopen("/proc/devices", "r");
+if (!f) {
+return false;
+}
+
+maj = major(st->st_rdev);
+
+while (fgets(line, sizeof(line), f)) {
+if (sscanf(line, "%u %15s", , devname) != 2) {
+continue;
+}
+if (strcmp(devname, "device-mapper") == 0) {
+ret = (maj == maj2);
+break;
+}
+}
+
+fclose(f);
+return ret;
+}
+
static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
bool exact, PreallocMode prealloc,
BdrvRequestFlags flags, Error **errp)
@@ -2670,6 +2702,35 @@ static int coroutine_fn raw_co_truncate(BlockDriverState
*bs, int64_t offset,
if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
int64_t cur_length = raw_getlength(bs);
+/*
+ * Try to resize an LVM device using LVM tools.
+ */
+if (device_is_dm() && offset > 0) {
+int spawn_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL;
+int status;
+bool success;
+char *err;
+GError *gerr = NULL, *gerr_exit = NULL;
+g_autofree char *size_str = g_strdup_printf("%" PRId64 "B",
offset);
+const char *cmd[] = {"lvresize", "-f", "-L",
+ size_str, bs->filename, NULL};
+
+success = g_spawn_sync(NULL, (gchar **)cmd, NULL, spawn_flags,
+ NULL, NULL, NULL, , , );
+
+if (success && g_spawn_check_exit_status(status, _exit)) {
+return 0;
+}
+
+if (success) {
+error_setg(errp, "%s: %s", gerr_exit->message, err);
+} else {
+error_setg(errp, "lvresize execution error: %s",
gerr->message);
+}
+
+return -EINVAL;
+}
+
if (offset != cur_length && exact) {
error_setg(errp, "Cannot resize device files");
return -ENOTSUP;
--
2.40.1
Re: [PATCH v2] block: Use LVM tools for LV block device truncation
On 3/14/24 13:44, Daniel P. Berrangé wrote:
On Wed, Mar 13, 2024 at 11:43:27AM +0100, Alexander Ivanov wrote:
If a block device is an LVM logical volume we can resize it using
standard LVM tools.
Add a helper to detect if a device is a DM device. In raw_co_truncate()
check if the block device is DM and resize it executing lvresize.
Signed-off-by: Alexander Ivanov
---
block/file-posix.c | 61 ++
1 file changed, 61 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..5f07d98aa5 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd,
int64_t offset,
return raw_thread_pool_submit(handle_aiocb_truncate, );
}
static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
bool exact, PreallocMode prealloc,
BdrvRequestFlags flags, Error **errp)
@@ -2670,6 +2702,35 @@ static int coroutine_fn raw_co_truncate(BlockDriverState
*bs, int64_t offset,
if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
int64_t cur_length = raw_getlength(bs);
+/*
+ * Try to resize an LVM device using LVM tools.
+ */
+if (device_is_dm() && offset > 0) {
+int spawn_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL;
+int status;
+bool success;
+char *err;
+GError *gerr = NULL;
+g_autofree char *size_str = g_strdup_printf("%ldB", offset);
offset is 64-bit, but '%ld' is not guaranteed to be 64-bit. I expect
this will break on 32-bit platforms. Try PRId64 instead.
+const char *cmd[] = {"lvresize", "-f", "-L",
+ size_str, bs->filename, NULL};
+
+success = g_spawn_sync(NULL, (gchar **)cmd, NULL, spawn_flags,
+ NULL, NULL, NULL, , , );
+
+if (success && WEXITSTATUS(status) == 0) {
+return 0;
+}
We should probably check 'g_spawn_check_wait_status' rather than
WEXITSTATUS, as this then gives us further eror message details
that
Thank you.
I think it would be better to use 'g_spawn_check_exit_status' because
there is no
'g_spawn_check_wait_status' in glib before 2.70 and even in 2.78 it
leads to
'g_spawn_check_wait_status is deprecated: Not available before 2.70' error.
+
+if (!success) {
+error_setg(errp, "lvresize execution error: %s",
gerr->message);
+} else {
+error_setg(errp, "%s", err);
...we would also include here, such as the exit code or terminal
signal.
+}
+
+return -EINVAL;
+}
+
if (offset != cur_length && exact) {
error_setg(errp, "Cannot resize device files");
return -ENOTSUP;
--
2.40.1
With regards,
Daniel
--
Best regards,
Alexander Ivanov
[PATCH v2] block: Use LVM tools for LV block device truncation
If a block device is an LVM logical volume we can resize it using
standard LVM tools.
Add a helper to detect if a device is a DM device. In raw_co_truncate()
check if the block device is DM and resize it executing lvresize.
Signed-off-by: Alexander Ivanov
---
block/file-posix.c | 61 ++
1 file changed, 61 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..5f07d98aa5 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2642,6 +2642,38 @@ raw_regular_truncate(BlockDriverState *bs, int fd,
int64_t offset,
return raw_thread_pool_submit(handle_aiocb_truncate, );
}
+static bool device_is_dm(struct stat *st)
+{
+unsigned int maj, maj2;
+char line[32], devname[16];
+bool ret = false;
+FILE *f;
+
+if (!S_ISBLK(st->st_mode)) {
+return false;
+}
+
+f = fopen("/proc/devices", "r");
+if (!f) {
+return false;
+}
+
+maj = major(st->st_rdev);
+
+while (fgets(line, sizeof(line), f)) {
+if (sscanf(line, "%u %15s", , devname) != 2) {
+continue;
+}
+if (strcmp(devname, "device-mapper") == 0) {
+ret = (maj == maj2);
+break;
+}
+}
+
+fclose(f);
+return ret;
+}
+
static int coroutine_fn raw_co_truncate(BlockDriverState *bs, int64_t offset,
bool exact, PreallocMode prealloc,
BdrvRequestFlags flags, Error **errp)
@@ -2670,6 +2702,35 @@ static int coroutine_fn raw_co_truncate(BlockDriverState
*bs, int64_t offset,
if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
int64_t cur_length = raw_getlength(bs);
+/*
+ * Try to resize an LVM device using LVM tools.
+ */
+if (device_is_dm() && offset > 0) {
+int spawn_flags = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL;
+int status;
+bool success;
+char *err;
+GError *gerr = NULL;
+g_autofree char *size_str = g_strdup_printf("%ldB", offset);
+const char *cmd[] = {"lvresize", "-f", "-L",
+ size_str, bs->filename, NULL};
+
+success = g_spawn_sync(NULL, (gchar **)cmd, NULL, spawn_flags,
+ NULL, NULL, NULL, , , );
+
+if (success && WEXITSTATUS(status) == 0) {
+return 0;
+}
+
+if (!success) {
+error_setg(errp, "lvresize execution error: %s",
gerr->message);
+} else {
+error_setg(errp, "%s", err);
+}
+
+return -EINVAL;
+}
+
if (offset != cur_length && exact) {
error_setg(errp, "Cannot resize device files");
return -ENOTSUP;
--
2.40.1
Re: [PATCH] block: Use LVM tools for LV block device truncation
Thank you for the review.
On 3/11/24 19:24, Daniel P. Berrangé wrote:
On Mon, Mar 11, 2024 at 06:40:44PM +0100, Alexander Ivanov wrote:
If a block device is an LVM logical volume we can resize it using
standard LVM tools.
In raw_co_truncate() check if the block device is a LV using lvdisplay
and resize it executing lvresize.
Signed-off-by: Alexander Ivanov
---
block/file-posix.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..cf8a04e6f7 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2670,6 +2670,33 @@ static int coroutine_fn raw_co_truncate(BlockDriverState
*bs, int64_t offset,
if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
int64_t cur_length = raw_getlength(bs);
+/*
+ * Check if the device is an LVM logical volume and try to resize
+ * it using LVM tools.
+ */
+if (S_ISBLK(st.st_mode) && offset > 0) {
+char cmd[PATH_MAX + 32];
+
+snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null",
+ bs->filename);
PATH_MAX + snprint is a bad practice - g_strdup_printf() is recommended
for dynamic allocation, along with g_autofree for release.
+ret = system(cmd);
IMHO using 'system' for spawning processes is dubious in any non-trivial
program.
Historically at least it does not have well defined behaviour wrt signal
handling in the child, before execve is called. ie potentially a signal
handler registered by QEMU in the parent could run in the child having
ill-effects.
'system' also executes via the shell which opens up many risks with
unsanitized files path being substituted into the command line.
+if (ret != 0) {
+error_setg(errp, "lvdisplay returned %d error for '%s'",
+ ret, bs->filename);
+return ret;
+}
Calling 'lvdisplay' doesn't seem to be needed. Surely 'lvresize' is
going to report errors if it isn't an LVM device.
The problem is that we don't know if 'lvresize' returned an error because of
non-LVM device or there was another reason. For the first variant we should
continue original code execution, for the second - return an error.
If we want to detect an LVM device though, couldn't we lookup
'device-mapper' in /proc/devices and then major the device major
node number.
It will require more code for getting device major, file reading and
parsing.
Why this way is better?
+
+snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null",
+ offset, bs->filename);
+ret = system(cmd);
+if (ret != 0) {
+error_setg(errp, "lvresize returned %d error for '%s'",
+ ret, bs->filename);
lvresize might display an message on stderr on failure but that's at best
going to QEMU's stderr. Any error needs to be captured and put into
this error message that's fed back to the user / QMP client.
It seems I need to implement a high level function for programs
execution. Looked at
g_spawn_sync(), but it uses fork() under the hood and we could have a
performance
issue. Haven't found a high level function that uses vfork() and allows
to catch stderr.
+}
+
+ return ret;
+}
+
With regards,
Daniel
--
Best regards,
Alexander Ivanov
[PATCH v5 09/22] 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 Reviewed-by: Denis V. Lunev --- block/parallels.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/parallels.c b/block/parallels.c index 9056dd6a11..66990925a8 100644 --- a/block/parallels.c +++ b/block/parallels.c @@ -842,7 +842,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.40.1
[PATCH v5 17/22] 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
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 72 +++
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index f51e35788e..5155b8ac48 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -778,7 +778,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);
@@ -786,43 +786,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.40.1
[PATCH v5 22/22] 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.40.1
[PATCH v5 21/22] 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.40.1
[PATCH v5 20/22] 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.40.1
[PATCH v5 08/22] 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
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index e7c3763017..9056dd6a11 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -812,6 +812,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.40.1
[PATCH v5 16/22] 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
Reviewed-by: Denis V. Lunev
---
block/parallels-ext.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 67fa3b73c2..0816b9dc07 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -278,6 +278,7 @@ parallels_parse_format_extension(BlockDriverState *bs,
int64_t ext_off,
if (!bitmap) {
goto fail;
}
+bdrv_dirty_bitmap_set_persistence(bitmap, true);
bitmaps = g_slist_append(bitmaps, bitmap);
break;
--
2.40.1
[PATCH v5 15/22] 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 1ec23aa1e7..67fa3b73c2 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -383,7 +383,7 @@ static void GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
offset = 0;
while ((offset = bdrv_dirty_bitmap_next_dirty(bitmap, offset, bm_size)) >=
0) {
idx = offset / bits_per_cluster;
-int64_t cluster_off, end, write_size;
+int64_t cluster_off, end, write_size, first_zero;
offset = QEMU_ALIGN_DOWN(offset, bits_per_cluster);
end = MIN(bm_size, offset + bits_per_cluster);
@@ -398,6 +398,16 @@ static void GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
l1_table[idx] = 0;
+first_zero = bdrv_dirty_bitmap_next_zero(bitmap, offset, bm_size);
+if (first_zero < 0) {
+goto clean_allocated_clusters;
+}
+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) {
error_report("Can't save dirty bitmap '%s': cluster allocation
error",
--
2.40.1
[PATCH v5 04/22] parallels: Limit search in parallels_mark_used to the last marked claster
There is no necessity to search to the end of the bitmap. Limit the search
area as cluster_index + count.
Add cluster_end variable to avoid its calculation in a few places.
Signed-off-by: Alexander Ivanov
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index ae524f1820..4470519656 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -182,13 +182,14 @@ 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);
+uint32_t cluster_end, cluster_index = host_cluster_index(s, off);
unsigned long next_used;
-if (cluster_index + count > bitmap_size) {
+cluster_end = cluster_index + count;
+if (cluster_end > bitmap_size) {
return -E2BIG;
}
-next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
-if (next_used < cluster_index + count) {
+next_used = find_next_bit(bitmap, cluster_end, cluster_index);
+if (next_used < cluster_end) {
return -EBUSY;
}
bitmap_set(bitmap, cluster_index, count);
--
2.40.1
[PATCH v5 02/22] 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
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 072b1efd78..992362ce29 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1424,6 +1424,20 @@ fail:
return ret;
}
+static int GRAPH_RDLOCK 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)
{
@@ -1432,12 +1446,7 @@ static void parallels_close(BlockDriverState *bs)
GRAPH_RDLOCK_GUARD_MAINLOOP();
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);
@@ -1476,6 +1485,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.40.1
[PATCH v5 19/22] 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.
Note: bdrv_pwrite_zeroes() was removed from the branch where we reallocate
clusters. It's redundant - bdrv_pwrite_zeroes() was already called for all
such clusters.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 45 +
block/parallels.h | 1 -
2 files changed, 13 insertions(+), 33 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index fd80179642..11b6f97454 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,6 +268,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 GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -279,7 +286,7 @@ int64_t GRAPH_RDLOCK
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;
@@ -307,31 +314,14 @@ int64_t GRAPH_RDLOCK
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);
/* Not enough continuous clusters in the middle, adjust the size */
*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;
-
-/*
- * 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_pwrite_zeroes(bs->file, host_off, bytes, 0);
-if (ret < 0) {
-return ret;
-}
-}
}
ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
@@ -763,13 +753,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;
}
@@ -808,8 +792,6 @@ parallels_check_unused_clusters(BlockDriverState *bs, bool
truncate)
return ret;
}
-s->data_end = end_off / BDRV_SECTOR_SIZE;
-
return leak;
}
@@ -1401,8 +1383,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...
@@ -1435,11 +1416,11 @@ 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;
+break;
}
}
-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 d1e46dcfa8..1d41a59b6f 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.40.1
[PATCH v5 07/22] 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
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/block/parallels.c b/block/parallels.c
index c6d82961c9..e7c3763017 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -811,6 +811,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.40.1
[PATCH v5 13/22] parallels: Let image extensions work in RW mode
Now we support extensions saving and can let to work with them in
read-write mode.
Place dirty bitmap loading after used bitmap initialization - in the next
patch we will work with used bitmap during dirty bitmap loading.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 4
block/parallels.c | 25 -
2 files changed, 8 insertions(+), 21 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 7fa14b057d..2d839006a4 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data,
size_t data_size,
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 9536e4241e..f51e35788e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1378,23 +1378,6 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
s->header_size = size;
}
-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;
-}
-}
-}
-
if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_INACTIVE)) {
s->header->inuse = cpu_to_le32(HEADER_INUSE_MAGIC);
ret = parallels_update_header(bs);
@@ -1432,6 +1415,14 @@ static int parallels_open(BlockDriverState *bs, QDict
*options, int flags,
}
need_check = need_check || ret < 0; /* These are correctable errors */
+if (ph.ext_off) {
+ret = parallels_read_format_extension(
+bs, le64_to_cpu(ph.ext_off) << BDRV_SECTOR_BITS, errp);
+if (ret < 0) {
+goto fail;
+}
+}
+
/*
* 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.
--
2.40.1
[PATCH v5 11/22] 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 | 187 ++
block/parallels.c | 16 +++-
block/parallels.h | 5 ++
3 files changed, 206 insertions(+), 2 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index b4e14c88f2..9cb8e65c0d 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"
@@ -300,3 +301,189 @@ out:
return ret;
}
+
+static void GRAPH_RDLOCK 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, bits_per_cluster;
+int64_t bm_size, ser_size, offset, buf_used;
+int64_t idx, 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);
+bits_per_cluster =
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)) {
+error_report("Can't save dirty bitmap: not enoughf space");
+return;
+}
+
+name = bdrv_dirty_bitmap_name(bitmap);
+ret = qemu_uuid_parse(name, );
+if (ret < 0) {
+error_report("Can't save dirty bitmap '%s': ID parsing error", name);
+return;
+}
+
+fh = (ParallelsFeatureHeader *)*buf;
+bh = (ParallelsDirtyBitmapFeature *)(fh + 1);
+l1_table = (uint64_t *)(bh + 1);
+
+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) {
+idx = offset / bits_per_cluster;
+int64_t cluster_off, end, write_size;
+
+offset = QEMU_ALIGN_DOWN(offset, bits_per_cluster);
+end = MIN(bm_size, offset + bits_per_cluster);
+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);
+}
+
+l1_table[idx] = 0;
+
+cluster_off = parallels_allocate_host_clusters(bs, _size);
+if (cluster_off <= 0) {
+error_report("Can't save dirty bitmap '%s': cluster allocation
error",
+ name);
+goto clean_allocated_clusters;
+}
+
+l1_table[idx] = cpu_to_le64(cluster_off >> BDRV_SECTOR_BITS);
+
+ret = bdrv_pwrite(bs->file, cluster_off, s->cluster_size, bm_buf, 0);
+if (ret < 0) {
+error_report("Can't save dirty bitmap '%s': write error", name);
+memset(>magic, 0, sizeof(fh->magic));
+goto clean_allocated_clusters;
+}
+
+offset = end;
+}
+
+*buf_size -= buf_used;
+*buf += buf_used;
+qemu_vfree(bm_buf);
+return;
+
+clean_allocated_clusters:
+for (; idx >= 0; idx--) {
+uint64_t cluster_off;
+cluster_off = le64_to_cpu(l1_table[idx]);
+if (cluster_off == 0 || cluster_off == 1) {
+continue;
+}
+parallels_mark_unused(bs, s->used_bmap, s->used_bmap_size,
+ cluster_off << BDRV_SECTOR_BITS, 1);
+}
+qemu_vfree(bm_buf);
+}
+
+int GRAPH_RDLOCK
+parallels_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+{
+
[PATCH v5 03/22] 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
Reviewed-by: Denis V. Lunev
---
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 992362ce29..ae524f1820 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 GRAPH_RDLOCK
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;
@@ -827,7 +829,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;
@@ -887,7 +889,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 423b2ad727..68077416b1 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 GRAPH_RDLOCK
parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
Error **errp);
--
2.40.1
[PATCH v5 10/22] 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.
In some checks (like duplication check) we need to work with used bitmap
so it have to be created before checks are running.
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 66990925a8..86958506d0 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1426,13 +1426,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.40.1
[PATCH v5 01/22] 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 Reviewed-by: Denis V. Lunev --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 9205a0864f..072b1efd78 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.40.1
[PATCH v5 18/22] parallels: Check unused clusters in parallels_check_leak()
Since we have a used bitmap, current leak check is useless. Add
parallels_check_unused_clusters() helper that checks if there are unused
clusters at the end of image and truncates them (if needed). Use it for
leak check and at image inactivation.
In leak check we need to recreate used bitmap because it will be used
during work. At image inactivation we can skip it.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 106 +-
1 file changed, 68 insertions(+), 38 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 5155b8ac48..fd80179642 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -773,57 +773,89 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
return 0;
}
+static int64_t GRAPH_RDLOCK
+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;
+
+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;
+int64_t leak, count, size;
int ret;
+leak = parallels_check_unused_clusters(bs, fix & BDRV_FIX_LEAKS);
+if (leak < 0) {
+res->check_errors++;
+return leak;
+}
+if (leak == 0) {
+return 0;
+}
+
+parallels_free_used_bitmap(bs);
+ret = parallels_fill_used_bitmap(bs);
+if (ret < 0) {
+res->check_errors++;
+return ret;
+}
+
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;
@@ -1475,9 +1507,7 @@ static int GRAPH_RDLOCK
parallels_inactivate(BlockDriverState *bs)
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);
+ret = parallels_check_unused_clusters(bs, true);
if (ret < 0) {
error_report("Failed to truncate image: %s", strerror(-ret));
}
--
2.40.1
[PATCH v5 06/22] 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
parallels_allocate_host_clusters().
This function can be called not only from coroutines so all the
*_co_* functions were replaced by corresponding wrappers.
Signed-off-by: Alexander Ivanov
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 123 +-
block/parallels.h | 3 ++
2 files changed, 71 insertions(+), 55 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 13726fb3d5..c6d82961c9 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,42 +268,21 @@ 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 GRAPH_RDLOCK 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
@@ -311,15 +290,14 @@ 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, 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;
@@ -329,15 +307,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;
@@ -349,14 +327,59 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
[PATCH v5 12/22] parallels: drop dirty bitmap data if the image was not properly closed
From: "Denis V. Lunev"
This data is obsolete.
The approach is exactly the same like we use with QCOW2.
Signed-off-by: Denis V. Lunev
---
block/parallels-ext.c | 8
1 file changed, 8 insertions(+)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 9cb8e65c0d..7fa14b057d 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,14 @@ parallels_parse_format_extension(BlockDriverState *bs,
uint8_t *ext_cluster,
return 0;
case PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC:
+if (s->header_unclean) {
+/*
+ * The image was not closed correctly and thus dirty bitmap
+ * data inside the image is considered as incorrect and thus
+ * it should be dropper, exactly like we do for QCOW2.
+ */
+break;
+}
bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
if (!bitmap) {
goto fail;
--
2.40.1
[PATCH v5 05/22] parallels: Add parallels_mark_unused() helper
Add a helper to set unused areas in the used bitmap.
Signed-off-by: Alexander Ivanov
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 18 ++
block/parallels.h | 2 ++
2 files changed, 20 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 4470519656..13726fb3d5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -196,6 +196,24 @@ 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_end, cluster_index = host_cluster_index(s, off);
+unsigned long next_unused;
+cluster_end = cluster_index + count;
+if (cluster_end > bitmap_size) {
+return -E2BIG;
+}
+next_unused = find_next_zero_bit(bitmap, cluster_end, cluster_index);
+if (next_unused < cluster_end) {
+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 68077416b1..02b857b4a4 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 GRAPH_RDLOCK
parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
--
2.40.1
[PATCH v5 00/22] 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-v5 v5: 6: Returned a comment about fallocate. 10: Added a motivation of used_bitmap usage to the commit message. 11: Renamed 'limit' variable. Added a few error messages. Fixed 'remaining' value by extension header size. Simplified bh and l1_table pointer calculation. Added clusters cleaning after error. Add return value to parallels_store_persistent_dirty_bitmaps(). 12: New patch, all the next patches will be shifted by 1. Added header unclean check to dirty bitmap loading. 13: Placed dirty bitmap loading after used bitmap initialization. 14: New patch, all the next patches will be shifted by 2. Preserved extensions cluster for non-transient extensions. 16: (previous numeration): Dropped the patch because the next patch deleted all the code of this patch. All the next patches will be shifted by 1. 18: Moved used bitmap parallels_check_unused_clusters() to parallels_check_leak(). 19: Removed redundant bdrv_pwrite_zeroes() (see a note in the commit message). Added a break to outside-of-image clusters at image opening. v4: 4: A new patch with limitation of search in parallels_mark_used. 5: Previously 4. Search is limited to (cluster_index + count). 6: Previously 5. Added GRAPH_RDLOCK annotation, added a note in the commit message. 11: Previously 10. Added GRAPH_RDLOCK annotation. 16-18: Added GRAPH_RDLOCK annotations. 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: Limit search in parallels_mark_used to the last marked claster 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: Let image extensions work in RW mode parallels: Preserve extensions cluster for non-transient extensions 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: 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 Denis V. Lunev (1): parallels: drop dirty bitmap data if the image was not properly closed block/parallels-ext.c | 243 - block/parallels.c | 367 block/parallels.h | 14 +- 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, 502 insertions(+), 183 deletions(-) -- 2.40.1
[PATCH v5 14/22] parallels: Preserve extensions cluster for non-transient extensions
There could be non-transient extensions that require presence of the
extensions cluster during work. Mark extensions cluster as used at
extensions loading end nullify l1 tables of dirty bitmaps.
Use this cluster at dirty bitmap saving if it exists.
Signed-off-by: Alexander Ivanov
---
block/parallels-ext.c | 49 ++-
1 file changed, 35 insertions(+), 14 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 2d839006a4..1ec23aa1e7 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -122,7 +122,7 @@ finish:
*/
static BdrvDirtyBitmap * GRAPH_RDLOCK
parallels_load_bitmap(BlockDriverState *bs, uint8_t *data, size_t data_size,
- Error **errp)
+ int64_t ext_off, Error **errp)
{
int ret;
ParallelsDirtyBitmapFeature bf;
@@ -130,6 +130,7 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data,
size_t data_size,
BdrvDirtyBitmap *bitmap;
QemuUUID uuid;
char uuidstr[UUID_STR_LEN];
+int64_t l1_off;
int i;
if (data_size < sizeof(bf)) {
@@ -171,16 +172,29 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t
*data, size_t data_size,
ret = parallels_load_bitmap_data(bs, l1_table, bf.l1_size, bitmap, errp);
if (ret < 0) {
-bdrv_release_dirty_bitmap(bitmap);
-return NULL;
+goto err;
+}
+
+if (!(bs->open_flags & BDRV_O_RDWR)) {
+return bitmap;
+}
+
+l1_off = ext_off + sizeof(ParallelsDirtyBitmapFeature);
+ret = bdrv_pwrite_zeroes(bs->file, l1_off,
+ bf.l1_size * sizeof(uint64_t), 0);
+if (ret < 0) {
+goto err;
}
return bitmap;
+err:
+bdrv_release_dirty_bitmap(bitmap);
+return NULL;
}
static int GRAPH_RDLOCK
-parallels_parse_format_extension(BlockDriverState *bs, uint8_t *ext_cluster,
- Error **errp)
+parallels_parse_format_extension(BlockDriverState *bs, int64_t ext_off,
+ uint8_t *ext_cluster, Error **errp)
{
BDRVParallelsState *s = bs->opaque;
int ret;
@@ -259,7 +273,8 @@ parallels_parse_format_extension(BlockDriverState *bs,
uint8_t *ext_cluster,
*/
break;
}
-bitmap = parallels_load_bitmap(bs, pos, fh.data_size, errp);
+bitmap = parallels_load_bitmap(bs, pos, fh.data_size,
+ ext_off + pos - ext_cluster, errp);
if (!bitmap) {
goto fail;
}
@@ -298,7 +313,11 @@ int parallels_read_format_extension(BlockDriverState *bs,
goto out;
}
-ret = parallels_parse_format_extension(bs, ext_cluster, errp);
+ret = parallels_parse_format_extension(bs, ext_off, ext_cluster, errp);
+if (ret == 0) {
+ret = parallels_mark_used(bs, s->used_bmap, s->used_bmap_size,
+ ext_off, 1);
+}
out:
qemu_vfree(ext_cluster);
@@ -429,8 +448,6 @@ parallels_store_persistent_dirty_bitmaps(BlockDriverState
*bs, Error **errp)
size_t hash_len = 0;
int ret;
-s->header->ext_off = 0;
-
if (!bdrv_has_named_bitmaps(bs)) {
return 0;
}
@@ -447,11 +464,15 @@ parallels_store_persistent_dirty_bitmaps(BlockDriverState
*bs, Error **errp)
parallels_save_bitmap(bs, bitmap, , );
}
-header_off = parallels_allocate_host_clusters(bs, _size);
-if (header_off < 0) {
-error_report("Can't save dirty bitmap: cluster allocation error");
-ret = header_off;
-goto end;
+if (s->header->ext_off) {
+header_off = le64_to_cpu(s->header->ext_off) << BDRV_SECTOR_BITS;
+} else {
+header_off = parallels_allocate_host_clusters(bs, _size);
+if (header_off < 0) {
+error_report("Can't save dirty bitmap: cluster allocation error");
+ret = header_off;
+goto end;
+}
}
ret = qcrypto_hash_bytes(QCRYPTO_HASH_ALG_MD5,
--
2.40.1
[PATCH] block: Use LVM tools for LV block device truncation
If a block device is an LVM logical volume we can resize it using
standard LVM tools.
In raw_co_truncate() check if the block device is a LV using lvdisplay
and resize it executing lvresize.
Signed-off-by: Alexander Ivanov
---
block/file-posix.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/block/file-posix.c b/block/file-posix.c
index 35684f7e21..cf8a04e6f7 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2670,6 +2670,33 @@ static int coroutine_fn raw_co_truncate(BlockDriverState
*bs, int64_t offset,
if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
int64_t cur_length = raw_getlength(bs);
+/*
+ * Check if the device is an LVM logical volume and try to resize
+ * it using LVM tools.
+ */
+if (S_ISBLK(st.st_mode) && offset > 0) {
+char cmd[PATH_MAX + 32];
+
+snprintf(cmd, sizeof(cmd), "lvdisplay %s > /dev/null",
+ bs->filename);
+ret = system(cmd);
+if (ret != 0) {
+error_setg(errp, "lvdisplay returned %d error for '%s'",
+ ret, bs->filename);
+return ret;
+}
+
+snprintf(cmd, sizeof(cmd), "lvresize -f -L %ldB %s > /dev/null",
+ offset, bs->filename);
+ret = system(cmd);
+if (ret != 0) {
+error_setg(errp, "lvresize returned %d error for '%s'",
+ ret, bs->filename);
+}
+
+return ret;
+}
+
if (offset != cur_length && exact) {
error_setg(errp, "Cannot resize device files");
return -ENOTSUP;
--
2.40.1
Re: [PATCH v4 13/21] parallels: Handle L1 entries equal to one
On 1/18/24 14:37, Denis V. Lunev wrote:
On 12/28/23 11:12, Alexander Ivanov wrote:
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 195b01b109..033ca3ec3a 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
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 GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
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;
That is not enough. We should handle all-one and all-zeroes according
to the spec and all-zeroes would be much more common.
Buffer for extensions contains zeroes before handling (it was allocated with
qemu_blockalign0). We skip all all-zeroes l1 entries and the stay zeroed.
--
Best regards,
Alexander Ivanov
Re: [PATCH v4 12/21] parallels: Let image extensions work in RW mode
On 1/18/24 14:31, Denis V. Lunev wrote:
On 1/16/24 15:45, Denis V. Lunev wrote:
On 12/28/23 11:12, Alexander Ivanov wrote:
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 c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs,
uint8_t *data, size_t data_size,
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 a49922c6a7..d5d87984cf 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;
}
}
Reviewed-by: Denis V. Lunev
This patch also deserves a note, what will happen with
format extensions clusters. According to the current
policy, we have only transient extensions, i.e.
CBT. Cluster allocation mechanism will reuse these
clusters as they are not marked as used.
Thus we should either set format extension offset
in the header to 0 or perform any other correct
measures to properly handle this.
Yes, all the clusters used by extensions are marked as unused
on loading. In further work they can be reallocated for other
purposes.
Agree that we need to set ext_off to zero.
It should also be noted, that on any QEMU crash
appropriate format extensions are to be properly
treated. We could not make them RW until this would
not be addressed as we could easily mess up with
trashed metadata.
If QEMU crashes after extensions loading there will be
zero in the ext_off field and an inappropriate dirty bitmap
will be ignored.
--
Best regards,
Alexander Ivanov
Re: [PATCH v3] blockcommit: Reopen base image as RO after abort
Could you please review the patch?
On 1/30/24 10:14, Alexander Ivanov wrote:
If a blockcommit is aborted the base image remains in RW mode, that leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk inside the guest ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda
--abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos:0
flags: 02140002 < The last 2 means RW mode
If the base image is in RW mode at the end of blockcommit and was in RO
mode before blockcommit, check if src BDS has refcnt > 1. If so, the BDS
will not be removed after blockcommit, and we should make the base image
RO. Otherwise check recursively if there is a parent BDS of src BDS and
reopen the base BDS in RO in this case.
Signed-off-by: Alexander Ivanov
---
block/mirror.c | 38 --
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 5145eb53e1..52a7fee75e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -93,6 +93,7 @@ typedef struct MirrorBlockJob {
int64_t active_write_bytes_in_flight;
bool prepared;
bool in_drain;
+bool base_ro;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -652,6 +653,32 @@ static void coroutine_fn
mirror_wait_for_all_io(MirrorBlockJob *s)
}
}
+/*
+ * Check recursively if there is a parent BDS referenced more than
+ * min_refcnt times. This argument is needed because at the first
+ * call there is a bds referenced in blockcommit.
+ */
+static bool bdrv_chain_has_significant_parent(BlockDriverState *bs)
+{
+BdrvChild *parent;
+BlockDriverState *parent_bs;
+
+QLIST_FOREACH(parent, >parents, next) {
+if (!(parent->klass->parent_is_bds)) {
+continue;
+}
+parent_bs = parent->opaque;
+if (parent_bs->drv && !parent_bs->drv->is_filter) {
+return true;
+}
+if (bdrv_chain_has_significant_parent(parent_bs)) {
+return true;
+}
+}
+
+return false;
+}
+
/**
* mirror_exit_common: handle both abort() and prepare() cases.
* for .prepare, returns 0 on success and -errno on failure.
@@ -793,6 +820,11 @@ static int mirror_exit_common(Job *job)
bdrv_drained_end(target_bs);
bdrv_unref(target_bs);
+if (s->base_ro && !bdrv_is_read_only(target_bs) &&
+(src->refcnt > 1 || bdrv_chain_has_significant_parent(src))) {
+bdrv_reopen_set_read_only(target_bs, true, NULL);
+}
+
bs_opaque->job = NULL;
bdrv_drained_end(src);
@@ -1715,6 +1747,7 @@ static BlockJob *mirror_start_job(
bool is_none_mode, BlockDriverState *base,
bool auto_complete, const char *filter_node_name,
bool is_mirror, MirrorCopyMode copy_mode,
+ bool base_ro,
Error **errp)
{
MirrorBlockJob *s;
@@ -1798,6 +1831,7 @@ static BlockJob *mirror_start_job(
bdrv_unref(mirror_top_bs);
s->mirror_top_bs = mirror_top_bs;
+s->base_ro = base_ro;
/* No resize for the target either; while the mirror is still running, a
* consistent read isn't necessarily possible. We could possibly allow
@@ -2027,7 +2061,7 @@ void mirror_start(const char *job_id, BlockDriverState
*bs,
speed, granularity, buf_size, backing_mode, zero_target,
on_source_error, on_target_error, unmap, NULL, NULL,
_job_driver, is_none_mode, base, false,
- filter_node_name, true, copy_mode, errp);
+ filter_node_name, true, copy_mode, false, errp);
}
BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -2056,7 +2090,7 @@ BlockJob *commit_active_start(const char *job_id,
BlockDriverState *bs,
on_error, on_error, true, cb, opaque,
_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- errp);
+ base_read_only, errp);
if (!job) {
goto error_restore_flags;
}
--
Best regards,
Alexander Ivanov
Re: [PATCH v4 11/21] parallels: Add dirty bitmaps saving
On 1/18/24 14:27, Denis V. Lunev wrote:
On 12/28/23 11:12, Alexander Ivanov wrote:
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 | 168 ++
block/parallels.c | 16 +++-
block/parallels.h | 5 ++
3 files changed, 187 insertions(+), 2 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index b4e14c88f2..c83d1ea393 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"
@@ -300,3 +301,170 @@ out:
return ret;
}
+
+static void GRAPH_RDLOCK parallels_save_bitmap(BlockDriverState *bs,
+ BdrvDirtyBitmap *bitmap,
+ uint8_t **buf, int
*buf_size)
Do we need a error?
I think no. We save bitmaps in a loop. If some bitmap has a problem it
would be better
just to print an error message and try to save other bitmaps.
+{
+ BDRVParallelsState *s = bs->opaque;
+ ParallelsFeatureHeader *fh;
+ ParallelsDirtyBitmapFeature *bh;
+ uint64_t *l1_table, l1_size, granularity, limit;
I would say that 'limit' here means 'bits_in_cluster'
We are writing the new code and I would prefer if we
would have bits, bytes, clusters, sectors etc are
clearly seen in variable names. It is quite complex
to track various measurables.
OK
+ 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);
As far as I can see, bdrv_dirty_bitmap_serialization_size() returns
bytes.
That is correct. Thus multiplying it by 8 seems fatal mistake.
l1_size is amount of entries in l1_table. Every entry has 8 bytes size
and corresponds
to one cluster containing a part of a bitmap.
I am also quite unsure that we should roundup to the cluster, that
will occupy more clusters than needed. Can you please take a look
here
https://src.openvz.org/users/ibazhitov/repos/ploop/browse/tools/ploop-cbt.c
Why shouldn't we? We need to have l1_table which should be able to contain
all the bitmap data. Note: there is not a ROUND_UP(), but DIV_ROUND_UP().
+ /* 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));
bh = fh + 1 ?
+ l1_table = (uint64_t *)((uint8_t *)bh + sizeof(*bh));
l1_table = bh + 1 ?
Yes, it's better.
+
+ fh->magic = cpu_to_le64(PARALLELS_DIRTY_BITMAP_FEATURE_MAGIC);
+ fh->data_size = cpu_to_le32(l1_size * 8 + sizeof(*bh));
I am quite concerned here. Please compare with
int save_dirty_bitmap(int devfd, struct delta *delta, off_t offset,
void *buf, __u32 *size, void *or_data, writer_fn wr,
void *data)
{
int ret = 0;
struct ploop_pvd_header *vh;
size_t block_size;
__u64 bits, bytes, *p;
__u32 byte_granularity;
void *block;
struct ploop_pvd_dirty_bitmap_raw *raw = (struct
ploop_pvd_dirty_bitmap_raw *)buf;
char x[50];
vh = (struct ploop_pvd_header *)delta->hdr0;
/* granularity and uuid */
if ((ret = cbt_get_dirty_bitmap_metadata(devfd, raw->m_Id,
>m_Granularity)))
return ret;
raw->m_Granularity /= SECTOR_SIZE;
block_size = vh->m_Sectors * SECTOR_SIZE;
if (p_memalign((void **), 4096, block_size))
return SYSEXIT_MALLOC;
raw->m_Size = vh->m_SizeInSectors_v2;
byte_granularity = raw->m_Granularity * SECTOR_SIZE;
bits = ((raw->m_Size + raw->m_Granularity - 1) / raw->m_Granularity);
bytes = (bits + 7) >> 3;
raw->m_L1Size = (bytes + block_size - 1) / block_size;
which means that the header is rotten. In that case can you pls
take a look why this has not been caught by tests?
It looks the same if block_size is cluster size in bytes.
bytes - bitm
[PATCH v3] blockcommit: Reopen base image as RO after abort
If a blockcommit is aborted the base image remains in RW mode, that leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk inside the guest ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda --abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos:0
flags: 02140002 < The last 2 means RW mode
If the base image is in RW mode at the end of blockcommit and was in RO
mode before blockcommit, check if src BDS has refcnt > 1. If so, the BDS
will not be removed after blockcommit, and we should make the base image
RO. Otherwise check recursively if there is a parent BDS of src BDS and
reopen the base BDS in RO in this case.
Signed-off-by: Alexander Ivanov
---
block/mirror.c | 38 --
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 5145eb53e1..52a7fee75e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -93,6 +93,7 @@ typedef struct MirrorBlockJob {
int64_t active_write_bytes_in_flight;
bool prepared;
bool in_drain;
+bool base_ro;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -652,6 +653,32 @@ static void coroutine_fn
mirror_wait_for_all_io(MirrorBlockJob *s)
}
}
+/*
+ * Check recursively if there is a parent BDS referenced more than
+ * min_refcnt times. This argument is needed because at the first
+ * call there is a bds referenced in blockcommit.
+ */
+static bool bdrv_chain_has_significant_parent(BlockDriverState *bs)
+{
+BdrvChild *parent;
+BlockDriverState *parent_bs;
+
+QLIST_FOREACH(parent, >parents, next) {
+if (!(parent->klass->parent_is_bds)) {
+continue;
+}
+parent_bs = parent->opaque;
+if (parent_bs->drv && !parent_bs->drv->is_filter) {
+return true;
+}
+if (bdrv_chain_has_significant_parent(parent_bs)) {
+return true;
+}
+}
+
+return false;
+}
+
/**
* mirror_exit_common: handle both abort() and prepare() cases.
* for .prepare, returns 0 on success and -errno on failure.
@@ -793,6 +820,11 @@ static int mirror_exit_common(Job *job)
bdrv_drained_end(target_bs);
bdrv_unref(target_bs);
+if (s->base_ro && !bdrv_is_read_only(target_bs) &&
+(src->refcnt > 1 || bdrv_chain_has_significant_parent(src))) {
+bdrv_reopen_set_read_only(target_bs, true, NULL);
+}
+
bs_opaque->job = NULL;
bdrv_drained_end(src);
@@ -1715,6 +1747,7 @@ static BlockJob *mirror_start_job(
bool is_none_mode, BlockDriverState *base,
bool auto_complete, const char *filter_node_name,
bool is_mirror, MirrorCopyMode copy_mode,
+ bool base_ro,
Error **errp)
{
MirrorBlockJob *s;
@@ -1798,6 +1831,7 @@ static BlockJob *mirror_start_job(
bdrv_unref(mirror_top_bs);
s->mirror_top_bs = mirror_top_bs;
+s->base_ro = base_ro;
/* No resize for the target either; while the mirror is still running, a
* consistent read isn't necessarily possible. We could possibly allow
@@ -2027,7 +2061,7 @@ void mirror_start(const char *job_id, BlockDriverState
*bs,
speed, granularity, buf_size, backing_mode, zero_target,
on_source_error, on_target_error, unmap, NULL, NULL,
_job_driver, is_none_mode, base, false,
- filter_node_name, true, copy_mode, errp);
+ filter_node_name, true, copy_mode, false, errp);
}
BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -2056,7 +2090,7 @@ BlockJob *commit_active_start(const char *job_id,
BlockDriverState *bs,
on_error, on_error, true, cb, opaque,
_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- errp);
+ base_read_only, errp);
if (!job) {
goto error_restore_flags;
}
--
2.40.1
[PATCH v2] blockcommit: Reopen base image as RO after abort
If a blockcommit is aborted the base image remains in RW mode, that leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk inside the guest ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda --abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos:0
flags: 02140002 < The last 2 means RW mode
Reopen the base image in RO mode in mirror_exit_common() if the blockjob
is aborted and the base image was opened in RO mode before the blockcommit.
Signed-off-by: Alexander Ivanov
---
block/mirror.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 5145eb53e1..9a5f5cbb4d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -93,6 +93,7 @@ typedef struct MirrorBlockJob {
int64_t active_write_bytes_in_flight;
bool prepared;
bool in_drain;
+bool base_ro;
} MirrorBlockJob;
typedef struct MirrorBDSOpaque {
@@ -793,6 +794,10 @@ static int mirror_exit_common(Job *job)
bdrv_drained_end(target_bs);
bdrv_unref(target_bs);
+if (abort && s->base_ro && !bdrv_is_read_only(target_bs)) {
+bdrv_reopen_set_read_only(target_bs, true, NULL);
+}
+
bs_opaque->job = NULL;
bdrv_drained_end(src);
@@ -1715,6 +1720,7 @@ static BlockJob *mirror_start_job(
bool is_none_mode, BlockDriverState *base,
bool auto_complete, const char *filter_node_name,
bool is_mirror, MirrorCopyMode copy_mode,
+ bool base_ro,
Error **errp)
{
MirrorBlockJob *s;
@@ -1798,6 +1804,7 @@ static BlockJob *mirror_start_job(
bdrv_unref(mirror_top_bs);
s->mirror_top_bs = mirror_top_bs;
+s->base_ro = base_ro;
/* No resize for the target either; while the mirror is still running, a
* consistent read isn't necessarily possible. We could possibly allow
@@ -2027,7 +2034,7 @@ void mirror_start(const char *job_id, BlockDriverState
*bs,
speed, granularity, buf_size, backing_mode, zero_target,
on_source_error, on_target_error, unmap, NULL, NULL,
_job_driver, is_none_mode, base, false,
- filter_node_name, true, copy_mode, errp);
+ filter_node_name, true, copy_mode, false, errp);
}
BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
@@ -2056,7 +2063,7 @@ BlockJob *commit_active_start(const char *job_id,
BlockDriverState *bs,
on_error, on_error, true, cb, opaque,
_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- errp);
+ base_read_only, errp);
if (!job) {
goto error_restore_flags;
}
--
2.40.1
[PATCH v4 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 195b01b109..033ca3ec3a 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
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 GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
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.40.1
[PATCH v4 10/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 0ae06ec0b1..f38dd2309c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1421,13 +1421,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.40.1
[PATCH v4 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.40.1
[PATCH v4 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 c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data,
size_t data_size,
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 a49922c6a7..d5d87984cf 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.40.1
[PATCH v4 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 033ca3ec3a..2a7ff6e35b 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,7 @@ parallels_parse_format_extension(BlockDriverState *bs,
uint8_t *ext_cluster,
if (!bitmap) {
goto fail;
}
+bdrv_dirty_bitmap_set_persistence(bitmap, true);
bitmaps = g_slist_append(bitmaps, bitmap);
break;
--
2.40.1
[PATCH v4 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 136865d53e..5ed58826bb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -768,57 +768,87 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
return 0;
}
+static int64_t GRAPH_RDLOCK
+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;
@@ -1454,23 +1484,6 @@ fail:
return ret;
}
-static int GRAPH_RDLOCK 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;
@@ -1488,7 +1501,7 @@ static int GRAPH_RDLOCK
parallels_inactivate(BlockDriverState *bs)
parallels
[PATCH v4 05/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 | 18 ++
block/parallels.h | 2 ++
2 files changed, 20 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 4470519656..13726fb3d5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -196,6 +196,24 @@ 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_end, cluster_index = host_cluster_index(s, off);
+unsigned long next_unused;
+cluster_end = cluster_index + count;
+if (cluster_end > bitmap_size) {
+return -E2BIG;
+}
+next_unused = find_next_zero_bit(bitmap, cluster_end, cluster_index);
+if (next_unused < cluster_end) {
+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 68077416b1..02b857b4a4 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 GRAPH_RDLOCK
parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
--
2.40.1
[PATCH v4 04/21] parallels: Limit search in parallels_mark_used to the last marked claster
There is no necessity to search to the end of the bitmap. Limit the search
area as cluster_index + count.
Add cluster_end variable to avoid its calculation in a few places.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index ae524f1820..4470519656 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -182,13 +182,14 @@ 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);
+uint32_t cluster_end, cluster_index = host_cluster_index(s, off);
unsigned long next_used;
-if (cluster_index + count > bitmap_size) {
+cluster_end = cluster_index + count;
+if (cluster_end > bitmap_size) {
return -E2BIG;
}
-next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
-if (next_used < cluster_index + count) {
+next_used = find_next_bit(bitmap, cluster_end, cluster_index);
+if (next_used < cluster_end) {
return -EBUSY;
}
bitmap_set(bitmap, cluster_index, count);
--
2.40.1
[PATCH v4 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.40.1
[PATCH v4 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.40.1
[PATCH v4 11/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 | 168 ++
block/parallels.c | 16 +++-
block/parallels.h | 5 ++
3 files changed, 187 insertions(+), 2 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index b4e14c88f2..c83d1ea393 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"
@@ -300,3 +301,170 @@ out:
return ret;
}
+
+static void GRAPH_RDLOCK 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 GRAPH_RDLOCK
+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
[PATCH v4 09/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 04c114f696..0ae06ec0b1 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.40.1
[PATCH v4 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
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 072b1efd78..992362ce29 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1424,6 +1424,20 @@ fail:
return ret;
}
+static int GRAPH_RDLOCK 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)
{
@@ -1432,12 +1446,7 @@ static void parallels_close(BlockDriverState *bs)
GRAPH_RDLOCK_GUARD_MAINLOOP();
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);
@@ -1476,6 +1485,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.40.1
[PATCH v4 08/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 8a6e2ba7ee..04c114f696 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.40.1
[PATCH v4 06/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
parallels_allocate_host_clusters().
This function can be called not only from coroutines so all the
*_co_* functions were replaced by corresponding wrappers.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 128 --
block/parallels.h | 3 ++
2 files changed, 71 insertions(+), 60 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 13726fb3d5..658902ae51 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,58 +268,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 GRAPH_RDLOCK 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;
@@ -329,15 +302,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;
@@ -349,14 +322,59 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
if (s->prealloc_mode == PRL_PREAL
[PATCH v4 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 5ed58826bb..2803119699 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,6 +268,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 GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -279,7 +286,7 @@ int64_t GRAPH_RDLOCK
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;
@@ -302,9 +309,6 @@ int64_t GRAPH_RDLOCK
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);
@@ -320,8 +324,7 @@ int64_t GRAPH_RDLOCK
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;
@@ -758,13 +761,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;
}
@@ -803,8 +800,6 @@ parallels_check_unused_clusters(BlockDriverState *bs, bool
truncate)
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...
@@ -1436,11 +1430,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 9db4f5c908..b494d93139 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.40.1
[PATCH v4 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 d5d87984cf..fb7bc5e555 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -773,7 +773,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);
@@ -781,43 +781,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.40.1
[PATCH v4 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 fb7bc5e555..136865d53e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1454,6 +1454,23 @@ fail:
return ret;
}
+static int GRAPH_RDLOCK 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;
@@ -1471,8 +1488,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.40.1
[PATCH v4 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
Reviewed-by: Denis V. Lunev
---
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 992362ce29..ae524f1820 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 GRAPH_RDLOCK
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;
@@ -827,7 +829,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;
@@ -887,7 +889,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 423b2ad727..68077416b1 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 GRAPH_RDLOCK
parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
Error **errp);
--
2.40.1
[PATCH v4 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-v4 v4: 4: A new patch with limitation of search in parallels_mark_used. 5: Previously 4. Search is limited to (cluster_index + count). 6: Previously 5. Added GRAPH_RDLOCK annotation, added a note in the commit message. 11: Previously 10. Added GRAPH_RDLOCK annotation. 16-18: Added GRAPH_RDLOCK annotations. 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: Limit search in parallels_mark_used to the last marked claster 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: 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 | 183 +- block/parallels.c | 371 block/parallels.h | 14 +- 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, 453 insertions(+), 176 deletions(-) -- 2.40.1
[PATCH v4 07/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 658902ae51..8a6e2ba7ee 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.40.1
[PATCH v4 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 Reviewed-by: Denis V. Lunev --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 9205a0864f..072b1efd78 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.40.1
Re: [PATCH 01/21] parallels: Set s->used_bmap to NULL in parallels_free_used_bitmap()
Sorry, incorrect patch set, please ignore. On 12/28/23 10:41, Alexander Ivanov wrote: 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 Reviewed-by: Denis V. Lunev --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 9205a0864f..072b1efd78 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 -- Best regards, Alexander Ivanov
[PATCH 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 fb7bc5e555..136865d53e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1454,6 +1454,23 @@ fail:
return ret;
}
+static int GRAPH_RDLOCK 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;
@@ -1471,8 +1488,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.40.1
[PATCH 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
Reviewed-by: Denis V. Lunev
---
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 992362ce29..ae524f1820 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 GRAPH_RDLOCK
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;
@@ -827,7 +829,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;
@@ -887,7 +889,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 423b2ad727..68077416b1 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 GRAPH_RDLOCK
parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
Error **errp);
--
2.40.1
[PATCH 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 c83d1ea393..195b01b109 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -175,10 +175,6 @@ parallels_load_bitmap(BlockDriverState *bs, uint8_t *data,
size_t data_size,
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 a49922c6a7..d5d87984cf 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.40.1
[PATCH 08/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 8a6e2ba7ee..04c114f696 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.40.1
[PATCH 07/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 658902ae51..8a6e2ba7ee 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.40.1
[PATCH 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
Reviewed-by: Denis V. Lunev
---
block/parallels.c | 22 --
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 072b1efd78..992362ce29 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1424,6 +1424,20 @@ fail:
return ret;
}
+static int GRAPH_RDLOCK 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)
{
@@ -1432,12 +1446,7 @@ static void parallels_close(BlockDriverState *bs)
GRAPH_RDLOCK_GUARD_MAINLOOP();
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);
@@ -1476,6 +1485,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.40.1
[PATCH 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.40.1
[PATCH 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.40.1
[PATCH 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 Reviewed-by: Denis V. Lunev --- block/parallels.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/parallels.c b/block/parallels.c index 9205a0864f..072b1efd78 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.40.1
[PATCH 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 136865d53e..5ed58826bb 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -768,57 +768,87 @@ parallels_check_outside_image(BlockDriverState *bs,
BdrvCheckResult *res,
return 0;
}
+static int64_t GRAPH_RDLOCK
+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;
@@ -1454,23 +1484,6 @@ fail:
return ret;
}
-static int GRAPH_RDLOCK 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;
@@ -1488,7 +1501,7 @@ static int GRAPH_RDLOCK
parallels_inactivate(BlockDriverState *bs)
parallels
[PATCH 11/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 | 168 ++
block/parallels.c | 16 +++-
block/parallels.h | 5 ++
3 files changed, 187 insertions(+), 2 deletions(-)
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index b4e14c88f2..c83d1ea393 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"
@@ -300,3 +301,170 @@ out:
return ret;
}
+
+static void GRAPH_RDLOCK 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 GRAPH_RDLOCK
+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
[PATCH 10/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 0ae06ec0b1..f38dd2309c 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1421,13 +1421,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.40.1
[PATCH 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 5ed58826bb..2803119699 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,6 +268,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 GRAPH_RDLOCK parallels_allocate_host_clusters(BlockDriverState *bs,
int64_t *clusters)
{
@@ -279,7 +286,7 @@ int64_t GRAPH_RDLOCK
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;
@@ -302,9 +309,6 @@ int64_t GRAPH_RDLOCK
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);
@@ -320,8 +324,7 @@ int64_t GRAPH_RDLOCK
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;
@@ -758,13 +761,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;
}
@@ -803,8 +800,6 @@ parallels_check_unused_clusters(BlockDriverState *bs, bool
truncate)
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...
@@ -1436,11 +1430,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 9db4f5c908..b494d93139 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.40.1
[PATCH 06/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
parallels_allocate_host_clusters().
This function can be called not only from coroutines so all the
*_co_* functions were replaced by corresponding wrappers.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 128 --
block/parallels.h | 3 ++
2 files changed, 71 insertions(+), 60 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index 13726fb3d5..658902ae51 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -268,58 +268,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 GRAPH_RDLOCK 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;
@@ -329,15 +302,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;
@@ -349,14 +322,59 @@ allocate_clusters(BlockDriverState *bs, int64_t
sector_num,
*/
if (s->prealloc_mode == PRL_PREAL
[PATCH 05/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 | 18 ++
block/parallels.h | 2 ++
2 files changed, 20 insertions(+)
diff --git a/block/parallels.c b/block/parallels.c
index 4470519656..13726fb3d5 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -196,6 +196,24 @@ 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_end, cluster_index = host_cluster_index(s, off);
+unsigned long next_unused;
+cluster_end = cluster_index + count;
+if (cluster_end > bitmap_size) {
+return -E2BIG;
+}
+next_unused = find_next_zero_bit(bitmap, cluster_end, cluster_index);
+if (next_unused < cluster_end) {
+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 68077416b1..02b857b4a4 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 GRAPH_RDLOCK
parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
--
2.40.1
[PATCH 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.40.1
[PATCH 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 195b01b109..033ca3ec3a 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -354,7 +354,7 @@ static void GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
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 GRAPH_RDLOCK
parallels_save_bitmap(BlockDriverState *bs,
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.40.1
[PATCH 09/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 04c114f696..0ae06ec0b1 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.40.1
[PATCH 04/21] parallels: Limit search in parallels_mark_used to the last marked claster
There is no necessity to search to the end of the bitmap. Limit the search
area as cluster_index + count.
Add cluster_end variable to avoid its calculation in a few places.
Signed-off-by: Alexander Ivanov
---
block/parallels.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/parallels.c b/block/parallels.c
index ae524f1820..4470519656 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -182,13 +182,14 @@ 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);
+uint32_t cluster_end, cluster_index = host_cluster_index(s, off);
unsigned long next_used;
-if (cluster_index + count > bitmap_size) {
+cluster_end = cluster_index + count;
+if (cluster_end > bitmap_size) {
return -E2BIG;
}
-next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
-if (next_used < cluster_index + count) {
+next_used = find_next_bit(bitmap, cluster_end, cluster_index);
+if (next_used < cluster_end) {
return -EBUSY;
}
bitmap_set(bitmap, cluster_index, count);
--
2.40.1
[PATCH 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 d5d87984cf..fb7bc5e555 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -773,7 +773,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);
@@ -781,43 +781,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.40.1
[PATCH 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 033ca3ec3a..2a7ff6e35b 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -255,6 +255,7 @@ parallels_parse_format_extension(BlockDriverState *bs,
uint8_t *ext_cluster,
if (!bitmap) {
goto fail;
}
+bdrv_dirty_bitmap_set_persistence(bitmap, true);
bitmaps = g_slist_append(bitmaps, bitmap);
break;
--
2.40.1
Re: [PATCH] blockcommit: Reopen base image as RO after abort
Ping. Could someone please review the code?
On 11/30/23 11:11, Alexander Ivanov wrote:
If a blockcommit is aborted the base image remains in RW mode, that leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda
--abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos:0
flags: 02140002
Reopen the base image in RO mode in mirror_exit_common() if the blockjob
is aborted.
Signed-off-by: Alexander Ivanov
---
block/mirror.c | 4
1 file changed, 4 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index dcd88de2e3..50a2825b1c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -789,6 +789,10 @@ static int mirror_exit_common(Job *job)
block_job_remove_all_bdrv(bjob);
bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs,
_abort);
+if (abort && !bdrv_is_read_only(target_bs)) {
+bdrv_reopen_set_read_only(target_bs, true, NULL);
+}
+
bs_opaque->job = NULL;
bdrv_drained_end(src);
--
Best regards,
Alexander Ivanov
[PATCH] blockcommit: Reopen base image as RO after abort
If a blockcommit is aborted the base image remains in RW mode, that leads
to a fail of subsequent live migration.
How to reproduce:
$ virsh snapshot-create-as vm snp1 --disk-only
*** write something to the disk ***
$ virsh blockcommit vm vda --active --shallow && virsh blockjob vm vda --abort
$ lsof /vzt/vm.qcow2
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
qemu-syst 433203 root 45u REG 253,0 1724776448 133 /vzt/vm.qcow2
$ cat /proc/433203/fdinfo/45
pos:0
flags: 02140002
Reopen the base image in RO mode in mirror_exit_common() if the blockjob
is aborted.
Signed-off-by: Alexander Ivanov
---
block/mirror.c | 4
1 file changed, 4 insertions(+)
diff --git a/block/mirror.c b/block/mirror.c
index dcd88de2e3..50a2825b1c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -789,6 +789,10 @@ static int mirror_exit_common(Job *job)
block_job_remove_all_bdrv(bjob);
bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, _abort);
+if (abort && !bdrv_is_read_only(target_bs)) {
+bdrv_reopen_set_read_only(target_bs, true, NULL);
+}
+
bs_opaque->job = NULL;
bdrv_drained_end(src);
--
2.39.2
Re: [PATCH v3 04/21] parallels: Add parallels_mark_unused() helper
On 10/30/23 10:09, Denis V. Lunev wrote:
On 10/30/23 10:06, Denis V. Lunev wrote:
On 10/27/23 09:46, Alexander Ivanov wrote:
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;
+ }
I would limit the search with 'off + count'.
There is no necessity to traverse the bitmap further.
Den
I mean 'cluster_index + off' to avoid the confusion.
Den
Do you mean 'cluster_index + count'?
Should I set the same limit in parallels_mark_used()?
--
Best regards,
Alexander Ivanov
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 */
