Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info
Am 24.11.2014 um 04:11 hat Chun Yan Liu geschrieben:
On 11/22/2014 at 12:25 AM, in message
20141121162505.gf3...@noname.redhat.com, Kevin Wolf kw...@redhat.com
wrote:
Am 30.07.2014 um 04:55 hat Chunyan Liu geschrieben:
Add nocow info in 'qemu-img info' output to show whether the file
currently has NOCOW flag set or not.
Signed-off-by: Chunyan Liu cy...@suse.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
Resend for QEMU 2.2. Change json version comment. Add Reviewed-by.
block/qapi.c | 25 +
qapi/block-core.json | 5 -
2 files changed, 29 insertions(+), 1 deletion(-)
Unfortunately, I have never been CCed on this patch and I saw the code
it added only now. I think it doesn't work correctly and has the wrong
design, so I would prefer to revert it for 2.2 rather than making a bad
API stable.
diff --git a/block/qapi.c b/block/qapi.c
index f44f6b4..aa53f19 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -28,6 +28,13 @@
#include qapi-visit.h
#include qapi/qmp-output-visitor.h
#include qapi/qmp/types.h
+#ifdef __linux__
+#include linux/fs.h
+#include sys/ioctl.h
+#ifndef FS_NOCOW_FL
+#define FS_NOCOW_FL 0x0080 /* Do not cow file */
+#endif
+#endif
BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
{
@@ -173,6 +180,20 @@ void bdrv_query_image_info(BlockDriverState *bs,
Error *err = NULL;
ImageInfo *info = g_new0(ImageInfo, 1);
+#ifdef __linux__
+int fd, attr;
+
+/* get NOCOW info */
+fd = qemu_open(bs-filename, O_RDONLY | O_NONBLOCK);
+if (fd = 0) {
+if (ioctl(fd, FS_IOC_GETFLAGS, attr) == 0 (attr
FS_NOCOW_FL)) {
+info-has_nocow = true;
+info-nocow = true;
+}
+qemu_close(fd);
+}
+#endif
Code like this has no business in the general block layer. This belongs
in raw-posix, which already has an open fd, and nowhere else. There you
wouldn't have to rely on bs-filename, which may be empty or contain
something different from a local filename, e.g. a URL. The approach of
this patch also doesn't work if the file has been renamed or deleted
since qemu opened it (e.g. because of -snapshot).
In short: This code may happen to work in some special cases, but
generally speaking, it is wrong.
bdrv_get_geometry(bs, total_sectors);
info-filename= g_strdup(bs-filename);
@@ -625,4 +646,8 @@ void bdrv_image_info_dump(fprintf_function
func_fprintf,
void *f,
func_fprintf(f, Format specific information:\n);
bdrv_image_info_specific_dump(func_fprintf, f,
info-format_specific);
}
+
+if (info-has_nocow info-nocow) {
+func_fprintf(f, NOCOW flag: set\n);
+}
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e378653..72b4015 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -115,6 +115,8 @@
# @format-specific: #optional structure supplying additional
format-specific
# information (since 1.7)
#
+# @nocow: #optional info of whether NOCOW flag is set or not. (since
2.2)
+#
# Since: 1.3
#
##
@@ -126,7 +128,8 @@
'*backing-filename': 'str', '*full-backing-filename': 'str',
'*backing-filename-format': 'str', '*snapshots':
['SnapshotInfo'],
'*backing-image': 'ImageInfo',
- '*format-specific': 'ImageInfoSpecific' } }
+ '*format-specific': 'ImageInfoSpecific',
+ '*nocow': 'bool' } }
As this field makes only sense for raw-posix, it should be moved to
ImageInfoSpecific. (format-specific is also an unfortunate misnomer,
it should be driver-specific, but it's too late to fix that.
I'm not disagreeing revert this patch if there is mistake. But here I need
to add some explanation to seek a correct future solution:
'Nocow' is not a specific attribute to raw-posix. This option is valid
to general disk images types, like 'raw', 'qcow2', 'qcow', 'vhdx', 'vpc',
'vmdk', etc. The reason why we only add 'nocow' in .createoptions in
raw-posix.c is: when creating all those file types, the creating point
is always in raw-posix.c, and since the NOCOW attr must be handled
in creating point, so we can only handle it in raw-posix.c file.
That's why we only add 'nocow' to raw-posix.c, but not other file types.
In creating, it will append two createoptions (like creating qcow2 file,
it will append qcow2 createoptions and also raw_posix createoptions),
so add 'nocow' to raw-posix is enough. Add 'nocow' to each file types
will touch many files.
I believe there are two misunderstandings here:
First, I'm not talking about
Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info
On 11/22/2014 at 12:25 AM, in message
20141121162505.gf3...@noname.redhat.com, Kevin Wolf kw...@redhat.com wrote:
Am 30.07.2014 um 04:55 hat Chunyan Liu geschrieben:
Add nocow info in 'qemu-img info' output to show whether the file
currently has NOCOW flag set or not.
Signed-off-by: Chunyan Liu cy...@suse.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
Resend for QEMU 2.2. Change json version comment. Add Reviewed-by.
block/qapi.c | 25 +
qapi/block-core.json | 5 -
2 files changed, 29 insertions(+), 1 deletion(-)
Unfortunately, I have never been CCed on this patch and I saw the code
it added only now. I think it doesn't work correctly and has the wrong
design, so I would prefer to revert it for 2.2 rather than making a bad
API stable.
diff --git a/block/qapi.c b/block/qapi.c
index f44f6b4..aa53f19 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -28,6 +28,13 @@
#include qapi-visit.h
#include qapi/qmp-output-visitor.h
#include qapi/qmp/types.h
+#ifdef __linux__
+#include linux/fs.h
+#include sys/ioctl.h
+#ifndef FS_NOCOW_FL
+#define FS_NOCOW_FL 0x0080 /* Do not cow file */
+#endif
+#endif
BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
{
@@ -173,6 +180,20 @@ void bdrv_query_image_info(BlockDriverState *bs,
Error *err = NULL;
ImageInfo *info = g_new0(ImageInfo, 1);
+#ifdef __linux__
+int fd, attr;
+
+/* get NOCOW info */
+fd = qemu_open(bs-filename, O_RDONLY | O_NONBLOCK);
+if (fd = 0) {
+if (ioctl(fd, FS_IOC_GETFLAGS, attr) == 0 (attr
FS_NOCOW_FL)) {
+info-has_nocow = true;
+info-nocow = true;
+}
+qemu_close(fd);
+}
+#endif
Code like this has no business in the general block layer. This belongs
in raw-posix, which already has an open fd, and nowhere else. There you
wouldn't have to rely on bs-filename, which may be empty or contain
something different from a local filename, e.g. a URL. The approach of
this patch also doesn't work if the file has been renamed or deleted
since qemu opened it (e.g. because of -snapshot).
In short: This code may happen to work in some special cases, but
generally speaking, it is wrong.
bdrv_get_geometry(bs, total_sectors);
info-filename= g_strdup(bs-filename);
@@ -625,4 +646,8 @@ void bdrv_image_info_dump(fprintf_function
func_fprintf,
void *f,
func_fprintf(f, Format specific information:\n);
bdrv_image_info_specific_dump(func_fprintf, f,
info-format_specific);
}
+
+if (info-has_nocow info-nocow) {
+func_fprintf(f, NOCOW flag: set\n);
+}
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e378653..72b4015 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -115,6 +115,8 @@
# @format-specific: #optional structure supplying additional
format-specific
# information (since 1.7)
#
+# @nocow: #optional info of whether NOCOW flag is set or not. (since 2.2)
+#
# Since: 1.3
#
##
@@ -126,7 +128,8 @@
'*backing-filename': 'str', '*full-backing-filename': 'str',
'*backing-filename-format': 'str', '*snapshots':
['SnapshotInfo'],
'*backing-image': 'ImageInfo',
- '*format-specific': 'ImageInfoSpecific' } }
+ '*format-specific': 'ImageInfoSpecific',
+ '*nocow': 'bool' } }
As this field makes only sense for raw-posix, it should be moved to
ImageInfoSpecific. (format-specific is also an unfortunate misnomer,
it should be driver-specific, but it's too late to fix that.
I'm not disagreeing revert this patch if there is mistake. But here I need
to add some explanation to seek a correct future solution:
'Nocow' is not a specific attribute to raw-posix. This option is valid
to general disk images types, like 'raw', 'qcow2', 'qcow', 'vhdx', 'vpc',
'vmdk', etc. The reason why we only add 'nocow' in .createoptions in
raw-posix.c is: when creating all those file types, the creating point
is always in raw-posix.c, and since the NOCOW attr must be handled
in creating point, so we can only handle it in raw-posix.c file.
That's why we only add 'nocow' to raw-posix.c, but not other file types.
In creating, it will append two createoptions (like creating qcow2 file,
it will append qcow2 createoptions and also raw_posix createoptions),
so add 'nocow' to raw-posix is enough. Add 'nocow' to each file types
will touch many files.
- Chunyan
Kevin
Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info
Am 30.07.2014 um 04:55 hat Chunyan Liu geschrieben:
Add nocow info in 'qemu-img info' output to show whether the file
currently has NOCOW flag set or not.
Signed-off-by: Chunyan Liu cy...@suse.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
Resend for QEMU 2.2. Change json version comment. Add Reviewed-by.
block/qapi.c | 25 +
qapi/block-core.json | 5 -
2 files changed, 29 insertions(+), 1 deletion(-)
Unfortunately, I have never been CCed on this patch and I saw the code
it added only now. I think it doesn't work correctly and has the wrong
design, so I would prefer to revert it for 2.2 rather than making a bad
API stable.
diff --git a/block/qapi.c b/block/qapi.c
index f44f6b4..aa53f19 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -28,6 +28,13 @@
#include qapi-visit.h
#include qapi/qmp-output-visitor.h
#include qapi/qmp/types.h
+#ifdef __linux__
+#include linux/fs.h
+#include sys/ioctl.h
+#ifndef FS_NOCOW_FL
+#define FS_NOCOW_FL 0x0080 /* Do not cow file */
+#endif
+#endif
BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
{
@@ -173,6 +180,20 @@ void bdrv_query_image_info(BlockDriverState *bs,
Error *err = NULL;
ImageInfo *info = g_new0(ImageInfo, 1);
+#ifdef __linux__
+int fd, attr;
+
+/* get NOCOW info */
+fd = qemu_open(bs-filename, O_RDONLY | O_NONBLOCK);
+if (fd = 0) {
+if (ioctl(fd, FS_IOC_GETFLAGS, attr) == 0 (attr FS_NOCOW_FL)) {
+info-has_nocow = true;
+info-nocow = true;
+}
+qemu_close(fd);
+}
+#endif
Code like this has no business in the general block layer. This belongs
in raw-posix, which already has an open fd, and nowhere else. There you
wouldn't have to rely on bs-filename, which may be empty or contain
something different from a local filename, e.g. a URL. The approach of
this patch also doesn't work if the file has been renamed or deleted
since qemu opened it (e.g. because of -snapshot).
In short: This code may happen to work in some special cases, but
generally speaking, it is wrong.
bdrv_get_geometry(bs, total_sectors);
info-filename= g_strdup(bs-filename);
@@ -625,4 +646,8 @@ void bdrv_image_info_dump(fprintf_function func_fprintf,
void *f,
func_fprintf(f, Format specific information:\n);
bdrv_image_info_specific_dump(func_fprintf, f,
info-format_specific);
}
+
+if (info-has_nocow info-nocow) {
+func_fprintf(f, NOCOW flag: set\n);
+}
}
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e378653..72b4015 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -115,6 +115,8 @@
# @format-specific: #optional structure supplying additional format-specific
# information (since 1.7)
#
+# @nocow: #optional info of whether NOCOW flag is set or not. (since 2.2)
+#
# Since: 1.3
#
##
@@ -126,7 +128,8 @@
'*backing-filename': 'str', '*full-backing-filename': 'str',
'*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'],
'*backing-image': 'ImageInfo',
- '*format-specific': 'ImageInfoSpecific' } }
+ '*format-specific': 'ImageInfoSpecific',
+ '*nocow': 'bool' } }
As this field makes only sense for raw-posix, it should be moved to
ImageInfoSpecific. (format-specific is also an unfortunate misnomer,
it should be driver-specific, but it's too late to fix that.)
Kevin
Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info
On 11/21/2014 09:25 AM, Kevin Wolf wrote: Am 30.07.2014 um 04:55 hat Chunyan Liu geschrieben: Add nocow info in 'qemu-img info' output to show whether the file currently has NOCOW flag set or not. Signed-off-by: Chunyan Liu cy...@suse.com Reviewed-by: Eric Blake ebl...@redhat.com --- Resend for QEMU 2.2. Change json version comment. Add Reviewed-by. block/qapi.c | 25 + qapi/block-core.json | 5 - 2 files changed, 29 insertions(+), 1 deletion(-) Unfortunately, I have never been CCed on this patch and I saw the code it added only now. I think it doesn't work correctly and has the wrong design, so I would prefer to revert it for 2.2 rather than making a bad API stable. Even though I reviewed the original, you make some valid points, and I can agree to a revert to avoid baking in a mistake. @@ -126,7 +128,8 @@ '*backing-filename': 'str', '*full-backing-filename': 'str', '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'], '*backing-image': 'ImageInfo', - '*format-specific': 'ImageInfoSpecific' } } + '*format-specific': 'ImageInfoSpecific', + '*nocow': 'bool' } } As this field makes only sense for raw-posix, it should be moved to ImageInfoSpecific. (format-specific is also an unfortunate misnomer, it should be driver-specific, but it's too late to fix that.) Yes, that alone is a compelling reason to back this out and get the interface right. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info
On Wed, Jul 30, 2014 at 10:55:06AM +0800, Chunyan Liu wrote: Add nocow info in 'qemu-img info' output to show whether the file currently has NOCOW flag set or not. Signed-off-by: Chunyan Liu cy...@suse.com Reviewed-by: Eric Blake ebl...@redhat.com --- Resend for QEMU 2.2. Change json version comment. Add Reviewed-by. block/qapi.c | 25 + qapi/block-core.json | 5 - 2 files changed, 29 insertions(+), 1 deletion(-) Thanks, applied to my block-next tree: https://github.com/stefanha/qemu/commits/block-next Stefan pgpRPBfZFSuXr.pgp Description: PGP signature
Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info
On 07/29/2014 01:18 AM, Chunyan Liu wrote: Add nocow info in 'qemu-img info' output to show whether the file currently has NOCOW flag set or not. Signed-off-by: Chunyan Liu cy...@suse.com Acked-by: Eric Blake ebl...@redhat.com Actually, this should be Reviewed-by, not Acked-by. My understanding is that Reviewed-by is stronger (I actually read through the patch, and looked for possible problems) while Acked-by is weaker (a maintainer stating that the work is taken in on the basis of third-party reviews without actually reviewing the patch itself). --- Resend for QEMU 2.2. Change json version comment. block/qapi.c | 25 + qapi/block-core.json | 5 - 2 files changed, 29 insertions(+), 1 deletion(-) -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info
On 7/29/2014 at 08:45 PM, in message 53d79771.1040...@redhat.com, Eric Blake ebl...@redhat.com wrote: On 07/29/2014 01:18 AM, Chunyan Liu wrote: Add nocow info in 'qemu-img info' output to show whether the file currently has NOCOW flag set or not. Signed-off-by: Chunyan Liu cy...@suse.com Acked-by: Eric Blake ebl...@redhat.com Actually, this should be Reviewed-by, not Acked-by. My understanding is that Reviewed-by is stronger (I actually read through the patch, and looked for possible problems) while Acked-by is weaker (a maintainer stating that the work is taken in on the basis of third-party reviews without actually reviewing the patch itself). Thanks for clarification. I corrected it and sent. --- Resend for QEMU 2.2. Change json version comment. block/qapi.c | 25 + qapi/block-core.json | 5 - 2 files changed, 29 insertions(+), 1 deletion(-) -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org
