[Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Tony Breeds]

The command
  qemu-img convert -O raw inputimage.qcow2 outputimage.raw

intermittently creates corrupted output images, when the input image is not yet fully synchronized to disk.  This patch preferese the use of seek_hole checks to determine if the sector is present in the disk image.

While we're there modify try_fiemap() to set FIEMAP_FLAG_SYNC.

Reported-By: Michael Steffens <michael_steffens@posteo.de>
CC: Pádraig Brady <pbrady@redhat.com>
Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
---
 block/raw-posix.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a253697..b438c54 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1472,7 +1472,7 @@ static int64_t try_fiemap(BlockDriverState *bs, off_t start, off_t *data,
 
     f.fm.fm_start = start;
     f.fm.fm_length = (int64_t)nb_sectors * BDRV_SECTOR_SIZE;
-    f.fm.fm_flags = 0;
+    f.fm.fm_flags = FIEMAP_FLAG_SYNC;
     f.fm.fm_extent_count = 1;
     f.fm.fm_reserved = 0;
     if (ioctl(s->fd, FS_IOC_FIEMAP, &f) == -1) {
@@ -1561,9 +1561,9 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
 
     start = sector_num * BDRV_SECTOR_SIZE;
 
-    ret = try_fiemap(bs, start, &data, &hole, nb_sectors, pnum);
+    ret = try_seek_hole(bs, start, &data, &hole, pnum);
     if (ret < 0) {
-        ret = try_seek_hole(bs, start, &data, &hole, pnum);
+        ret = try_fiemap(bs, start, &data, &hole, nb_sectors, pnum);
         if (ret < 0) {
             /* Assume everything is allocated. */
             data = 0;
-- 
1.9.3

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Markus Armbruster]

Please copy Kevin & Stefan for block patches.  Doing that for you.  I
also copy Max, who left his fingerprints on commit 4f11aa8.

Tony Breeds <tony@bakeyournoodle.com> writes:

> The command
>   qemu-img convert -O raw inputimage.qcow2 outputimage.raw
>
> intermittently creates corrupted output images, when the input image is not yet fully synchronized to disk.  This patch preferese the use of seek_hole checks to determine if the sector is present in the disk image.
>
> While we're there modify try_fiemap() to set FIEMAP_FLAG_SYNC.
>
> Reported-By: Michael Steffens <michael_steffens@posteo.de>
> CC: Pádraig Brady <pbrady@redhat.com>
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> ---
>  block/raw-posix.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index a253697..b438c54 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1472,7 +1472,7 @@ static int64_t try_fiemap(BlockDriverState *bs, off_t start, off_t *data,
>  
>      f.fm.fm_start = start;
>      f.fm.fm_length = (int64_t)nb_sectors * BDRV_SECTOR_SIZE;
> -    f.fm.fm_flags = 0;
> +    f.fm.fm_flags = FIEMAP_FLAG_SYNC;
>      f.fm.fm_extent_count = 1;
>      f.fm.fm_reserved = 0;
>      if (ioctl(s->fd, FS_IOC_FIEMAP, &f) == -1) {
> @@ -1561,9 +1561,9 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
>  
>      start = sector_num * BDRV_SECTOR_SIZE;
>  
> -    ret = try_fiemap(bs, start, &data, &hole, nb_sectors, pnum);
> +    ret = try_seek_hole(bs, start, &data, &hole, pnum);
>      if (ret < 0) {
> -        ret = try_seek_hole(bs, start, &data, &hole, pnum);
> +        ret = try_fiemap(bs, start, &data, &hole, nb_sectors, pnum);
>          if (ret < 0) {
>              /* Assume everything is allocated. */
>              data = 0;

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 264 bytes --]

On Thu, Sep 25, 2014 at 08:21:27AM +0200, Markus Armbruster wrote:
> Please copy Kevin & Stefan for block patches.  Doing that for you.  I
> also copy Max, who left his fingerprints on commit 4f11aa8.

Sorry.

Is there a qemu version of get_maintainers.pl?

Tony.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Greg Kurz]

On Thu, 25 Sep 2014 16:29:31 +1000
Tony Breeds <tony@bakeyournoodle.com> wrote:

> On Thu, Sep 25, 2014 at 08:21:27AM +0200, Markus Armbruster wrote:
> > Please copy Kevin & Stefan for block patches.  Doing that for you.  I
> > also copy Max, who left his fingerprints on commit 4f11aa8.
> 
> Sorry.
> 
> Is there a qemu version of get_maintainers.pl?
> 
> Tony.

Hi Tony,

Same path : ./scripts/get_maintainer.pl

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Markus Armbruster]

Tony Breeds <tony@bakeyournoodle.com> writes:

> On Thu, Sep 25, 2014 at 08:21:27AM +0200, Markus Armbruster wrote:
>> Please copy Kevin & Stefan for block patches.  Doing that for you.  I
>> also copy Max, who left his fingerprints on commit 4f11aa8.
>
> Sorry.
>
> Is there a qemu version of get_maintainers.pl?

Yes :)

$ scripts/get_maintainer.pl $your_msg
Kevin Wolf <kwolf@redhat.com> (supporter:Block)
Stefan Hajnoczi <stefanha@redhat.com> (supporter:Block)

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 138 bytes --]

On Thu, Sep 25, 2014 at 08:42:25AM +0200, Greg Kurz wrote:

> Same path : ./scripts/get_maintainer.pl

Thanks Greg.  Now I know :)

Tony.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Kevin Wolf]

Am 25.09.2014 um 08:21 hat Markus Armbruster geschrieben:
> Please copy Kevin & Stefan for block patches.  Doing that for you.  I
> also copy Max, who left his fingerprints on commit 4f11aa8.
> 
> Tony Breeds <tony@bakeyournoodle.com> writes:
> 
> > The command
> >   qemu-img convert -O raw inputimage.qcow2 outputimage.raw
> >
> > intermittently creates corrupted output images, when the input image is not yet fully synchronized to disk.  This patch preferese the use of seek_hole checks to determine if the sector is present in the disk image.

Does this fix the problem or does it just make it less likely that it
becomes apparent?

If there is a data corruptor, we need to fix it, not just ensure that
only the less common environments are affected.

> > While we're there modify try_fiemap() to set FIEMAP_FLAG_SYNC.

That looks like a logically separate change, so it should probably be
a separate patch.

Is this fix for the corruptor? The commit message doesn't make it
clear. If so and fiemap is safe now, why would we still prefer
seek_hole?

> > Reported-By: Michael Steffens <michael_steffens@posteo.de>
> > CC: Pádraig Brady <pbrady@redhat.com>
> > Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>

Kevin

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

On Thu, Sep 25, 2014 at 09:30:30AM +0200, Kevin Wolf wrote:

> Does this fix the problem or does it just make it less likely that it
> becomes apparent?

Sorry for not making this clearer in my commit message.

I haven't been able to reproduce the corruption with the fiemap flag
change.
 
> If there is a data corruptor, we need to fix it, not just ensure that
> only the less common environments are affected.

I agree. I believe that the FIEMAP_FLAG_SYNC flag change fixes the
corrupter and then, as you say, makes that code less commonly executed.
 
> That looks like a logically separate change, so it should probably be
> a separate patch.

Sure I can do that, and be more explicit about the reason in the commit
message.
 
> Is this fix for the corruptor? The commit message doesn't make it
> clear. If so and fiemap is safe now, why would we still prefer
> seek_hole?

The preference for seek_hole was a suggestion from Pádraig Brady , so
I'll let him defend that :) but as I said above I think it was about 
reducing the situations where fiemap was/is called.

Tony.

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1974 bytes --]

Am 25.09.2014 um 11:00 hat Tony Breeds geschrieben:
> On Thu, Sep 25, 2014 at 09:30:30AM +0200, Kevin Wolf wrote:
> 
> > Does this fix the problem or does it just make it less likely that it
> > becomes apparent?
> 
> Sorry for not making this clearer in my commit message.
> 
> I haven't been able to reproduce the corruption with the fiemap flag
> change.
>  
> > If there is a data corruptor, we need to fix it, not just ensure that
> > only the less common environments are affected.
> 
> I agree. I believe that the FIEMAP_FLAG_SYNC flag change fixes the
> corrupter and then, as you say, makes that code less commonly executed.
>  
> > That looks like a logically separate change, so it should probably be
> > a separate patch.
> 
> Sure I can do that, and be more explicit about the reason in the commit
> message.
>  
> > Is this fix for the corruptor? The commit message doesn't make it
> > clear. If so and fiemap is safe now, why would we still prefer
> > seek_hole?
> 
> The preference for seek_hole was a suggestion from Pádraig Brady , so
> I'll let him defend that :) but as I said above I think it was about 
> reducing the situations where fiemap was/is called.

Okay. I'm not opposed to the change, we just need to split and document
it properly.

So first we should fix the bug by adding FIEMAP_FLAG_SYNC. Or actually,
it might just be a workaround for a kernel bug. Is the expected
behaviour documented, and if yes, what is it? We should describe as
precisely as possible what the situation is and why we have to add this
flag in qemu. You can probably reuse a good part of your current commit
message for that and extend it a bit.

And then we may or may not make the second change with the preference
for seek_hole. If FIEMAP_FLAG_SYNC actually does additional writes or an
fsync(), switching is a performance optimisation and might be justified
as such. Maybe Pádraig has some more input on this.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Pádraig Brady]

On 09/25/2014 10:00 AM, Tony Breeds wrote:
> On Thu, Sep 25, 2014 at 09:30:30AM +0200, Kevin Wolf wrote:
> 
>> Does this fix the problem or does it just make it less likely that it
>> becomes apparent?
> 
> Sorry for not making this clearer in my commit message.
> 
> I haven't been able to reproduce the corruption with the fiemap flag
> change.
>  
>> If there is a data corruptor, we need to fix it, not just ensure that
>> only the less common environments are affected.
> 
> I agree. I believe that the FIEMAP_FLAG_SYNC flag change fixes the
> corrupter and then, as you say, makes that code less commonly executed.
>  
>> That looks like a logically separate change, so it should probably be
>> a separate patch.
> 
> Sure I can do that, and be more explicit about the reason in the commit
> message.
>  
>> Is this fix for the corruptor? The commit message doesn't make it
>> clear. If so and fiemap is safe now, why would we still prefer
>> seek_hole?

syncing a file has performance side effects,
so best go try the (more portable) seek hole interface.

> The preference for seek_hole was a suggestion from Pádraig Brady , so
> I'll let him defend that :) but as I said above I think it was about 
> reducing the situations where fiemap was/is called.

IMHO it's a kernel bug that fiemap() needs a sync to be useful.
This was being fixed up in various file systems but Dave Chiner
pushed back on XFS adjustments, and thus changes were stalled.
The current situation is it's best to avoid fiemap().

thanks,
Pádraig.

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

On 09/24/2014 11:23 PM, Tony Breeds wrote:
> The command
>   qemu-img convert -O raw inputimage.qcow2 outputimage.raw
> 
> intermittently creates corrupted output images, when the input image
is not yet fully synchronized to disk.  This patch preferese the use of
seek_hole checks to determine if the sector is present in the disk image.

s/preferese/prefers/

Please wrap your commit messages to fit in 70 or so columns (so that git
log, which indents messages, is still legible in an 80-column window).

> 
> While we're there modify try_fiemap() to set FIEMAP_FLAG_SYNC.
> 
> Reported-By: Michael Steffens <michael_steffens@posteo.de>
> CC: Pádraig Brady <pbrady@redhat.com>
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> ---
>  block/raw-posix.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Adding Kevin and Stefan in cc (per scripts/get_maintainer.pl).
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH] block/raw-posix: use seek_hole ahead of fiemap

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

On 09/25/2014 01:30 AM, Kevin Wolf wrote:
> Am 25.09.2014 um 08:21 hat Markus Armbruster geschrieben:
>> Please copy Kevin & Stefan for block patches.  Doing that for you.  I
>> also copy Max, who left his fingerprints on commit 4f11aa8.
>>
>> Tony Breeds <tony@bakeyournoodle.com> writes:
>>
>>> The command
>>>   qemu-img convert -O raw inputimage.qcow2 outputimage.raw
>>>
>>> intermittently creates corrupted output images, when the input image is not yet fully synchronized to disk.  This patch preferese the use of seek_hole checks to determine if the sector is present in the disk image.
> 
> Does this fix the problem or does it just make it less likely that it
> becomes apparent?
> 
> If there is a data corruptor, we need to fix it, not just ensure that
> only the less common environments are affected.
> 
>>> While we're there modify try_fiemap() to set FIEMAP_FLAG_SYNC.
> 
> That looks like a logically separate change, so it should probably be
> a separate patch.
> 
> Is this fix for the corruptor? The commit message doesn't make it
> clear. If so and fiemap is safe now, why would we still prefer
> seek_hole?

My understanding, based on what coreutils learned:

fiemap without FIEMAP_FLAG_SYNC is a data corrupter.  fiemap _with_
FIEMAP_FLAG_SYNC is a performance killer (noticeably slower, because it
forces a sync).  SEEK_HOLE is a much simpler interface, and therefore
the kernel can optimize it to return correct results faster than it can
for fiemap, and it does not suffer from the fiemap on unsynced file data
corruption.  So coreutils _prefers_ seek_hole first, and when it has to
use fiemap, prefers using fiemap with FIEMAP_FLAG_SYNC.

I agree with splitting this into two patches; one to add the flag as a
data corruption fix but acknowledging that it slows fiemap down, the
other to relegate fiemap to the fallback case since seek_hole can be
faster when it doesn't have to force a sync.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

[Qemu-devel] [PATCH v2 1/2] block/raw-posix: Fix disk corruption in try_fiemap

[Tony Breeds]

Using fiemap without FIEMAP_FLAG_SYNC is a known corrupter.

Add the FIEMAP_FLAG_SYNC flag to the FS_IOC_FIEMAP ioctl.  This has
the downside of significantly reducing performance.

Reported-By: Michael Steffens <michael_steffens@posteo.de>
Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Pádraig Brady <pbrady@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
---
Changes since v1:
 - split in to 2 patches
 - tried to make the commit messages better

 block/raw-posix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a253697..b8203e9 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1472,7 +1472,7 @@ static int64_t try_fiemap(BlockDriverState *bs, off_t start, off_t *data,
 
     f.fm.fm_start = start;
     f.fm.fm_length = (int64_t)nb_sectors * BDRV_SECTOR_SIZE;
-    f.fm.fm_flags = 0;
+    f.fm.fm_flags = FIEMAP_FLAG_SYNC;
     f.fm.fm_extent_count = 1;
     f.fm.fm_reserved = 0;
     if (ioctl(s->fd, FS_IOC_FIEMAP, &f) == -1) {
-- 
1.9.3

[Qemu-devel] [PATCH v2 2/2] block/raw-posix: use seek_hole ahead of fiemap

[Tony Breeds]

try_fiemap() uses FIEMAP_FLAG_SYNC which has a significant performance
impact.

Prefer seek_hole() over fiemap() to avoid this impact where possible.
seek_hole is more widely used and, arguably, has potential to be
optimised in the kernel.

Reported-By: Michael Steffens <michael_steffens@posteo.de>
Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: Pádraig Brady <pbrady@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
---
Changes since v1:
 - split in to 2 patches
 - tried to make the commit messages better

 block/raw-posix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index b8203e9..b438c54 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1561,9 +1561,9 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs,
 
     start = sector_num * BDRV_SECTOR_SIZE;
 
-    ret = try_fiemap(bs, start, &data, &hole, nb_sectors, pnum);
+    ret = try_seek_hole(bs, start, &data, &hole, pnum);
     if (ret < 0) {
-        ret = try_seek_hole(bs, start, &data, &hole, pnum);
+        ret = try_fiemap(bs, start, &data, &hole, nb_sectors, pnum);
         if (ret < 0) {
             /* Assume everything is allocated. */
             data = 0;
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 2/2] block/raw-posix: use seek_hole ahead of fiemap

[Max Reitz]

On 26.09.2014 01:14, Tony Breeds wrote:
> try_fiemap() uses FIEMAP_FLAG_SYNC which has a significant performance
> impact.
>
> Prefer seek_hole() over fiemap() to avoid this impact where possible.
> seek_hole is more widely used and, arguably, has potential to be
> optimised in the kernel.
>
> Reported-By: Michael Steffens <michael_steffens@posteo.de>
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Pádraig Brady <pbrady@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> Changes since v1:
>   - split in to 2 patches
>   - tried to make the commit messages better

I used this order in at least one of the non-final versions of 4f11aa8a, 
so I'm fine with it. The reason it got rejected was (as far as I 
remember) that the FIEMAP ioctl() returns ENOTSUP if not supported, 
whereas lseek() with SEEK_HOLE/SEEK_DATA returns some failsafe value, 
probably reporting no holes at all. By reversing the order of 
try_fiemap() and try_seek_hole(), try_fiemap() therefore may very well 
become dead code. I don't object, however.

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/2] block/raw-posix: Fix disk corruption in try_fiemap

[Max Reitz]

On 26.09.2014 01:14, Tony Breeds wrote:
> Using fiemap without FIEMAP_FLAG_SYNC is a known corrupter.
>
> Add the FIEMAP_FLAG_SYNC flag to the FS_IOC_FIEMAP ioctl.  This has
> the downside of significantly reducing performance.
>
> Reported-By: Michael Steffens <michael_steffens@posteo.de>
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Pádraig Brady <pbrady@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> Changes since v1:
>   - split in to 2 patches
>   - tried to make the commit messages better
>
>   block/raw-posix.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/2] block/raw-posix: Fix disk corruption in try_fiemap

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On 09/25/2014 05:14 PM, Tony Breeds wrote:
> Using fiemap without FIEMAP_FLAG_SYNC is a known corrupter.
> 
> Add the FIEMAP_FLAG_SYNC flag to the FS_IOC_FIEMAP ioctl.  This has
> the downside of significantly reducing performance.
> 
> Reported-By: Michael Steffens <michael_steffens@posteo.de>
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Pádraig Brady <pbrady@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> Changes since v1:
>  - split in to 2 patches
>  - tried to make the commit messages better
> 
>  block/raw-posix.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

[you buried your patches in deep reply to a longer thread, making them
harder to find; in the future, you can just send a v2 as a new top-level
thread]

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 2/2] block/raw-posix: use seek_hole ahead of fiemap

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

On 09/25/2014 05:14 PM, Tony Breeds wrote:
> try_fiemap() uses FIEMAP_FLAG_SYNC which has a significant performance
> impact.
> 
> Prefer seek_hole() over fiemap() to avoid this impact where possible.
> seek_hole is more widely used and, arguably, has potential to be
> optimised in the kernel.

It also has the potential to be less precise than fiemap on older
kernels (any filesystem that supports fiemap but has not yet wired up
seek_hole will get the kernel's default fallback of claiming no holes at
all), but that problem will fade as more people use modern kernels.
This matches what coreutils does, so I like it on that grounds.

> 
> Reported-By: Michael Steffens <michael_steffens@posteo.de>
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Pádraig Brady <pbrady@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> Changes since v1:
>  - split in to 2 patches
>  - tried to make the commit messages better

Thanks; it is indeed better.

> 
>  block/raw-posix.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] block/raw-posix: Fix disk corruption in try_fiemap

[Tony Breeds]

[-- Attachment #1: Type: text/plain, Size: 267 bytes --]

On Fri, Sep 26, 2014 at 11:09:30AM -0600, Eric Blake wrote:

> [you buried your patches in deep reply to a longer thread, making them
> harder to find; in the future, you can just send a v2 as a new top-level
> thread]

Thanks.  I'll remember that next time.

Tony.


[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] block/raw-posix: Fix disk corruption in try_fiemap

[Kevin Wolf]

Am 26.09.2014 um 01:14 hat Tony Breeds geschrieben:
> Using fiemap without FIEMAP_FLAG_SYNC is a known corrupter.
> 
> Add the FIEMAP_FLAG_SYNC flag to the FS_IOC_FIEMAP ioctl.  This has
> the downside of significantly reducing performance.
> 
> Reported-By: Michael Steffens <michael_steffens@posteo.de>
> Signed-off-by: Tony Breeds <tony@bakeyournoodle.com>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: Pádraig Brady <pbrady@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> ---
> Changes since v1:
>  - split in to 2 patches
>  - tried to make the commit messages better

Thanks, applied to the block branch.

Kevin