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

[Kevin Wolf <kwolf@redhat.com>]

[-- 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 --]