* Re: [RFC PATCH] block: always update auto_backing_file to full path
2021-04-01 9:57 ` Max Reitz
@ 2021-04-01 21:47 ` Joe Jin
0 siblings, 0 replies; 3+ messages in thread
From: Joe Jin @ 2021-04-01 21:47 UTC (permalink / raw)
To: Max Reitz, Stefan Hajnoczi, Kevin Wolf; +Cc: qemu-devel
Hi Max,
Thanks very much for your replies.
On 4/1/21 2:57 AM, Max Reitz wrote:
> On 01.04.21 06:22, Joe Jin wrote:
>> Some time after created snapshot, auto_backing_file only has filename,
>> this confused overridden check, update it to full path if it is not.
>>
>> Signed-off-by: Joe Jin <joe.jin@oracle.com>
>> ---
>> block.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>
> Do you have a test for this?
My issue is my guest image is on NFS, I could created snapshot from ovirt but I could not delete it, tried to commit it by virsh and it complained qemu internal error:
# virsh blockcommit snap-test sda --active --verbose --pivot
error: internal error: qemu block name 'json:{"backing": {"driver": "qcow2", "file": {"driver": "file", "filename": "/rhev/data-center/mnt/nfs_server:_nfsexport/fee77d23-1291-4bdb-9157-12ff6cd5ee96/images/66bb625e-7458-478d-a238-b012f87062b8/919a4cda-bf11-4bb7-a2e3-d9e4515ed646"}}, "driver": "qcow2", "file": {"driver": "file", "filename": "/rhev/data-center/mnt/nfs_server:_nfsexport/fee77d23-1291-4bdb-9157-12ff6cd5ee96/images/66bb625e-7458-478d-a238-b012f87062b8/24d28fcd-67e3-49d2-8860-2110f3a5e796"}}' doesn't match expected '/rhev/data-center/mnt/nfs_server:_nfsexport/fee77d23-1291-4bdb-9157-12ff6cd5ee96/images/66bb625e-7458-478d-a238-b012f87062b8/24d28fcd-67e3-49d2-8860-2110f3a5e796'
Tracked qemu block and I found when issue happened, bdrv_backing_overridden() was tried to compare absolute path(bs->backing->bs->filename) with relative path(bs->auto_backing_file) but they are same filename, then it triggered generated json filename.
Regarding auto_backing_file, it updated by qcow2_do_open(), and read the backing file name from image:
/* read the backing file name */
if (header.backing_file_offset != 0) {
len = header.backing_file_size;
if (len > MIN(1023, s->cluster_size - header.backing_file_offset) ||
len >= sizeof(bs->backing_file)) {
error_setg(errp, "Backing file name too long");
ret = -EINVAL;
goto fail;
}
ret = bdrv_pread(bs->file, header.backing_file_offset,
bs->auto_backing_file, len);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not read backing file name");
goto fail;
}
bs->auto_backing_file[len] = '\0';
pstrcpy(bs->backing_file, sizeof(bs->backing_file),
bs->auto_backing_file);
s->image_backing_file = g_strdup(bs->auto_backing_file);
}
Updated auto_backing_file to absolute path, I could successfully delete snapshot from ovirt, or execute blockcommit by virsh.
>
> The thing is, I’m not sure about this solution, and I think having a test would help me understand better.
> bs->auto_backing_file is meant to track what filename a BDS would have if we opened bs->backing_file. To this end, we generally set it to whatever bs->backing_file is and then refresh it when we actually do open a BDS from it.
>
> So it seems strange to blindly modify it somewhere that doesn’t have to do with any of these things.
>
> Now, when opening a backing file from bs->backing_file, we first do make it an absolute filename via bdrv_get_full_backing_filename(). So it kind of seems prudent to replicate that elsewhere, but I’m not sure where exactly. I would think the best place would be whenever auto_backing_file is set to be equal to backing_file (which is generally in the image format drivers, when they read the backing file string from the image metadata), but that might break the strcmp() in bdrv_open_backing_file()...
>
> I don’t think bdrv_refresh_filename() is the right place, though, because I’m afraid that this might modify filenames we’ve already retrieved from the actual backing BDS, or something.
>
> For example, with this patch applied, iotest 024 fails.
Yes after applied my patch, I can reproduced the failure, it caused by bdrv_make_absolute_filename() returned relative path, not sure if this is bdrv_make_absolute_filename() bug?
Added path_is_absolute(backing_filename) check before update auto_backing_file, test passed:
+ backing_filename = bdrv_make_absolute_filename(bs, bs->auto_backing_file, &local_err);
+ if (!local_err && backing_filename && path_is_absolute(backing_filename)) {
+ pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
+ backing_filename);
>
>> diff --git a/block.c b/block.c
>> index c5b887cec1..8f9a027dee 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6969,6 +6969,19 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>> return;
>> }
>> + /* auto_backing_file only has filename, update it to the full path */
>> + if (bs->backing && bs->auto_backing_file[0] != '/') {
>> + char *backing_filename = NULL;
>> + Error *local_err = NULL;
>> +
>> + backing_filename = bdrv_make_absolute_filename(bs,
>> + bs->auto_backing_file, &local_err);
>> + if (!local_err && backing_filename) {
>> + pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file),
>> + backing_filename);
>> + g_free(backing_filename);
>> + }
>> + }
>
> All spaces here are 0xa0 (non-breaking space), which is kind of broken and makes it difficult to apply this patch.
Sorry about this, I may use git send-email to send it.
>
> Furthermore, if local_err != NULL, we’d need to free it.
Thanks for reminder, will take care of this.
Thanks,
Joe
>
> Max
>
>> backing_overridden = bdrv_backing_overridden(bs);
>> if (bs->open_flags & BDRV_O_NO_IO) {
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread