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

[Chun Yan Liu]

>>> On 11/22/2014 at 12:25 AM, in message
<address@hidden>, Kevin Wolf <address@hidden> 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 <address@hidden> 
> > Reviewed-by: Eric Blake <address@hidden> 
> > --- 
> > 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 
>  
>  
>