Re: [Qemu-devel] [PATCH 2/2] raw-posix: SEEK_HOLE suffices, get rid of FIEMAP

[Kevin Wolf]

Am 13.11.2014 um 13:38 hat Markus Armbruster geschrieben:
> 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.

If something has shortened the file outside of qemu, total_size still
has the old larger size and we don't restrict nb_sectors to an area
before EOF.

I was however confused about the lseek() behaviour after EOF and assumed
that we would get offs == start and an assertion failure. In fact,
however, we get -ENXIO, which is probably good enough for this case.

So with your code, the problem only exists for external modification
between our two lseek() calls, not for any resize after opening the
image.

Kevin