[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of F
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP |
Date: |
Thu, 13 Nov 2014 13:38:56 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
Kevin Wolf <address@hidden> writes:
> Am 13.11.2014 um 12:45 hat Max Reitz geschrieben:
>> On 2014-11-13 at 12:40, Kevin Wolf wrote:
>> >Am 13.11.2014 um 00:25 hat Eric Blake geschrieben:
>> >>On 11/12/2014 01:27 PM, Markus Armbruster wrote:
>> >>>+ /* in hole, end not yet known */
>> >>>+ offs = lseek(s->fd, start, SEEK_DATA);
>> >>>+ if (offs < 0) {
>> >>>+ /* no idea where the hole ends, give up (unlikely to happen) */
>> >>>+ goto dunno;
>> >>>+ }
>> >>>+ assert(offs >= start);
>> >>>+ *hole = start;
>> >>>+ *data = offs;
>> >>This assertion feels like an off-by-one. The same offset cannot be both
>> >>a hole and data (except in some racy situation where some other process
>> >>is writing data to that offset in between our two lseek calls, but
>> >>that's already in no-man's land because no one else should be writing
>> >>the file while qemu has it open). Is it worth using 'assert(offs >
>> >>start)' instead?
>> >As soon as you say "except", it's wrong to assert this at all. We can't
>> >guarantee that the condition is true and it's not a programming error
>> >in qemu if it's false. Sounds to me as if it should be a normal error
>> >check rather than an assertion.
You're right, it's not necessarily a programming error, it could also be
caused by another process filling in holes behind our back. We need to
handle == some other way. We could start over, but I figure return
-EBUSY is simpler and good enough for this corner case.
>> >Also, what happens after EOF? I haven't read the patch yet, maybe it
>> >handles the situation already earlier, but if it doesn't, won't we get
>> >offset == start then?
>>
>> raw_co_get_block_status() already bails out if start is at or beyond EOF.
>
> Okay, so that's basically the same "except" as above.
>
> Except that the window for the race is much larger because the
> raw_co_get_block_status() check uses the cached value, so any file size
> change in the background after qemu has opened the image would trigger
> an assertion failure.
Bails out like this:
total_size = bdrv_getlength(bs);
if (total_size < 0) {
return total_size;
Can't actually happen, because bdrv_nb_sectors() can fail only if
!bs->drv (surely false here), or drv->has_variable_length (also false
here).
} else if (start >= total_size) {
*pnum = 0;
return 0;
If something else has lengthened the file, we simply refuse to notice.
} else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
}
Likewise.
- [Qemu-devel] [PATCH 0/2] raw-posix: Get rid of FIEMAP, Markus Armbruster, 2014/11/12
- [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Markus Armbruster, 2014/11/12
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Fam Zheng, 2014/11/12
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Eric Blake, 2014/11/12
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Markus Armbruster, 2014/11/13
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Kevin Wolf, 2014/11/13
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Max Reitz, 2014/11/13
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Kevin Wolf, 2014/11/13
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Max Reitz, 2014/11/13
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP,
Markus Armbruster <=
- Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Kevin Wolf, 2014/11/13
Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP, Max Reitz, 2014/11/13
[Qemu-devel] [PATCH 1/2] raw-posix: Fix comment for raw_co_get_block_status(), Markus Armbruster, 2014/11/12
Re: [Qemu-devel] [PATCH 0/2] raw-posix: Get rid of FIEMAP, Paolo Bonzini, 2014/11/12