Re: [Qemu-devel] [RESEND PATCH V3] qemu-img info: show nocow info

[Kevin Wolf <kwolf@redhat.com>]

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