| Message ID | 1415820422-17796-3-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/12/2014 01:27 PM, Markus Armbruster wrote: > Commit 5500316 (May 2012) implemented raw_co_is_allocated() as > follows: > > Thus, the FIEMAP code executes rarely. Makes it a nice hidey-hole for > bugs. Worse, bugs hiding there can theoretically bite even on a host > that has SEEK_HOLE/SEEK_DATA. > > I don't want to worry about this crap, not even theoretically. Get > rid of it, then clean up the mess, including spotty error checking. Sounds reasonable to me. It's rather a big patch (both nuking a bad interface and rewriting the use of the good interface) that might have been better as two commits, but I can live with it. > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/raw-posix.c | 128 ++++++++++++++++++++---------------------------------- > 1 file changed, 47 insertions(+), 81 deletions(-) > > +/* > + * Find allocation range in @bs around offset @start. > + * If @start is in a hole, store @start in @hole and the end of the > + * hole in @data. > + * If @start is in a data, store @start to @data, and the end of the > + * data to @hole. > + * If we can't find out, pretend there are no holes. > + */ > +static void find_allocation(BlockDriverState *bs, off_t start, > + off_t *data, off_t *hole) Sounds like a good contract interface. > + /* 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? > + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > + find_allocation(bs, start, &data, &hole); > + if (data == start) { > /* On a data extent, compute sectors to the end of the extent. */ > *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE); > - return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > } else { > /* On a hole, compute sectors to the beginning of the next extent. */ > + assert(hole == start); > *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE); > - return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start; > + ret |= BDRV_BLOCK_ZERO; > } > + return ret; The old code omits BDRV_BLOCK_DATA on a hole. Why are you adding it here, and why are you not mentioning it in the commit message?
On Wed, 11/12 20:27, Markus Armbruster wrote: > Commit 5500316 (May 2012) implemented raw_co_is_allocated() as > follows: > > 1. If defined(CONFIG_FIEMAP), use the FS_IOC_FIEMAP ioctl > > 2. Else if defined(SEEK_HOLE) && defined(SEEK_DATA), use lseek() > > 3. Else pretend there are no holes > > Later on, raw_co_is_allocated() was generalized to > raw_co_get_block_status(). > > Commit 4f11aa8 (May 2014) changed it to try the three methods in order > until success, because "there may be implementations which support > [SEEK_HOLE/SEEK_DATA] but not [FIEMAP] (e.g., NFSv4.2) as well as vice > versa." > > Unfortunately, we used FIEMAP incorrectly: we lacked FIEMAP_FLAG_SYNC. > Commit 38c4d0a (Sep 2014) added it. Because that's a significant > speed hit, the next commit 38c4d0a put SEEK_HOLE/SEEK_DATA first. s/38c4d0a/7c159037/ > > As you see, the obvious use of FIEMAP is wrong, and the correct use is > slow. I guess this puts it somewhere between -7 "The obvious use is > wrong" and -10 "It's impossible to get right" on Rusty Russel's Hard > to Misuse scale[*]. Nice reading :) > > "Fortunately", the FIEMAP code is used only when > > * SEEK_HOLE/SEEK_DATA arent't defined, but CONFIG_FIEMAP is > > Uncommon. SEEK_HOLE had no XFS implementation between 2011 (when it > was introduced for ext4 and btrfs) and 2012. > > * SEEK_HOLE/SEEK_DATA and CONFIG_FIEMAP are defined, but lseek() fails > > Unlikely. > > Thus, the FIEMAP code executes rarely. Makes it a nice hidey-hole for > bugs. Worse, bugs hiding there can theoretically bite even on a host > that has SEEK_HOLE/SEEK_DATA. > > I don't want to worry about this crap, not even theoretically. Get > rid of it, then clean up the mess, including spotty error checking. > > [*] http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/raw-posix.c | 128 ++++++++++++++++++++---------------------------------- > 1 file changed, 47 insertions(+), 81 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 706d3c0..d16764c 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -60,9 +60,6 @@ > #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ > #endif > #endif > -#ifdef CONFIG_FIEMAP > -#include <linux/fiemap.h> > -#endif > #ifdef CONFIG_FALLOCATE_PUNCH_HOLE > #include <linux/falloc.h> > #endif > @@ -1481,77 +1478,56 @@ out: > return result; > } > > -static int try_fiemap(BlockDriverState *bs, off_t start, off_t *data, > - off_t *hole, int nb_sectors) > +/* > + * Find allocation range in @bs around offset @start. > + * If @start is in a hole, store @start in @hole and the end of the > + * hole in @data. > + * If @start is in a data, store @start to @data, and the end of the > + * data to @hole. > + * If we can't find out, pretend there are no holes. > + */ > +static void find_allocation(BlockDriverState *bs, off_t start, > + off_t *data, off_t *hole) > { > -#ifdef CONFIG_FIEMAP > +#if defined(SEEK_DATA) && defined(SEEK_HOLE) > BDRVRawState *s = bs->opaque; > - int ret = 0; > - struct { > - struct fiemap fm; > - struct fiemap_extent fe; > - } f; > + off_t offs; > > - if (s->skip_fiemap) { > - return -ENOTSUP; > + offs = lseek(s->fd, start, SEEK_HOLE); > + if (offs < 0) { > + goto dunno; > } > + assert(offs >= start); > > - f.fm.fm_start = start; > - f.fm.fm_length = (int64_t)nb_sectors * BDRV_SECTOR_SIZE; > - 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) { > - s->skip_fiemap = true; > - return -errno; > - } > - > - if (f.fm.fm_mapped_extents == 0) { > - /* No extents found, data is beyond f.fm.fm_start + f.fm.fm_length. > - * f.fm.fm_start + f.fm.fm_length must be clamped to the file size! > - */ > - off_t length = lseek(s->fd, 0, SEEK_END); > - *hole = f.fm.fm_start; > - *data = MIN(f.fm.fm_start + f.fm.fm_length, length); > - } else { > - *data = f.fe.fe_logical; > - *hole = f.fe.fe_logical + f.fe.fe_length; > - if (f.fe.fe_flags & FIEMAP_EXTENT_UNWRITTEN) { > - ret |= BDRV_BLOCK_ZERO; > - } > - } > - > - return ret; > -#else > - return -ENOTSUP; > -#endif > -} > - > -static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data, > - off_t *hole) > -{ > -#if defined SEEK_HOLE && defined SEEK_DATA > - BDRVRawState *s = bs->opaque; > - > - *hole = lseek(s->fd, start, SEEK_HOLE); > - if (*hole == -1) { > - return -errno; > - } > - > - if (*hole > start) { > + if (offs > start) { > + /* in data, next hole at offs */ > *data = start; > - } else { > - /* On a hole. We need another syscall to find its end. */ > - *data = lseek(s->fd, start, SEEK_DATA); > - if (*data == -1) { > - *data = lseek(s->fd, 0, SEEK_END); > - } > + *hole = offs; > + return; > } > > - return 0; > -#else > - return -ENOTSUP; > + /* 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; > + return; > + > +dunno: > #endif > + /* assume all data */ > + offs = lseek(s->fd, 0, SEEK_END); > + if (offs < 0) { > + /* now that's *really* unexpected */ > + offs = (off_t)1 << (sizeof(off_t) * 8 - 1); > + offs += offs - 1; > + } > + *data = start; > + *hole = offs; > } > > /* > @@ -1591,28 +1567,18 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, > nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE); > } > > - ret = try_seek_hole(bs, start, &data, &hole); > - if (ret < 0) { > - ret = try_fiemap(bs, start, &data, &hole, nb_sectors); > - if (ret < 0) { > - /* Assume everything is allocated. */ > - data = 0; > - hole = start + nb_sectors * BDRV_SECTOR_SIZE; > - ret = 0; > - } > - } > - > - assert(ret >= 0); > - > - if (data <= start) { > + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > + find_allocation(bs, start, &data, &hole); > + if (data == start) { > /* On a data extent, compute sectors to the end of the extent. */ > *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE); > - return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > } else { > /* On a hole, compute sectors to the beginning of the next extent. */ > + assert(hole == start); > *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE); > - return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start; > + ret |= BDRV_BLOCK_ZERO; > } > + return ret; > } > > static coroutine_fn BlockAIOCB *raw_aio_discard(BlockDriverState *bs, > -- > 1.9.3 > > Other than the wrong commit id in message, Reviewed-by: Fam Zheng <famz@redhat.com>
Fam Zheng <famz@redhat.com> writes: > On Wed, 11/12 20:27, Markus Armbruster wrote: >> Commit 5500316 (May 2012) implemented raw_co_is_allocated() as >> follows: >> >> 1. If defined(CONFIG_FIEMAP), use the FS_IOC_FIEMAP ioctl >> >> 2. Else if defined(SEEK_HOLE) && defined(SEEK_DATA), use lseek() >> >> 3. Else pretend there are no holes >> >> Later on, raw_co_is_allocated() was generalized to >> raw_co_get_block_status(). >> >> Commit 4f11aa8 (May 2014) changed it to try the three methods in order >> until success, because "there may be implementations which support >> [SEEK_HOLE/SEEK_DATA] but not [FIEMAP] (e.g., NFSv4.2) as well as vice >> versa." >> >> Unfortunately, we used FIEMAP incorrectly: we lacked FIEMAP_FLAG_SYNC. >> Commit 38c4d0a (Sep 2014) added it. Because that's a significant >> speed hit, the next commit 38c4d0a put SEEK_HOLE/SEEK_DATA first. > > s/38c4d0a/7c159037/ Fixing... >> As you see, the obvious use of FIEMAP is wrong, and the correct use is >> slow. I guess this puts it somewhere between -7 "The obvious use is >> wrong" and -10 "It's impossible to get right" on Rusty Russel's Hard >> to Misuse scale[*]. > > Nice reading :) Adapted from a comment Paolo made in a discussion preceding this patch :) [...] > Other than the wrong commit id in message, > > Reviewed-by: Fam Zheng <famz@redhat.com> Thanks!
On 2014-11-12 at 20:27, Markus Armbruster wrote: > Commit 5500316 (May 2012) implemented raw_co_is_allocated() as > follows: > > 1. If defined(CONFIG_FIEMAP), use the FS_IOC_FIEMAP ioctl > > 2. Else if defined(SEEK_HOLE) && defined(SEEK_DATA), use lseek() > > 3. Else pretend there are no holes > > Later on, raw_co_is_allocated() was generalized to > raw_co_get_block_status(). > > Commit 4f11aa8 (May 2014) changed it to try the three methods in order > until success, because "there may be implementations which support > [SEEK_HOLE/SEEK_DATA] but not [FIEMAP] (e.g., NFSv4.2) as well as vice > versa." > > Unfortunately, we used FIEMAP incorrectly: we lacked FIEMAP_FLAG_SYNC. > Commit 38c4d0a (Sep 2014) added it. Because that's a significant > speed hit, the next commit 38c4d0a put SEEK_HOLE/SEEK_DATA first. > > As you see, the obvious use of FIEMAP is wrong, and the correct use is > slow. I guess this puts it somewhere between -7 "The obvious use is > wrong" and -10 "It's impossible to get right" on Rusty Russel's Hard > to Misuse scale[*]. > > "Fortunately", the FIEMAP code is used only when > > * SEEK_HOLE/SEEK_DATA arent't defined, but CONFIG_FIEMAP is > > Uncommon. SEEK_HOLE had no XFS implementation between 2011 (when it > was introduced for ext4 and btrfs) and 2012. > > * SEEK_HOLE/SEEK_DATA and CONFIG_FIEMAP are defined, but lseek() fails > > Unlikely. > > Thus, the FIEMAP code executes rarely. Makes it a nice hidey-hole for > bugs. Worse, bugs hiding there can theoretically bite even on a host > that has SEEK_HOLE/SEEK_DATA. > > I don't want to worry about this crap, not even theoretically. Get > rid of it, then clean up the mess, including spotty error checking. > > [*] http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/raw-posix.c | 128 ++++++++++++++++++++---------------------------------- > 1 file changed, 47 insertions(+), 81 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 706d3c0..d16764c 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -60,9 +60,6 @@ > #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ > #endif > #endif > -#ifdef CONFIG_FIEMAP > -#include <linux/fiemap.h> > -#endif > #ifdef CONFIG_FALLOCATE_PUNCH_HOLE > #include <linux/falloc.h> > #endif > @@ -1481,77 +1478,56 @@ out: > return result; > } > > -static int try_fiemap(BlockDriverState *bs, off_t start, off_t *data, > - off_t *hole, int nb_sectors) > +/* > + * Find allocation range in @bs around offset @start. > + * If @start is in a hole, store @start in @hole and the end of the > + * hole in @data. > + * If @start is in a data, store @start to @data, and the end of the > + * data to @hole. > + * If we can't find out, pretend there are no holes. > + */ > +static void find_allocation(BlockDriverState *bs, off_t start, > + off_t *data, off_t *hole) > { > -#ifdef CONFIG_FIEMAP > +#if defined(SEEK_DATA) && defined(SEEK_HOLE) > BDRVRawState *s = bs->opaque; > - int ret = 0; > - struct { > - struct fiemap fm; > - struct fiemap_extent fe; > - } f; > + off_t offs; > > - if (s->skip_fiemap) { > - return -ENOTSUP; > + offs = lseek(s->fd, start, SEEK_HOLE); > + if (offs < 0) { > + goto dunno; > } > + assert(offs >= start); > > - f.fm.fm_start = start; > - f.fm.fm_length = (int64_t)nb_sectors * BDRV_SECTOR_SIZE; > - 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) { > - s->skip_fiemap = true; > - return -errno; > - } > - > - if (f.fm.fm_mapped_extents == 0) { > - /* No extents found, data is beyond f.fm.fm_start + f.fm.fm_length. > - * f.fm.fm_start + f.fm.fm_length must be clamped to the file size! > - */ > - off_t length = lseek(s->fd, 0, SEEK_END); > - *hole = f.fm.fm_start; > - *data = MIN(f.fm.fm_start + f.fm.fm_length, length); > - } else { > - *data = f.fe.fe_logical; > - *hole = f.fe.fe_logical + f.fe.fe_length; > - if (f.fe.fe_flags & FIEMAP_EXTENT_UNWRITTEN) { > - ret |= BDRV_BLOCK_ZERO; > - } > - } > - > - return ret; > -#else > - return -ENOTSUP; > -#endif > -} > - > -static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data, > - off_t *hole) > -{ > -#if defined SEEK_HOLE && defined SEEK_DATA > - BDRVRawState *s = bs->opaque; > - > - *hole = lseek(s->fd, start, SEEK_HOLE); > - if (*hole == -1) { > - return -errno; > - } > - > - if (*hole > start) { > + if (offs > start) { > + /* in data, next hole at offs */ > *data = start; > - } else { > - /* On a hole. We need another syscall to find its end. */ > - *data = lseek(s->fd, start, SEEK_DATA); > - if (*data == -1) { > - *data = lseek(s->fd, 0, SEEK_END); > - } > + *hole = offs; > + return; > } > > - return 0; > -#else > - return -ENOTSUP; > + /* 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; > + return; > + > +dunno: > #endif > + /* assume all data */ > + offs = lseek(s->fd, 0, SEEK_END); Why are you calling lseek() here at all? Just set offs to the maximum value and let the MIN() in the caller handle the rest. > + if (offs < 0) { > + /* now that's *really* unexpected */ > + offs = (off_t)1 << (sizeof(off_t) * 8 - 1); > + offs += offs - 1; > + } > + *data = start; > + *hole = offs; > } > > /* > @@ -1591,28 +1567,18 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, > nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE); > } > > - ret = try_seek_hole(bs, start, &data, &hole); > - if (ret < 0) { > - ret = try_fiemap(bs, start, &data, &hole, nb_sectors); > - if (ret < 0) { > - /* Assume everything is allocated. */ > - data = 0; > - hole = start + nb_sectors * BDRV_SECTOR_SIZE; > - ret = 0; > - } > - } > - > - assert(ret >= 0); > - > - if (data <= start) { > + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > + find_allocation(bs, start, &data, &hole); > + if (data == start) { > /* On a data extent, compute sectors to the end of the extent. */ > *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE); > - return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; > } else { > /* On a hole, compute sectors to the beginning of the next extent. */ > + assert(hole == start); > *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE); > - return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start; > + ret |= BDRV_BLOCK_ZERO; As Eric already said, this changes the behavior (might even break some tests, I'm not sure). It seems fine to me, though. Whether DATA should be included on holes in the file or not is a question which I don't have an answer to, so I'm with either; but you may want to mention it in the commit message. > } > + return ret; > } > > static coroutine_fn BlockAIOCB *raw_aio_discard(BlockDriverState *bs, Because nothing is strictly* wrong (except the ID in the commit message), have another R-b (there seem to be plenty of them today): Reviewed-by: Max Reitz <mreitz@redhat.com> *with "not strictly wrong" I'm referring to the DATA+ZERO change.
Eric Blake <eblake@redhat.com> writes: > On 11/12/2014 01:27 PM, Markus Armbruster wrote: >> Commit 5500316 (May 2012) implemented raw_co_is_allocated() as >> follows: >> > >> Thus, the FIEMAP code executes rarely. Makes it a nice hidey-hole for >> bugs. Worse, bugs hiding there can theoretically bite even on a host >> that has SEEK_HOLE/SEEK_DATA. >> >> I don't want to worry about this crap, not even theoretically. Get >> rid of it, then clean up the mess, including spotty error checking. > > Sounds reasonable to me. It's rather a big patch (both nuking a bad > interface and rewriting the use of the good interface) that might have > been better as two commits, but I can live with it. > >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/raw-posix.c | 128 ++++++++++++++++++++---------------------------------- >> 1 file changed, 47 insertions(+), 81 deletions(-) >> > >> +/* >> + * Find allocation range in @bs around offset @start. >> + * If @start is in a hole, store @start in @hole and the end of the >> + * hole in @data. >> + * If @start is in a data, store @start to @data, and the end of the >> + * data to @hole. >> + * If we can't find out, pretend there are no holes. >> + */ >> +static void find_allocation(BlockDriverState *bs, off_t start, >> + off_t *data, off_t *hole) > > Sounds like a good contract interface. > >> + /* 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? Yes. Fixing... >> + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; >> + find_allocation(bs, start, &data, &hole); >> + if (data == start) { >> /* On a data extent, compute sectors to the end of the extent. */ >> *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE); >> - return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; >> } else { >> /* On a hole, compute sectors to the beginning of the next extent. */ >> + assert(hole == start); >> *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE); >> - return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start; >> + ret |= BDRV_BLOCK_ZERO; >> } >> + return ret; > > The old code omits BDRV_BLOCK_DATA on a hole. Why are you adding it > here, and why are you not mentioning it in the commit message? I got confused. Here's how block.h explains the allocation flags: * DATA ZERO OFFSET_VALID * t t t sectors read as zero, bs->file is zero at offset * t f t sectors read as valid from bs->file at offset * f t t sectors preallocated, read as zero, bs->file not * necessarily zero at offset * f f t sectors preallocated but read from backing_hd, * bs->file contains garbage at offset Should a hole in a bdrv_file bs have status DATA | ZERO (first row) or just ZERO (third row)? First row: * "sectors read as zero": certainly true in a hole. * "bs->file is zero at offset": not sure what that's supposed to mean. bs->file is null. Third row: * "sectors preallocated": not sure what that's supposed to mean. Probably preallocation != off. If that's what it means, then it's false in a hole. * "read as zero": certainly true in a hole. * "bs->file not necessarily zero at offset": not sure what that's supposed to mean. bs->file is null. Now you're probably confused, too. Anyway, I shouldn't make such a change in a cleanup patch! v2 will stick to the old flags. Thanks!
Max Reitz <mreitz@redhat.com> writes: > On 2014-11-12 at 20:27, Markus Armbruster wrote: >> Commit 5500316 (May 2012) implemented raw_co_is_allocated() as >> follows: >> >> 1. If defined(CONFIG_FIEMAP), use the FS_IOC_FIEMAP ioctl >> >> 2. Else if defined(SEEK_HOLE) && defined(SEEK_DATA), use lseek() >> >> 3. Else pretend there are no holes >> >> Later on, raw_co_is_allocated() was generalized to >> raw_co_get_block_status(). >> >> Commit 4f11aa8 (May 2014) changed it to try the three methods in order >> until success, because "there may be implementations which support >> [SEEK_HOLE/SEEK_DATA] but not [FIEMAP] (e.g., NFSv4.2) as well as vice >> versa." >> >> Unfortunately, we used FIEMAP incorrectly: we lacked FIEMAP_FLAG_SYNC. >> Commit 38c4d0a (Sep 2014) added it. Because that's a significant >> speed hit, the next commit 38c4d0a put SEEK_HOLE/SEEK_DATA first. >> >> As you see, the obvious use of FIEMAP is wrong, and the correct use is >> slow. I guess this puts it somewhere between -7 "The obvious use is >> wrong" and -10 "It's impossible to get right" on Rusty Russel's Hard >> to Misuse scale[*]. >> >> "Fortunately", the FIEMAP code is used only when >> >> * SEEK_HOLE/SEEK_DATA arent't defined, but CONFIG_FIEMAP is >> >> Uncommon. SEEK_HOLE had no XFS implementation between 2011 (when it >> was introduced for ext4 and btrfs) and 2012. >> >> * SEEK_HOLE/SEEK_DATA and CONFIG_FIEMAP are defined, but lseek() fails >> >> Unlikely. >> >> Thus, the FIEMAP code executes rarely. Makes it a nice hidey-hole for >> bugs. Worse, bugs hiding there can theoretically bite even on a host >> that has SEEK_HOLE/SEEK_DATA. >> >> I don't want to worry about this crap, not even theoretically. Get >> rid of it, then clean up the mess, including spotty error checking. >> >> [*] http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/raw-posix.c | 128 ++++++++++++++++++++---------------------------------- >> 1 file changed, 47 insertions(+), 81 deletions(-) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 706d3c0..d16764c 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -60,9 +60,6 @@ >> #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ >> #endif >> #endif >> -#ifdef CONFIG_FIEMAP >> -#include <linux/fiemap.h> >> -#endif >> #ifdef CONFIG_FALLOCATE_PUNCH_HOLE >> #include <linux/falloc.h> >> #endif >> @@ -1481,77 +1478,56 @@ out: >> return result; >> } >> -static int try_fiemap(BlockDriverState *bs, off_t start, off_t >> *data, >> - off_t *hole, int nb_sectors) >> +/* >> + * Find allocation range in @bs around offset @start. >> + * If @start is in a hole, store @start in @hole and the end of the >> + * hole in @data. >> + * If @start is in a data, store @start to @data, and the end of the >> + * data to @hole. >> + * If we can't find out, pretend there are no holes. >> + */ >> +static void find_allocation(BlockDriverState *bs, off_t start, >> + off_t *data, off_t *hole) >> { >> -#ifdef CONFIG_FIEMAP >> +#if defined(SEEK_DATA) && defined(SEEK_HOLE) >> BDRVRawState *s = bs->opaque; >> - int ret = 0; >> - struct { >> - struct fiemap fm; >> - struct fiemap_extent fe; >> - } f; >> + off_t offs; >> - if (s->skip_fiemap) { >> - return -ENOTSUP; >> + offs = lseek(s->fd, start, SEEK_HOLE); >> + if (offs < 0) { >> + goto dunno; >> } >> + assert(offs >= start); >> - f.fm.fm_start = start; >> - f.fm.fm_length = (int64_t)nb_sectors * BDRV_SECTOR_SIZE; >> - 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) { >> - s->skip_fiemap = true; >> - return -errno; >> - } >> - >> - if (f.fm.fm_mapped_extents == 0) { >> - /* No extents found, data is beyond f.fm.fm_start + f.fm.fm_length. >> - * f.fm.fm_start + f.fm.fm_length must be clamped to the file size! >> - */ >> - off_t length = lseek(s->fd, 0, SEEK_END); >> - *hole = f.fm.fm_start; >> - *data = MIN(f.fm.fm_start + f.fm.fm_length, length); >> - } else { >> - *data = f.fe.fe_logical; >> - *hole = f.fe.fe_logical + f.fe.fe_length; >> - if (f.fe.fe_flags & FIEMAP_EXTENT_UNWRITTEN) { >> - ret |= BDRV_BLOCK_ZERO; >> - } >> - } >> - >> - return ret; >> -#else >> - return -ENOTSUP; >> -#endif >> -} >> - >> -static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data, >> - off_t *hole) >> -{ >> -#if defined SEEK_HOLE && defined SEEK_DATA >> - BDRVRawState *s = bs->opaque; >> - >> - *hole = lseek(s->fd, start, SEEK_HOLE); >> - if (*hole == -1) { >> - return -errno; >> - } >> - >> - if (*hole > start) { >> + if (offs > start) { >> + /* in data, next hole at offs */ >> *data = start; >> - } else { >> - /* On a hole. We need another syscall to find its end. */ >> - *data = lseek(s->fd, start, SEEK_DATA); >> - if (*data == -1) { >> - *data = lseek(s->fd, 0, SEEK_END); >> - } >> + *hole = offs; >> + return; >> } >> - return 0; >> -#else >> - return -ENOTSUP; >> + /* 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; >> + return; >> + >> +dunno: >> #endif >> + /* assume all data */ >> + offs = lseek(s->fd, 0, SEEK_END); > > Why are you calling lseek() here at all? Just set offs to the maximum > value and let the MIN() in the caller handle the rest. You're right. Furthermore, making up a value for *hole here that the caller will clamp to nb_sectors feels stupid. I'll simplify in v2. >> + if (offs < 0) { >> + /* now that's *really* unexpected */ >> + offs = (off_t)1 << (sizeof(off_t) * 8 - 1); >> + offs += offs - 1; >> + } >> + *data = start; >> + *hole = offs; >> } >> /* >> @@ -1591,28 +1567,18 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, >> nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE); >> } >> - ret = try_seek_hole(bs, start, &data, &hole); >> - if (ret < 0) { >> - ret = try_fiemap(bs, start, &data, &hole, nb_sectors); >> - if (ret < 0) { >> - /* Assume everything is allocated. */ >> - data = 0; >> - hole = start + nb_sectors * BDRV_SECTOR_SIZE; >> - ret = 0; >> - } >> - } >> - >> - assert(ret >= 0); >> - >> - if (data <= start) { >> + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; >> + find_allocation(bs, start, &data, &hole); >> + if (data == start) { >> /* On a data extent, compute sectors to the end of the extent. */ >> *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE); >> - return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; >> } else { >> /* On a hole, compute sectors to the beginning of the next extent. */ >> + assert(hole == start); >> *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE); >> - return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start; >> + ret |= BDRV_BLOCK_ZERO; > > As Eric already said, this changes the behavior (might even break some > tests, I'm not sure). It seems fine to me, though. Whether DATA should > be included on holes in the file or not is a question which I don't > have an answer to, so I'm with either; but you may want to mention it > in the commit message. See my reply to Eric. >> } >> + return ret; >> } >> static coroutine_fn BlockAIOCB *raw_aio_discard(BlockDriverState >> *bs, > > Because nothing is strictly* wrong (except the ID in the commit > message), have another R-b (there seem to be plenty of them today): > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > *with "not strictly wrong" I'm referring to the DATA+ZERO change. Thanks!
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. 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? Kevin
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. > > 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. Max
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. > > > >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. Kevin
On 2014-11-13 at 13:00, Kevin Wolf wrote: > 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. >>> >>> 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. Well, iotest 102 tests exactly that. Max
Kevin Wolf <kwolf@redhat.com> 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.
Am 13.11.2014 um 13:38 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@redhat.com> 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
diff --git a/block/raw-posix.c b/block/raw-posix.c index 706d3c0..d16764c 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -60,9 +60,6 @@ #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ #endif #endif -#ifdef CONFIG_FIEMAP -#include <linux/fiemap.h> -#endif #ifdef CONFIG_FALLOCATE_PUNCH_HOLE #include <linux/falloc.h> #endif @@ -1481,77 +1478,56 @@ out: return result; } -static int try_fiemap(BlockDriverState *bs, off_t start, off_t *data, - off_t *hole, int nb_sectors) +/* + * Find allocation range in @bs around offset @start. + * If @start is in a hole, store @start in @hole and the end of the + * hole in @data. + * If @start is in a data, store @start to @data, and the end of the + * data to @hole. + * If we can't find out, pretend there are no holes. + */ +static void find_allocation(BlockDriverState *bs, off_t start, + off_t *data, off_t *hole) { -#ifdef CONFIG_FIEMAP +#if defined(SEEK_DATA) && defined(SEEK_HOLE) BDRVRawState *s = bs->opaque; - int ret = 0; - struct { - struct fiemap fm; - struct fiemap_extent fe; - } f; + off_t offs; - if (s->skip_fiemap) { - return -ENOTSUP; + offs = lseek(s->fd, start, SEEK_HOLE); + if (offs < 0) { + goto dunno; } + assert(offs >= start); - f.fm.fm_start = start; - f.fm.fm_length = (int64_t)nb_sectors * BDRV_SECTOR_SIZE; - 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) { - s->skip_fiemap = true; - return -errno; - } - - if (f.fm.fm_mapped_extents == 0) { - /* No extents found, data is beyond f.fm.fm_start + f.fm.fm_length. - * f.fm.fm_start + f.fm.fm_length must be clamped to the file size! - */ - off_t length = lseek(s->fd, 0, SEEK_END); - *hole = f.fm.fm_start; - *data = MIN(f.fm.fm_start + f.fm.fm_length, length); - } else { - *data = f.fe.fe_logical; - *hole = f.fe.fe_logical + f.fe.fe_length; - if (f.fe.fe_flags & FIEMAP_EXTENT_UNWRITTEN) { - ret |= BDRV_BLOCK_ZERO; - } - } - - return ret; -#else - return -ENOTSUP; -#endif -} - -static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data, - off_t *hole) -{ -#if defined SEEK_HOLE && defined SEEK_DATA - BDRVRawState *s = bs->opaque; - - *hole = lseek(s->fd, start, SEEK_HOLE); - if (*hole == -1) { - return -errno; - } - - if (*hole > start) { + if (offs > start) { + /* in data, next hole at offs */ *data = start; - } else { - /* On a hole. We need another syscall to find its end. */ - *data = lseek(s->fd, start, SEEK_DATA); - if (*data == -1) { - *data = lseek(s->fd, 0, SEEK_END); - } + *hole = offs; + return; } - return 0; -#else - return -ENOTSUP; + /* 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; + return; + +dunno: #endif + /* assume all data */ + offs = lseek(s->fd, 0, SEEK_END); + if (offs < 0) { + /* now that's *really* unexpected */ + offs = (off_t)1 << (sizeof(off_t) * 8 - 1); + offs += offs - 1; + } + *data = start; + *hole = offs; } /* @@ -1591,28 +1567,18 @@ static int64_t coroutine_fn raw_co_get_block_status(BlockDriverState *bs, nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE); } - ret = try_seek_hole(bs, start, &data, &hole); - if (ret < 0) { - ret = try_fiemap(bs, start, &data, &hole, nb_sectors); - if (ret < 0) { - /* Assume everything is allocated. */ - data = 0; - hole = start + nb_sectors * BDRV_SECTOR_SIZE; - ret = 0; - } - } - - assert(ret >= 0); - - if (data <= start) { + ret = BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; + find_allocation(bs, start, &data, &hole); + if (data == start) { /* On a data extent, compute sectors to the end of the extent. */ *pnum = MIN(nb_sectors, (hole - start) / BDRV_SECTOR_SIZE); - return ret | BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID | start; } else { /* On a hole, compute sectors to the beginning of the next extent. */ + assert(hole == start); *pnum = MIN(nb_sectors, (data - start) / BDRV_SECTOR_SIZE); - return ret | BDRV_BLOCK_ZERO | BDRV_BLOCK_OFFSET_VALID | start; + ret |= BDRV_BLOCK_ZERO; } + return ret; } static coroutine_fn BlockAIOCB *raw_aio_discard(BlockDriverState *bs,
Commit 5500316 (May 2012) implemented raw_co_is_allocated() as follows: 1. If defined(CONFIG_FIEMAP), use the FS_IOC_FIEMAP ioctl 2. Else if defined(SEEK_HOLE) && defined(SEEK_DATA), use lseek() 3. Else pretend there are no holes Later on, raw_co_is_allocated() was generalized to raw_co_get_block_status(). Commit 4f11aa8 (May 2014) changed it to try the three methods in order until success, because "there may be implementations which support [SEEK_HOLE/SEEK_DATA] but not [FIEMAP] (e.g., NFSv4.2) as well as vice versa." Unfortunately, we used FIEMAP incorrectly: we lacked FIEMAP_FLAG_SYNC. Commit 38c4d0a (Sep 2014) added it. Because that's a significant speed hit, the next commit 38c4d0a put SEEK_HOLE/SEEK_DATA first. As you see, the obvious use of FIEMAP is wrong, and the correct use is slow. I guess this puts it somewhere between -7 "The obvious use is wrong" and -10 "It's impossible to get right" on Rusty Russel's Hard to Misuse scale[*]. "Fortunately", the FIEMAP code is used only when * SEEK_HOLE/SEEK_DATA arent't defined, but CONFIG_FIEMAP is Uncommon. SEEK_HOLE had no XFS implementation between 2011 (when it was introduced for ext4 and btrfs) and 2012. * SEEK_HOLE/SEEK_DATA and CONFIG_FIEMAP are defined, but lseek() fails Unlikely. Thus, the FIEMAP code executes rarely. Makes it a nice hidey-hole for bugs. Worse, bugs hiding there can theoretically bite even on a host that has SEEK_HOLE/SEEK_DATA. I don't want to worry about this crap, not even theoretically. Get rid of it, then clean up the mess, including spotty error checking. [*] http://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/raw-posix.c | 128 ++++++++++++++++++++---------------------------------- 1 file changed, 47 insertions(+), 81 deletions(-)