| Message ID | 1406688906-6072-1-git-send-email-cyliu@suse.com |
|---|---|
| State | New |
| Headers | show |
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 <cyliu@suse.com> > Reviewed-by: Eric Blake <eblake@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
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 <cyliu@suse.com> > Reviewed-by: Eric Blake <eblake@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 0x00800000 /* 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
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 <cyliu@suse.com> >> Reviewed-by: Eric Blake <eblake@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.
>>> On 11/22/2014 at 12:25 AM, in message <20141121162505.GF3956@noname.redhat.com>, Kevin Wolf <kwolf@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 <cyliu@suse.com> > > Reviewed-by: Eric Blake <eblake@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 0x00800000 /* 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 > > >
Am 24.11.2014 um 04:11 hat Chun Yan Liu geschrieben: > > > >>> On 11/22/2014 at 12:25 AM, in message > <20141121162505.GF3956@noname.redhat.com>, Kevin Wolf <kwolf@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 <cyliu@suse.com> > > > Reviewed-by: Eric Blake <eblake@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 0x00800000 /* 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 create options, that was a different patch. The create options are already in a single place in raw-posix.c, so that's all fine. The patch I commented on is about query-block. Second, a block device in qemu isn't just qcow2 or just vmdk. It has at least two levels and is something like "qcow2 over file" or "vmdk over http". The nocow field is in fact specific to raw-posix.c (which provides the "file" driver), you won't get it for images backed by an http, gluster or nbd driver. Which format driver (like qcow2, vmdk or raw) is used on top of this, is completely independent from that. Kevin
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 0x00800000 /* 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 + 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' } } ## # @ImageCheck: