Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
On Wed, Dec 11, 2013 at 09:29:54AM +0100, Stefan Hajnoczi wrote: On Tue, Dec 10, 2013 at 10:23:41PM +, Alex Bennée wrote: stefa...@redhat.com writes: On Mon, Nov 18, 2013 at 12:54:59PM +0800, Chunyan Liu wrote: 2013/11/15 Stefan Hajnoczi stefa...@gmail.com On Thu, Nov 14, 2013 at 04:15:28PM +0800, Chunyan Liu wrote: Set NOCOW flag to newly created images to solve performance issues on btrfs. snip This should be optional and I'm not sure it should be the default. Rationale: If you're on btrfs you probably expect the copy-on-write and snapshot features of the file system. We shouldn't silently disable that unless the user asks for it. snip When the NOCOW attribute is set on a file, reflink copying (aka file-level snapshots) do not work: $ cp --reflink test.img test-snapshot.img This produces EINVAL. It is a regression if qemu-img create suddenly starts breaking this standard btrfs feature for existing users. Please make it a .bdrv_create() option which is off by default to avoid breaking existing users' workflows/scripts. The result should be something like: $ qemu-img create test.img 8G # file has NOCOW cleared $ qemu-img create -o nocow=on test.img 8G # file has NOCOW set I agree we shouldn't break existing work flows. I wonder if it would OK for qemu-img to issue a warning (when not --quiet) when it detects creation of an image on a partition where performance may not be as expected due to COW behaviour. A warning could help or at least prompt users to consider switching to nocow. IMHO such warnings are not nice - if a user does not wish to use the 'nocow' option because they want to keep the ability to use file label snapshots they should not be subjected to a warning message forever more. I suggest this is something to document in the man page instead. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
On Tue, Dec 10, 2013 at 10:23:41PM +, Alex Bennée wrote: stefa...@redhat.com writes: On Mon, Nov 18, 2013 at 12:54:59PM +0800, Chunyan Liu wrote: 2013/11/15 Stefan Hajnoczi stefa...@gmail.com On Thu, Nov 14, 2013 at 04:15:28PM +0800, Chunyan Liu wrote: Set NOCOW flag to newly created images to solve performance issues on btrfs. snip This should be optional and I'm not sure it should be the default. Rationale: If you're on btrfs you probably expect the copy-on-write and snapshot features of the file system. We shouldn't silently disable that unless the user asks for it. snip When the NOCOW attribute is set on a file, reflink copying (aka file-level snapshots) do not work: $ cp --reflink test.img test-snapshot.img This produces EINVAL. It is a regression if qemu-img create suddenly starts breaking this standard btrfs feature for existing users. Please make it a .bdrv_create() option which is off by default to avoid breaking existing users' workflows/scripts. The result should be something like: $ qemu-img create test.img 8G # file has NOCOW cleared $ qemu-img create -o nocow=on test.img 8G # file has NOCOW set I agree we shouldn't break existing work flows. I wonder if it would OK for qemu-img to issue a warning (when not --quiet) when it detects creation of an image on a partition where performance may not be as expected due to COW behaviour. A warning could help or at least prompt users to consider switching to nocow. Stefan
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
stefa...@redhat.com writes: On Mon, Nov 18, 2013 at 12:54:59PM +0800, Chunyan Liu wrote: 2013/11/15 Stefan Hajnoczi stefa...@gmail.com On Thu, Nov 14, 2013 at 04:15:28PM +0800, Chunyan Liu wrote: Set NOCOW flag to newly created images to solve performance issues on btrfs. snip This should be optional and I'm not sure it should be the default. Rationale: If you're on btrfs you probably expect the copy-on-write and snapshot features of the file system. We shouldn't silently disable that unless the user asks for it. snip When the NOCOW attribute is set on a file, reflink copying (aka file-level snapshots) do not work: $ cp --reflink test.img test-snapshot.img This produces EINVAL. It is a regression if qemu-img create suddenly starts breaking this standard btrfs feature for existing users. Please make it a .bdrv_create() option which is off by default to avoid breaking existing users' workflows/scripts. The result should be something like: $ qemu-img create test.img 8G # file has NOCOW cleared $ qemu-img create -o nocow=on test.img 8G # file has NOCOW set I agree we shouldn't break existing work flows. I wonder if it would OK for qemu-img to issue a warning (when not --quiet) when it detects creation of an image on a partition where performance may not be as expected due to COW behaviour. Cheers, -- Alex Bennée QEMU/KVM Hacker for Linaro
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
On Mon, Nov 18, 2013 at 12:54:59PM +0800, Chunyan Liu wrote:
2013/11/15 Stefan Hajnoczi stefa...@gmail.com
On Thu, Nov 14, 2013 at 04:15:28PM +0800, Chunyan Liu wrote:
Set NOCOW flag to newly created images to solve performance issues on
btrfs.
Btrfs has terrible performance when hosting VM images, even more when
the guest
in those VM are also using btrfs as file system. One way to mitigate
this bad
performance is to turn off COW attributes on VM files (since having copy
on
write for this kind of data is not useful).
Signed-off-by: Chunyan Liu cy...@suse.com
---
block/raw-posix.c |6 ++
block/vdi.c |7 +++
block/vmdk.c |7 +++
include/qemu-common.h |9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f6d48bb..4a3e9d0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1072,6 +1072,12 @@ static int raw_create(const char *filename,
QEMUOptionParameter *options,
result = -errno;
error_setg_errno(errp, -result, Could not create file);
} else {
+#ifdef __linux__
+/* set NOCOW flag to solve performance issue on fs like btrfs */
+int attr;
+attr = FS_NOCOW_FL;
+ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
This should be optional and I'm not sure it should be the default.
Rationale: If you're on btrfs you probably expect the copy-on-write and
snapshot features of the file system. We shouldn't silently disable
that unless the user asks for it.
The problem is: if users want to use copy-on-write (e.g, for snapshotting)
and
don't care about performance degrade, they still be able to issue chattr
to
change it to be COW. However, if a file is created as COW, but later users
care
about performance, there is no way to switch to NOCOW per file. NOCOW
should be
set to new or empty file only on btrfs.
When the NOCOW attribute is set on a file, reflink copying (aka
file-level snapshots) do not work:
$ cp --reflink test.img test-snapshot.img
This produces EINVAL.
It is a regression if qemu-img create suddenly starts breaking this
standard btrfs feature for existing users.
Please make it a .bdrv_create() option which is off by default to avoid
breaking existing users' workflows/scripts. The result should be
something like:
$ qemu-img create test.img 8G # file has NOCOW cleared
$ qemu-img create -o nocow=on test.img 8G # file has NOCOW set
Stefan
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
2013/11/15 Stefan Hajnoczi stefa...@gmail.com
On Thu, Nov 14, 2013 at 04:15:28PM +0800, Chunyan Liu wrote:
Set NOCOW flag to newly created images to solve performance issues on
btrfs.
Btrfs has terrible performance when hosting VM images, even more when
the guest
in those VM are also using btrfs as file system. One way to mitigate
this bad
performance is to turn off COW attributes on VM files (since having copy
on
write for this kind of data is not useful).
Signed-off-by: Chunyan Liu cy...@suse.com
---
block/raw-posix.c |6 ++
block/vdi.c |7 +++
block/vmdk.c |7 +++
include/qemu-common.h |9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f6d48bb..4a3e9d0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1072,6 +1072,12 @@ static int raw_create(const char *filename,
QEMUOptionParameter *options,
result = -errno;
error_setg_errno(errp, -result, Could not create file);
} else {
+#ifdef __linux__
+/* set NOCOW flag to solve performance issue on fs like btrfs */
+int attr;
+attr = FS_NOCOW_FL;
+ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
This should be optional and I'm not sure it should be the default.
Rationale: If you're on btrfs you probably expect the copy-on-write and
snapshot features of the file system. We shouldn't silently disable
that unless the user asks for it.
The problem is: if users want to use copy-on-write (e.g, for snapshotting)
and
don't care about performance degrade, they still be able to issue chattr
to
change it to be COW. However, if a file is created as COW, but later users
care
about performance, there is no way to switch to NOCOW per file. NOCOW
should be
set to new or empty file only on btrfs.
Chunyan
Stefan
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
On Thu, Nov 14, 2013 at 04:15:28PM +0800, Chunyan Liu wrote:
Set NOCOW flag to newly created images to solve performance issues on btrfs.
Btrfs has terrible performance when hosting VM images, even more when the
guest
in those VM are also using btrfs as file system. One way to mitigate this bad
performance is to turn off COW attributes on VM files (since having copy on
write for this kind of data is not useful).
Signed-off-by: Chunyan Liu cy...@suse.com
---
block/raw-posix.c |6 ++
block/vdi.c |7 +++
block/vmdk.c |7 +++
include/qemu-common.h |9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f6d48bb..4a3e9d0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1072,6 +1072,12 @@ static int raw_create(const char *filename,
QEMUOptionParameter *options,
result = -errno;
error_setg_errno(errp, -result, Could not create file);
} else {
+#ifdef __linux__
+/* set NOCOW flag to solve performance issue on fs like btrfs */
+int attr;
+attr = FS_NOCOW_FL;
+ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
This should be optional and I'm not sure it should be the default.
Rationale: If you're on btrfs you probably expect the copy-on-write and
snapshot features of the file system. We shouldn't silently disable
that unless the user asks for it.
Stefan
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
Am 15.11.2013 um 05:05 hat Chunyan Liu geschrieben:
2013/11/14 Kevin Wolf kw...@redhat.com
Am 14.11.2013 um 09:15 hat Chunyan Liu geschrieben:
Set NOCOW flag to newly created images to solve performance issues on
btrfs.
Btrfs has terrible performance when hosting VM images, even more when
the
guest
in those VM are also using btrfs as file system. One way to mitigate
this
bad
performance is to turn off COW attributes on VM files (since having copy
on
write for this kind of data is not useful).
Signed-off-by: Chunyan Liu cy...@suse.com
---
block/raw-posix.c | 6 ++
block/vdi.c | 7 +++
block/vmdk.c | 7 +++
include/qemu-common.h | 9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f6d48bb..4a3e9d0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1072,6 +1072,12 @@ static int raw_create(const char *filename,
QEMUOptionParameter *options,
result = -errno;
error_setg_errno(errp, -result, Could not create file);
} else {
+#ifdef __linux__
+ /* set NOCOW flag to solve performance issue on fs like btrfs
*/
+ int attr;
+ attr = FS_NOCOW_FL;
+ ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
ioctl() returning an error is ignored. This is probably okay because
we're only talking about an optimisation here. Perhaps worth a word or
two in the comment.
However, while this ioctl is setting FS_NOCOW_FL, it is at the same time
clearing all other flags. This doesn't look right.
Yes, strictly it should be GETFLAGS and then SETFLAGS. Here because it does
FS_IOC_SETFLAGS right after creating the file, and checking the qemu_open()
parameter, in fact no FLAGS has been set, so just setting FS_NOCOW_FL
directly.
I can revise that if it's not good.
Is there a guarantee that even just creating the file doesn't set any
flags? For example, while reading up on this, I saw that there's a
nodatacow mount option, which means exactly that FS_NOCOW_FL gets set
for new files. If any similar mechanism existed for other flags (or was
to be introduced in future kernel versions), we would break it here.
So, I'd prefer to play it safe and use GETFLAGS first.
Kevin
[Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
Set NOCOW flag to newly created images to solve performance issues on btrfs.
Btrfs has terrible performance when hosting VM images, even more when the guest
in those VM are also using btrfs as file system. One way to mitigate this bad
performance is to turn off COW attributes on VM files (since having copy on
write for this kind of data is not useful).
Signed-off-by: Chunyan Liu cy...@suse.com
---
block/raw-posix.c |6 ++
block/vdi.c |7 +++
block/vmdk.c |7 +++
include/qemu-common.h |9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f6d48bb..4a3e9d0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1072,6 +1072,12 @@ static int raw_create(const char *filename,
QEMUOptionParameter *options,
result = -errno;
error_setg_errno(errp, -result, Could not create file);
} else {
+#ifdef __linux__
+/* set NOCOW flag to solve performance issue on fs like btrfs */
+int attr;
+attr = FS_NOCOW_FL;
+ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
if (ftruncate(fd, total_size * BDRV_SECTOR_SIZE) != 0) {
result = -errno;
error_setg_errno(errp, -result, Could not resize file);
diff --git a/block/vdi.c b/block/vdi.c
index b6ec002..dfaa905 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -686,6 +686,13 @@ static int vdi_create(const char *filename,
QEMUOptionParameter *options,
0644);
if (fd 0) {
return -errno;
+} else {
+#ifdef __linux__
+/* set NOCOW flag to solve performance issue on fs like btrfs */
+int attr;
+attr = FS_NOCOW_FL;
+ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
}
/* We need enough blocks to store the given disk size,
diff --git a/block/vmdk.c b/block/vmdk.c
index a7ebd0f..bd953bc 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1447,6 +1447,13 @@ static int vmdk_create_extent(const char *filename,
int64_t filesize,
0644);
if (fd 0) {
return -errno;
+} else {
+#ifdef __linux__
+/* set NOCOW flag to solve performance issue on fs like btrfs */
+int attr;
+attr = FS_NOCOW_FL;
+ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
}
if (flat) {
ret = ftruncate(fd, filesize);
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 5054836..fe7dd9b 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -50,6 +50,15 @@
#include sysemu/os-posix.h
#endif
+#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
+
#ifndef O_LARGEFILE
#define O_LARGEFILE 0
#endif
--
1.6.0.2
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
Am 14.11.2013 um 09:15 hat Chunyan Liu geschrieben:
Set NOCOW flag to newly created images to solve performance issues on btrfs.
Btrfs has terrible performance when hosting VM images, even more when the
guest
in those VM are also using btrfs as file system. One way to mitigate this bad
performance is to turn off COW attributes on VM files (since having copy on
write for this kind of data is not useful).
Signed-off-by: Chunyan Liu cy...@suse.com
---
block/raw-posix.c |6 ++
block/vdi.c |7 +++
block/vmdk.c |7 +++
include/qemu-common.h |9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f6d48bb..4a3e9d0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1072,6 +1072,12 @@ static int raw_create(const char *filename,
QEMUOptionParameter *options,
result = -errno;
error_setg_errno(errp, -result, Could not create file);
} else {
+#ifdef __linux__
+/* set NOCOW flag to solve performance issue on fs like btrfs */
+int attr;
+attr = FS_NOCOW_FL;
+ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
ioctl() returning an error is ignored. This is probably okay because
we're only talking about an optimisation here. Perhaps worth a word or
two in the comment.
However, while this ioctl is setting FS_NOCOW_FL, it is at the same time
clearing all other flags. This doesn't look right.
Kevin
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
cy...@suse.com writes: Set NOCOW flag to newly created images to solve performance issues on btrfs. Btrfs has terrible performance when hosting VM images, even more when the guest in those VM are also using btrfs as file system. One way to mitigate this bad performance is to turn off COW attributes on VM files (since having copy on write for this kind of data is not useful). snip It's been a while since I had to mess with this performance mystery but I recall you also need to ensure the partition needs to be mounted with the nodatacow mount option. Unless this has changed we should probably warn the user about that. -- Alex Bennée
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
Am 14.11.2013 um 10:44 hat Alex Bennée geschrieben: cy...@suse.com writes: Set NOCOW flag to newly created images to solve performance issues on btrfs. Btrfs has terrible performance when hosting VM images, even more when the guest in those VM are also using btrfs as file system. One way to mitigate this bad performance is to turn off COW attributes on VM files (since having copy on write for this kind of data is not useful). snip It's been a while since I had to mess with this performance mystery but I recall you also need to ensure the partition needs to be mounted with the nodatacow mount option. Unless this has changed we should probably warn the user about that. From the information I read today while looking at this patch, the nodatacow mount option seems just to influence the default for newly created files. That is, if you set the flag like this patch is doing, it has the same effect as running qemu-img create while having mounted the file system with nodatacow. Never tried it on my own, though, so take it with a grain of salt. Kevin
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
--On 14 November 2013 10:17:26 +0100 Kevin Wolf kw...@redhat.com wrote: +#ifdef __linux__ +/* set NOCOW flag to solve performance issue on fs like btrfs */ +int attr; +attr = FS_NOCOW_FL; +ioctl(fd, FS_IOC_SETFLAGS, attr); +#endif ioctl() returning an error is ignored. This is probably okay because we're only talking about an optimisation here. Perhaps worth a word or two in the comment. However, while this ioctl is setting FS_NOCOW_FL, it is at the same time clearing all other flags. This doesn't look right. Also, given FS_NOCOW_FL was only introduced in 2.6.39, should this not be guarded by #ifdef FS_NOCOW_FL (or better tested in configure in case it becomes something other than a #define in which case this test could replace #ifdef __linux__) -- Alex Bligh
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
--On 14 November 2013 14:23:29 + Alex Bligh a...@alex.org.uk wrote: Also, given FS_NOCOW_FL was only introduced in 2.6.39, should this not be guarded by #ifdef FS_NOCOW_FL (or better tested in configure in case it becomes something other than a #define in which case this test could replace #ifdef __linux__) Apologies - I missed this hunk: diff --git a/include/qemu-common.h b/include/qemu-common.h index 5054836..fe7dd9b 100644 --- a/include/qemu-common.h +++ b/include/qemu-common.h @@ -50,6 +50,15 @@ #include sysemu/os-posix.h #endif +#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 + #ifndef O_LARGEFILE #define O_LARGEFILE 0 #endif -- Alex Bligh
Re: [Qemu-devel] [PATCH] qemu-img: set nocow flag to new file
2013/11/14 Kevin Wolf kw...@redhat.com
Am 14.11.2013 um 09:15 hat Chunyan Liu geschrieben:
Set NOCOW flag to newly created images to solve performance issues on
btrfs.
Btrfs has terrible performance when hosting VM images, even more when
the guest
in those VM are also using btrfs as file system. One way to mitigate
this bad
performance is to turn off COW attributes on VM files (since having copy
on
write for this kind of data is not useful).
Signed-off-by: Chunyan Liu cy...@suse.com
---
block/raw-posix.c |6 ++
block/vdi.c |7 +++
block/vmdk.c |7 +++
include/qemu-common.h |9 +
4 files changed, 29 insertions(+), 0 deletions(-)
diff --git a/block/raw-posix.c b/block/raw-posix.c
index f6d48bb..4a3e9d0 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1072,6 +1072,12 @@ static int raw_create(const char *filename,
QEMUOptionParameter *options,
result = -errno;
error_setg_errno(errp, -result, Could not create file);
} else {
+#ifdef __linux__
+/* set NOCOW flag to solve performance issue on fs like btrfs */
+int attr;
+attr = FS_NOCOW_FL;
+ioctl(fd, FS_IOC_SETFLAGS, attr);
+#endif
ioctl() returning an error is ignored. This is probably okay because
we're only talking about an optimisation here. Perhaps worth a word or
two in the comment.
However, while this ioctl is setting FS_NOCOW_FL, it is at the same time
clearing all other flags. This doesn't look right.
Yes, strictly it should be GETFLAGS and then SETFLAGS. Here because it does
FS_IOC_SETFLAGS right after creating the file, and checking the qemu_open()
parameter, in fact no FLAGS has been set, so just setting FS_NOCOW_FL
directly.
I can revise that if it's not good.
Kevin
