[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2] block/gluster: add support for SEEK_DATA/SEE
From: |
Jeff Cody |
Subject: |
Re: [Qemu-block] [PATCH v2] block/gluster: add support for SEEK_DATA/SEEK_HOLE |
Date: |
Wed, 9 Mar 2016 17:19:27 -0500 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Wed, Mar 09, 2016 at 07:12:41PM +0100, Niels de Vos wrote:
> On Wed, Mar 09, 2016 at 10:46:02AM -0500, Jeff Cody wrote:
> > On Wed, Mar 09, 2016 at 01:30:14PM +0100, Niels de Vos wrote:
> > > GlusterFS 3.8 contains support for SEEK_DATA and SEEK_HOLE. This makes
> > > it possible to detect sparse areas in files.
> > >
> > > Signed-off-by: Niels de Vos <address@hidden>
> > >
> > > ---
> > > Tested by compiling and running "qemu-img map gluster://..." with a
> > > build of the current master branch of glusterfs. Using a Fedora cloud
> > > image (in raw format) shows many SEEK procudure calls going back and
> > > forth over the network. The output of "qemu map" matches the output when
> > > run against the image on the local filesystem.
> > >
> > > v2 based on feedback from Jeff Cody:
> > > - Replace compile time detection by runtime detection
> > > - Update return pointer (new argument) for .bdrv_co_get_block_status
> > > ---
> > > block/gluster.c | 186
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 186 insertions(+)
> > >
> > > diff --git a/block/gluster.c b/block/gluster.c
> > > index 65077a0..b01ab52 100644
> > > --- a/block/gluster.c
> > > +++ b/block/gluster.c
> > > @@ -23,6 +23,7 @@ typedef struct GlusterAIOCB {
> > > typedef struct BDRVGlusterState {
> > > struct glfs *glfs;
> > > struct glfs_fd *fd;
> > > + bool supports_seek_data;
> > > } BDRVGlusterState;
> > >
> > > typedef struct GlusterConf {
> > > @@ -286,6 +287,33 @@ static void qemu_gluster_parse_flags(int bdrv_flags,
> > > int *open_flags)
> > > }
> > > }
> > >
> > > +/*
> > > + * Do SEEK_DATA/HOLE to detect if it is functional. In order to be
> > > usable, it
> > > + * should return different values for the start of data and the start of
> > > a
> > > + * hole. There are three different cases to handle:
> > > + *
> > > + * - the same position is returned for data/hole (indicates broken
> > > gfapi)
> > > + * - an error is returned:
> > > + * - ENXIO only gets returned if there is valid support on
> > > client+server
> > > + * - EINVAL is returned when gfapi or the server does not support it
> > > + */
> > > +static bool qemu_gluster_test_seek(struct glfs_fd *fd)
> > > +{
> > > + off_t start_data, start_hole;
> > > + bool supports_seek_data = false;
> > > +
> > > + start_data = glfs_lseek(fd, 0, SEEK_DATA);
> > > + if (start_data != -1) {
> >
> > I recommend just checking if the returned value is >= 0.
>
> Ok, I can change that.
>
> > > + start_hole = glfs_lseek(fd, 0, SEEK_HOLE);
> > > + if (start_hole != -1)
> >
> > Minor formatting nit: per QEMU coding standard, all conditional
> > statements require brackets.
>
> Ah, sure, will do.
>
> > > + supports_seek_data = !(start_data == start_hole);
> > > + } else if (errno == ENXIO) {
> >
> > This errno check for ENXIO won't catch the case if an ENXIO error
> > occurs in the SEEK_HOLE call.
>
> I'm not sure if I'm following. lseek(SEEK_DATA) returns -1 and sets
> errno to ENXIO when the position in the filedescriptor is EOF. In this
> test, we check from position=0, so when ENXIO is returned, we know the
> file is empty and the return value+errno has gone through the whole
> Gluster stack.
>
> EINVAL would be an other error that is expected, in case either (a
> current) gfapi or the server do not support SEEK_DATA.
>
> I do not think there is a need to check for ENXIO on lseek(SEEK_HOLE).
Hmm, I think you are right - I can't think of any scenario that
SEEK_HOLE should return ENXIO that SEEK_DATA would not.
As matter of fact, if we can rely on ENXIO always indicating if
SEEK_DATA is supported by gluster, then we can make the whole
detection process much simpler, like this:
static bool qemu_gluster_test_seek(struct glfs_fd *fd)
{
off_t ret, eof;
eof = glfs_lseek(fd, 0, SEEK_END);
if (eof < 0) {
/* this shouldn't occur */
return false;
}
/* this should always fail with ENXIO if SEEK_DATA is supported */
ret = glfs_lseek(fd, eof, SEEK_DATA);
return (ret < 0) && (errno == ENXIO);
}
>
> Do you have a preference on how I should change it?
>
> > > + supports_seek_data = true;
> > > + }
> > > +
> > > + return supports_seek_data;
> > > +}
> > > +
> > > static int qemu_gluster_open(BlockDriverState *bs, QDict *options,
> > > int bdrv_flags, Error **errp)
> > > {
> > > @@ -320,6 +348,8 @@ static int qemu_gluster_open(BlockDriverState *bs,
> > > QDict *options,
> > > ret = -errno;
> > > }
> > >
> > > + s->supports_seek_data = qemu_gluster_test_seek(s->fd);
> > > +
> > > out:
> > > qemu_opts_del(opts);
> > > qemu_gluster_gconf_free(gconf);
> > > @@ -677,6 +707,158 @@ static int
> > > qemu_gluster_has_zero_init(BlockDriverState *bs)
> > > return 0;
> > > }
> > >
> > > +/*
> > > + * Find allocation range in @bs around offset @start.
> > > + * May change underlying file descriptor's file offset.
> > > + * If @start is not in a hole, store @start in @data, and the
> > > + * beginning of the next hole in @hole, and return 0.
> > > + * If @start is in a non-trailing hole, store @start in @hole and the
> > > + * beginning of the next non-hole in @data, and return 0.
> > > + * If @start is in a trailing hole or beyond EOF, return -ENXIO.
> > > + * If we can't find out, return a negative errno other than -ENXIO.
> > > + *
> > > + * (Shamefully copied from raw-posix.c, only miniscule adaptions.)
> > > + */
> > > +static int find_allocation(BlockDriverState *bs, off_t start,
> > > + off_t *data, off_t *hole)
> > > +{
> > > + BDRVGlusterState *s = bs->opaque;
> > > + off_t offs;
> > > +
> > > + if (!s->supports_seek_data)
> >
> > Another formatting nit: brackets needed here as well.
>
> Ok!
>
> > > + return -EINVAL;
> >
> > -ENOTSUP would probably be a better fit here, but I don't care too
> > much, since the error code isn't passed along outside the gluster
> > driver.
>
> Yes, I agree that -ENOTSUP is nicer, will change that too.
>
> > > +
> > > + /*
> > > + * SEEK_DATA cases:
> > > + * D1. offs == start: start is in data
> > > + * D2. offs > start: start is in a hole, next data at offs
> > > + * D3. offs < 0, errno = ENXIO: either start is in a trailing hole
> > > + * or start is beyond EOF
> > > + * If the latter happens, the file has been truncated behind
> > > + * our back since we opened it. All bets are off then.
> > > + * Treating like a trailing hole is simplest.
> > > + * D4. offs < 0, errno != ENXIO: we learned nothing
> > > + */
> > > + offs = glfs_lseek(s->fd, start, SEEK_DATA);
> > > + if (offs < 0) {
> > > + return -errno; /* D3 or D4 */
> > > + }
> > > + assert(offs >= start);
> > > +
> > > + if (offs > start) {
> > > + /* D2: in hole, next data at offs */
> > > + *hole = start;
> > > + *data = offs;
> > > + return 0;
> > > + }
> > > +
> > > + /* D1: in data, end not yet known */
> > > +
> > > + /*
> > > + * SEEK_HOLE cases:
> > > + * H1. offs == start: start is in a hole
> > > + * If this happens here, a hole has been dug behind our back
> > > + * since the previous lseek().
> > > + * H2. offs > start: either start is in data, next hole at offs,
> > > + * or start is in trailing hole, EOF at offs
> > > + * Linux treats trailing holes like any other hole: offs ==
> > > + * start. Solaris seeks to EOF instead: offs > start (blech).
> > > + * If that happens here, a hole has been dug behind our back
> > > + * since the previous lseek().
> > > + * H3. offs < 0, errno = ENXIO: start is beyond EOF
> > > + * If this happens, the file has been truncated behind our
> > > + * back since we opened it. Treat it like a trailing hole.
> > > + * H4. offs < 0, errno != ENXIO: we learned nothing
> > > + * Pretend we know nothing at all, i.e. "forget" about D1.
> > > + */
> > > + offs = glfs_lseek(s->fd, start, SEEK_HOLE);
> > > + if (offs < 0) {
> > > + return -errno; /* D1 and (H3 or H4) */
> > > + }
> > > + assert(offs >= start);
> > > +
> > > + if (offs > start) {
> > > + /*
> > > + * D1 and H2: either in data, next hole at offs, or it was in
> > > + * data but is now in a trailing hole. In the latter case,
> > > + * all bets are off. Treating it as if it there was data all
> > > + * the way to EOF is safe, so simply do that.
> > > + */
> > > + *data = start;
> > > + *hole = offs;
> > > + return 0;
> > > + }
> > > +
> > > + /* D1 and H1 */
> > > + return -EBUSY;
> > > +}
> > > +
> > > +/*
> > > + * Returns the allocation status of the specified sectors.
> > > + *
> > > + * If 'sector_num' is beyond the end of the disk image the return value
> > > is 0
> > > + * and 'pnum' is set to 0.
> > > + *
> > > + * 'pnum' is set to the number of sectors (including and immediately
> > > following
> > > + * the specified sector) that are known to be in the same
> > > + * allocated/unallocated state.
> > > + *
> > > + * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors
> > > goes
> > > + * beyond the end of the disk image it will be clamped.
> > > + *
> > > + * (Based on raw_co_get_block_status() from raw-posix.c.)
> > > + */
> > > +static int64_t coroutine_fn qemu_gluster_co_get_block_status(
> > > + BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
> >
> > Nit: a tab snuck in the above line, before "BlockDriverState" (tabs
> > expanded to spaces in coding guidelines as well).
>
> Oh, yuck :-/
>
> Thanks for the review!
> Niels
>
>
> > > + BlockDriverState **file)
> > > +{
> > > + BDRVGlusterState *s = bs->opaque;
> > > + off_t start, data = 0, hole = 0;
> > > + int64_t total_size;
> > > + int ret = -EINVAL;
> > > +
> > > + if (!s->fd) {
> > > + return ret;
> > > + }
> > > +
> > > + start = sector_num * BDRV_SECTOR_SIZE;
> > > + total_size = bdrv_getlength(bs);
> > > + if (total_size < 0) {
> > > + return total_size;
> > > + } else if (start >= total_size) {
> > > + *pnum = 0;
> > > + return 0;
> > > + } else if (start + nb_sectors * BDRV_SECTOR_SIZE > total_size) {
> > > + nb_sectors = DIV_ROUND_UP(total_size - start, BDRV_SECTOR_SIZE);
> > > + }
> > > +
> > > + ret = find_allocation(bs, start, &data, &hole);
> > > + if (ret == -ENXIO) {
> > > + /* Trailing hole */
> > > + *pnum = nb_sectors;
> > > + ret = BDRV_BLOCK_ZERO;
> > > + } else if (ret < 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,
> > > + * possibly including a partial sector at EOF. */
> > > + *pnum = MIN(nb_sectors, DIV_ROUND_UP(hole - start,
> > > BDRV_SECTOR_SIZE));
> > > + 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);
> > > + ret = BDRV_BLOCK_ZERO;
> > > + }
> > > +
> > > + *file = bs;
> > > +
> > > + return ret | BDRV_BLOCK_OFFSET_VALID | start;
> > > +}
> > > +
> > > +
> > > static QemuOptsList qemu_gluster_create_opts = {
> > > .name = "qemu-gluster-create-opts",
> > > .head = QTAILQ_HEAD_INITIALIZER(qemu_gluster_create_opts.head),
> > > @@ -719,6 +901,7 @@ static BlockDriver bdrv_gluster = {
> > > #ifdef CONFIG_GLUSTERFS_ZEROFILL
> > > .bdrv_co_write_zeroes = qemu_gluster_co_write_zeroes,
> > > #endif
> > > + .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
> > > .create_opts = &qemu_gluster_create_opts,
> > > };
> > >
> > > @@ -746,6 +929,7 @@ static BlockDriver bdrv_gluster_tcp = {
> > > #ifdef CONFIG_GLUSTERFS_ZEROFILL
> > > .bdrv_co_write_zeroes = qemu_gluster_co_write_zeroes,
> > > #endif
> > > + .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
> > > .create_opts = &qemu_gluster_create_opts,
> > > };
> > >
> > > @@ -773,6 +957,7 @@ static BlockDriver bdrv_gluster_unix = {
> > > #ifdef CONFIG_GLUSTERFS_ZEROFILL
> > > .bdrv_co_write_zeroes = qemu_gluster_co_write_zeroes,
> > > #endif
> > > + .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
> > > .create_opts = &qemu_gluster_create_opts,
> > > };
> > >
> > > @@ -800,6 +985,7 @@ static BlockDriver bdrv_gluster_rdma = {
> > > #ifdef CONFIG_GLUSTERFS_ZEROFILL
> > > .bdrv_co_write_zeroes = qemu_gluster_co_write_zeroes,
> > > #endif
> > > + .bdrv_co_get_block_status = qemu_gluster_co_get_block_status,
> > > .create_opts = &qemu_gluster_create_opts,
> > > };
> > >
> > > --
> > > 2.5.0
> > >
- [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Niels de Vos, 2016/03/07
- Re: [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Jeff Cody, 2016/03/07
- Re: [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Niels de Vos, 2016/03/07
- Re: [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Jeff Cody, 2016/03/08
- Re: [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Niels de Vos, 2016/03/08
- [Qemu-block] [PATCH v2] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Niels de Vos, 2016/03/09
- Re: [Qemu-block] [PATCH v2] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Jeff Cody, 2016/03/09
- Re: [Qemu-block] [PATCH v2] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Niels de Vos, 2016/03/09
- Re: [Qemu-block] [PATCH v2] block/gluster: add support for SEEK_DATA/SEEK_HOLE,
Jeff Cody <=
- [Qemu-block] [PATCH v3] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Niels de Vos, 2016/03/10
- Re: [Qemu-block] [PATCH v3] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Jeff Cody, 2016/03/15
- Re: [Qemu-block] [PATCH v3] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Jeff Cody, 2016/03/15
- Re: [Qemu-block] [PATCH v3] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Niels de Vos, 2016/03/16
- Re: [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Kevin Wolf, 2016/03/08
- Re: [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Niels de Vos, 2016/03/08
Re: [Qemu-block] [PATCH] block/gluster: add support for SEEK_DATA/SEEK_HOLE, Eric Blake, 2016/03/07