| Message ID | ce32e1c2-c652-e83b-a6f4-c9773099cf9a@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] block: always update auto_backing_file to full path | expand |
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) { >> >
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); + } + } backing_overridden = bdrv_backing_overridden(bs); if (bs->open_flags & BDRV_O_NO_IO) {
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(+)