| Message ID | 1415873823-13844-5-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2014-11-13 at 11:17, Markus Armbruster wrote: > try_seek_hole() doesn't really seek to a hole, it tries to find out > whether its argument is in a hole or not, and where the hole or > non-hole ends. Rename to find_allocation() and add a proper function > comment. > > Using arguments passed by reference like local variables is a bad > habit. Only assign to them right before return. > > Avoid nesting of conditionals. > > When find_allocation() fails, don't make up a range that'll get mapped > to nb_sectors, simply set *pnum = nb_sectors directly. > > Don't repeat BDRV_BLOCK_OFFSET_VALID | start. > > Drop a pointless assertion, add some meaningful ones. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/raw-posix.c | 62 +++++++++++++++++++++++++++++++++---------------------- > 1 file changed, 37 insertions(+), 25 deletions(-) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 2a12a50..ea5b3b8 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1478,28 +1478,43 @@ out: > return result; > } > > -static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data, > - off_t *hole) > +/* > + * 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, and return 0. > + * If @start is in a data, store @start to @data, and the end of the "is in a data" sounds funny enough I'd even like to keep it. Probably should be "data extent" or something similar. > + * data to @hole, and return 0. Here, too. With or without that changed: Reviewed-by: Max Reitz <mreitz@redhat.com>
Max Reitz <mreitz@redhat.com> writes: > On 2014-11-13 at 11:17, Markus Armbruster wrote: >> try_seek_hole() doesn't really seek to a hole, it tries to find out >> whether its argument is in a hole or not, and where the hole or >> non-hole ends. Rename to find_allocation() and add a proper function >> comment. >> >> Using arguments passed by reference like local variables is a bad >> habit. Only assign to them right before return. >> >> Avoid nesting of conditionals. >> >> When find_allocation() fails, don't make up a range that'll get mapped >> to nb_sectors, simply set *pnum = nb_sectors directly. >> >> Don't repeat BDRV_BLOCK_OFFSET_VALID | start. >> >> Drop a pointless assertion, add some meaningful ones. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/raw-posix.c | 62 +++++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 37 insertions(+), 25 deletions(-) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 2a12a50..ea5b3b8 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -1478,28 +1478,43 @@ out: >> return result; >> } >> -static int try_seek_hole(BlockDriverState *bs, off_t start, off_t >> *data, >> - off_t *hole) >> +/* >> + * 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, and return 0. >> + * If @start is in a data, store @start to @data, and the end of the > > "is in a data" sounds funny enough I'd even like to keep it. Probably > should be "data extent" or something similar. Okay. >> + * data to @hole, and return 0. > > Here, too. > > With or without that changed: > > Reviewed-by: Max Reitz <mreitz@redhat.com> Thanks!
diff --git a/block/raw-posix.c b/block/raw-posix.c index 2a12a50..ea5b3b8 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1478,28 +1478,43 @@ out: return result; } -static int try_seek_hole(BlockDriverState *bs, off_t start, off_t *data, - off_t *hole) +/* + * 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, and return 0. + * If @start is in a data, store @start to @data, and the end of the + * data to @hole, and return 0. + * If we can't find out, return -errno. + */ +static int find_allocation(BlockDriverState *bs, off_t start, + off_t *data, off_t *hole) { #if defined SEEK_HOLE && defined SEEK_DATA BDRVRawState *s = bs->opaque; + off_t offs; - *hole = lseek(s->fd, start, SEEK_HOLE); - if (*hole == -1) { + offs = lseek(s->fd, start, SEEK_HOLE); + if (offs < 0) { return -errno; } + assert(offs >= start); - 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 < 0) { - /* no idea where the hole ends, give up (unlikely to happen) */ - return -errno; - } + *hole = offs; + return 0; } + /* 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) */ + return -errno; + } + assert(offs > start); + *hole = start; + *data = offs; return 0; #else return -ENOTSUP; @@ -1543,25 +1558,22 @@ 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) { - /* Assume everything is allocated. */ - data = 0; - hole = start + nb_sectors * BDRV_SECTOR_SIZE; - ret = 0; - } - - assert(ret >= 0); - - if (data <= start) { + ret = BDRV_BLOCK_OFFSET_VALID | start; + if (find_allocation(bs, start, &data, &hole) < 0) { + /* No info available, so pretend there are no holes */ + *pnum = nb_sectors; + ret |= BDRV_BLOCK_DATA; + } else 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; + ret |= BDRV_BLOCK_DATA; } 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,
try_seek_hole() doesn't really seek to a hole, it tries to find out whether its argument is in a hole or not, and where the hole or non-hole ends. Rename to find_allocation() and add a proper function comment. Using arguments passed by reference like local variables is a bad habit. Only assign to them right before return. Avoid nesting of conditionals. When find_allocation() fails, don't make up a range that'll get mapped to nb_sectors, simply set *pnum = nb_sectors directly. Don't repeat BDRV_BLOCK_OFFSET_VALID | start. Drop a pointless assertion, add some meaningful ones. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/raw-posix.c | 62 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 25 deletions(-)