On current released versions of glusterfs, glfs_lseek() will sometimes
return invalid values for SEEK_DATA or SEEK_HOLE.  For SEEK_DATA and
SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in
the case of error:

LSEEK(2):

    off_t lseek(int fd, off_t offset, int whence);

    [...]

    SEEK_HOLE
              Adjust  the file offset to the next hole in the file greater
              than or equal to offset.  If offset points into the middle of
              a hole, then the file offset is set to offset.  If there is no
              hole past offset, then the file offset is adjusted to the end
              of the file (i.e., there is  an implicit hole at the end of
              any file).

    [...]

    RETURN VALUE
              Upon  successful  completion,  lseek()  returns  the resulting
              offset location as measured in bytes from the beginning of the
              file.  On error, the value (off_t) -1 is returned and errno is
              set to indicate the error

However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a
value less than the passed offset, yet greater than zero.

For instance, here are example values observed from this call:

    offs = glfs_lseek(s->fd, start, SEEK_HOLE);
    if (offs < 0) {
        return -errno;          /* D1 and (H3 or H4) */
    }

start == 7608336384
offs == 7607877632

This causes QEMU to abort on the assert test.  When this value is
returned, errno is also 0.

This is a reported and known bug to glusterfs:
https://bugzilla.redhat.com/show_bug.cgi?id=1425293

Although this is being fixed in gluster, we still should work around it
in QEMU, given that multiple released versions of gluster behave this
way.

This patch treats the return case of (offs < start) the same as if an
error value other than ENXIO is returned; we will assume we learned
nothing, and there are no holes in the file.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/gluster.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 7c76cd0..c147909e 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
     if (offs < 0) {
         return -errno;          /* D3 or D4 */
     }
-    assert(offs >= start);
+
+    if (offs < start) {
+        /* This is not a valid return by lseek().  We are safe to just return
+         * -EIO in this case, and we'll treat it like D4. Unfortunately some
+         *  versions of libgfapi will return offs < start, so an assert here
+         *  will unneccesarily abort QEMU. */
+        return -EIO;
+    }
 
     if (offs > start) {
         /* D2: in hole, next data at offs */
@@ -1307,7 +1314,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
     if (offs < 0) {
         return -errno;          /* D1 and (H3 or H4) */
     }
-    assert(offs >= start);
+
+    if (offs < start) {
+        /* This is not a valid return by lseek().  We are safe to just return
+         * -EIO in this case, and we'll treat it like H4. Unfortunately some
+         *  versions of libgfapi will return offs < start, so an assert here
+         *  will unneccesarily abort QEMU. */
+        return -EIO;
+    }
 
     if (offs > start) {
         /*
-- 
2.9.3

On 05/23/2017 12:27 PM, Jeff Cody wrote:
> On current released versions of glusterfs, glfs_lseek() will sometimes
> return invalid values for SEEK_DATA or SEEK_HOLE.  For SEEK_DATA and
> SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in
> the case of error:
> 

> However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a
> value less than the passed offset, yet greater than zero.

Ouch.

> 
> Although this is being fixed in gluster, we still should work around it
> in QEMU, given that multiple released versions of gluster behave this
> way.

Fair enough.

> 
> This patch treats the return case of (offs < start) the same as if an
> error value other than ENXIO is returned; we will assume we learned
> nothing, and there are no holes in the file.

Yes, holes are merely an optimization, so treating bugs by the
pessimistic fallback of no holes rather than aborting is reasonable.


> +++ b/block/gluster.c
> @@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
>      if (offs < 0) {
>          return -errno;          /* D3 or D4 */
>      }
> -    assert(offs >= start);
> +
> +    if (offs < start) {
> +        /* This is not a valid return by lseek().  We are safe to just return
> +         * -EIO in this case, and we'll treat it like D4. Unfortunately some
> +         *  versions of libgfapi will return offs < start, so an assert here
> +         *  will unneccesarily abort QEMU. */

s/unneccesarily/unnecessarily/ (twice)

With spelling fix,
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Tue, May 23, 2017 at 01:27:50PM -0400, Jeff Cody wrote:
> On current released versions of glusterfs, glfs_lseek() will sometimes
> return invalid values for SEEK_DATA or SEEK_HOLE.  For SEEK_DATA and
> SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in
> the case of error:
> 
> LSEEK(2):
> 
>     off_t lseek(int fd, off_t offset, int whence);
> 
>     [...]
> 
>     SEEK_HOLE
>               Adjust  the file offset to the next hole in the file greater
>               than or equal to offset.  If offset points into the middle of
>               a hole, then the file offset is set to offset.  If there is no
>               hole past offset, then the file offset is adjusted to the end
>               of the file (i.e., there is  an implicit hole at the end of
>               any file).
> 
>     [...]
> 
>     RETURN VALUE
>               Upon  successful  completion,  lseek()  returns  the resulting
>               offset location as measured in bytes from the beginning of the
>               file.  On error, the value (off_t) -1 is returned and errno is
>               set to indicate the error
> 
> However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a
> value less than the passed offset, yet greater than zero.
> 
> For instance, here are example values observed from this call:
> 
>     offs = glfs_lseek(s->fd, start, SEEK_HOLE);
>     if (offs < 0) {
>         return -errno;          /* D1 and (H3 or H4) */
>     }
> 
> start == 7608336384
> offs == 7607877632
> 
> This causes QEMU to abort on the assert test.  When this value is
> returned, errno is also 0.
> 
> This is a reported and known bug to glusterfs:
> https://bugzilla.redhat.com/show_bug.cgi?id=1425293
> 
> Although this is being fixed in gluster, we still should work around it
> in QEMU, given that multiple released versions of gluster behave this
> way.

Versions of GlusterFS 3.8.0 - 3.8.8 are affected, 3.8.9 has the fix
already and was released in February this year. We encourage users to
update to recent versions, and provide a stable (bugfix only) update
each month.

The Red Hat Gluster Storage product that is often used in combination
with QEMU (+oVirt) does unfortunately not have an update where this is
fixed.

Using an unfixed Gluster storage environment can cause QEMU to segfault.
Although fixes exist for Gluster, not all users will have them
installed. Preventing the segfault in QEMU due to a broken storage
environment makes sense to me.

> This patch treats the return case of (offs < start) the same as if an
> error value other than ENXIO is returned; we will assume we learned
> nothing, and there are no holes in the file.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/gluster.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 7c76cd0..c147909e 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
>      if (offs < 0) {
>          return -errno;          /* D3 or D4 */
>      }
> -    assert(offs >= start);
> +
> +    if (offs < start) {
> +        /* This is not a valid return by lseek().  We are safe to just return
> +         * -EIO in this case, and we'll treat it like D4. Unfortunately some
> +         *  versions of libgfapi will return offs < start, so an assert here
> +         *  will unneccesarily abort QEMU. */

This is not really correct, the problem is not in libgfapi, but in the
protocol translator on the server-side. The version of libgfapi does not
matter here, it is the version on the server. But that might be too much
detail for the comment.

> +        return -EIO;
> +    }
>  
>      if (offs > start) {
>          /* D2: in hole, next data at offs */
> @@ -1307,7 +1314,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
>      if (offs < 0) {
>          return -errno;          /* D1 and (H3 or H4) */
>      }
> -    assert(offs >= start);
> +
> +    if (offs < start) {
> +        /* This is not a valid return by lseek().  We are safe to just return
> +         * -EIO in this case, and we'll treat it like H4. Unfortunately some
> +         *  versions of libgfapi will return offs < start, so an assert here
> +         *  will unneccesarily abort QEMU. */
> +        return -EIO;
> +    }
>  
>      if (offs > start) {
>          /*
> -- 
> 2.9.3
> 

You might want to explain the problem a little different in the commit
message. It is fine too if you think it would become too detailed, my
explanation is in the archives now in any case.

Reviewed-by: Niels de Vos <ndevos@redhat.com>

On Wed, May 24, 2017 at 11:02:02AM +0200, Niels de Vos wrote:
> On Tue, May 23, 2017 at 01:27:50PM -0400, Jeff Cody wrote:
> > On current released versions of glusterfs, glfs_lseek() will sometimes
> > return invalid values for SEEK_DATA or SEEK_HOLE.  For SEEK_DATA and
> > SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in
> > the case of error:
> > 
> > LSEEK(2):
> > 
> >     off_t lseek(int fd, off_t offset, int whence);
> > 
> >     [...]
> > 
> >     SEEK_HOLE
> >               Adjust  the file offset to the next hole in the file greater
> >               than or equal to offset.  If offset points into the middle of
> >               a hole, then the file offset is set to offset.  If there is no
> >               hole past offset, then the file offset is adjusted to the end
> >               of the file (i.e., there is  an implicit hole at the end of
> >               any file).
> > 
> >     [...]
> > 
> >     RETURN VALUE
> >               Upon  successful  completion,  lseek()  returns  the resulting
> >               offset location as measured in bytes from the beginning of the
> >               file.  On error, the value (off_t) -1 is returned and errno is
> >               set to indicate the error
> > 
> > However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a
> > value less than the passed offset, yet greater than zero.
> > 
> > For instance, here are example values observed from this call:
> > 
> >     offs = glfs_lseek(s->fd, start, SEEK_HOLE);
> >     if (offs < 0) {
> >         return -errno;          /* D1 and (H3 or H4) */
> >     }
> > 
> > start == 7608336384
> > offs == 7607877632
> > 
> > This causes QEMU to abort on the assert test.  When this value is
> > returned, errno is also 0.
> > 
> > This is a reported and known bug to glusterfs:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1425293
> > 
> > Although this is being fixed in gluster, we still should work around it
> > in QEMU, given that multiple released versions of gluster behave this
> > way.
> 
> Versions of GlusterFS 3.8.0 - 3.8.8 are affected, 3.8.9 has the fix
> already and was released in February this year. We encourage users to
> update to recent versions, and provide a stable (bugfix only) update
> each month.
> 
> The Red Hat Gluster Storage product that is often used in combination
> with QEMU (+oVirt) does unfortunately not have an update where this is
> fixed.
> 
> Using an unfixed Gluster storage environment can cause QEMU to segfault.
> Although fixes exist for Gluster, not all users will have them
> installed. Preventing the segfault in QEMU due to a broken storage
> environment makes sense to me.
> 
> > This patch treats the return case of (offs < start) the same as if an
> > error value other than ENXIO is returned; we will assume we learned
> > nothing, and there are no holes in the file.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/gluster.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/gluster.c b/block/gluster.c
> > index 7c76cd0..c147909e 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
> >      if (offs < 0) {
> >          return -errno;          /* D3 or D4 */
> >      }
> > -    assert(offs >= start);
> > +
> > +    if (offs < start) {
> > +        /* This is not a valid return by lseek().  We are safe to just return
> > +         * -EIO in this case, and we'll treat it like D4. Unfortunately some
> > +         *  versions of libgfapi will return offs < start, so an assert here
> > +         *  will unneccesarily abort QEMU. */
> 
> This is not really correct, the problem is not in libgfapi, but in the
> protocol translator on the server-side. The version of libgfapi does not
> matter here, it is the version on the server. But that might be too much
> detail for the comment.
> 
> > +        return -EIO;
> > +    }
> >  
> >      if (offs > start) {
> >          /* D2: in hole, next data at offs */
> > @@ -1307,7 +1314,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
> >      if (offs < 0) {
> >          return -errno;          /* D1 and (H3 or H4) */
> >      }
> > -    assert(offs >= start);
> > +
> > +    if (offs < start) {
> > +        /* This is not a valid return by lseek().  We are safe to just return
> > +         * -EIO in this case, and we'll treat it like H4. Unfortunately some
> > +         *  versions of libgfapi will return offs < start, so an assert here
> > +         *  will unneccesarily abort QEMU. */
> > +        return -EIO;
> > +    }
> >  
> >      if (offs > start) {
> >          /*
> > -- 
> > 2.9.3
> > 
> 
> You might want to explain the problem a little different in the commit
> message. It is fine too if you think it would become too detailed, my
> explanation is in the archives now in any case.


Thanks.  When applying it, I'll update the comments to mention glusterfs
server, and add the gluster version information you provided to the commit
message (and also fix the typo Eric pointed out).


> 
> Reviewed-by: Niels de Vos <ndevos@redhat.com>

On Wed, May 24, 2017 at 11:02:02AM +0200, Niels de Vos wrote:
> On Tue, May 23, 2017 at 01:27:50PM -0400, Jeff Cody wrote:
> > On current released versions of glusterfs, glfs_lseek() will sometimes
> > return invalid values for SEEK_DATA or SEEK_HOLE.  For SEEK_DATA and
> > SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in
> > the case of error:
> > 
> > LSEEK(2):
> > 
> >     off_t lseek(int fd, off_t offset, int whence);
> > 
> >     [...]
> > 
> >     SEEK_HOLE
> >               Adjust  the file offset to the next hole in the file greater
> >               than or equal to offset.  If offset points into the middle of
> >               a hole, then the file offset is set to offset.  If there is no
> >               hole past offset, then the file offset is adjusted to the end
> >               of the file (i.e., there is  an implicit hole at the end of
> >               any file).
> > 
> >     [...]
> > 
> >     RETURN VALUE
> >               Upon  successful  completion,  lseek()  returns  the resulting
> >               offset location as measured in bytes from the beginning of the
> >               file.  On error, the value (off_t) -1 is returned and errno is
> >               set to indicate the error
> > 
> > However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a
> > value less than the passed offset, yet greater than zero.
> > 
> > For instance, here are example values observed from this call:
> > 
> >     offs = glfs_lseek(s->fd, start, SEEK_HOLE);
> >     if (offs < 0) {
> >         return -errno;          /* D1 and (H3 or H4) */
> >     }
> > 
> > start == 7608336384
> > offs == 7607877632
> > 
> > This causes QEMU to abort on the assert test.  When this value is
> > returned, errno is also 0.
> > 
> > This is a reported and known bug to glusterfs:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1425293
> > 
> > Although this is being fixed in gluster, we still should work around it
> > in QEMU, given that multiple released versions of gluster behave this
> > way.
> 
> Versions of GlusterFS 3.8.0 - 3.8.8 are affected, 3.8.9 has the fix
> already and was released in February this year. We encourage users to
> update to recent versions, and provide a stable (bugfix only) update
> each month.

I am able to reproduce this bug on a server running glusterfs 3.11.0rc0.  Is
that expected?

> 
> The Red Hat Gluster Storage product that is often used in combination
> with QEMU (+oVirt) does unfortunately not have an update where this is
> fixed.
> 
> Using an unfixed Gluster storage environment can cause QEMU to segfault.
> Although fixes exist for Gluster, not all users will have them
> installed. Preventing the segfault in QEMU due to a broken storage
> environment makes sense to me.
> 
> > This patch treats the return case of (offs < start) the same as if an
> > error value other than ENXIO is returned; we will assume we learned
> > nothing, and there are no holes in the file.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> >  block/gluster.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/gluster.c b/block/gluster.c
> > index 7c76cd0..c147909e 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
> >      if (offs < 0) {
> >          return -errno;          /* D3 or D4 */
> >      }
> > -    assert(offs >= start);
> > +
> > +    if (offs < start) {
> > +        /* This is not a valid return by lseek().  We are safe to just return
> > +         * -EIO in this case, and we'll treat it like D4. Unfortunately some
> > +         *  versions of libgfapi will return offs < start, so an assert here
> > +         *  will unneccesarily abort QEMU. */
> 
> This is not really correct, the problem is not in libgfapi, but in the
> protocol translator on the server-side. The version of libgfapi does not
> matter here, it is the version on the server. But that might be too much
> detail for the comment.
> 
> > +        return -EIO;
> > +    }
> >  
> >      if (offs > start) {
> >          /* D2: in hole, next data at offs */
> > @@ -1307,7 +1314,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
> >      if (offs < 0) {
> >          return -errno;          /* D1 and (H3 or H4) */
> >      }
> > -    assert(offs >= start);
> > +
> > +    if (offs < start) {
> > +        /* This is not a valid return by lseek().  We are safe to just return
> > +         * -EIO in this case, and we'll treat it like H4. Unfortunately some
> > +         *  versions of libgfapi will return offs < start, so an assert here
> > +         *  will unneccesarily abort QEMU. */
> > +        return -EIO;
> > +    }
> >  
> >      if (offs > start) {
> >          /*
> > -- 
> > 2.9.3
> > 
> 
> You might want to explain the problem a little different in the commit
> message. It is fine too if you think it would become too detailed, my
> explanation is in the archives now in any case.
> 
> Reviewed-by: Niels de Vos <ndevos@redhat.com>

On Wed, May 24, 2017 at 04:50:03PM -0400, Jeff Cody wrote:
> On Wed, May 24, 2017 at 11:02:02AM +0200, Niels de Vos wrote:
> > On Tue, May 23, 2017 at 01:27:50PM -0400, Jeff Cody wrote:
> > > On current released versions of glusterfs, glfs_lseek() will sometimes
> > > return invalid values for SEEK_DATA or SEEK_HOLE.  For SEEK_DATA and
> > > SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in
> > > the case of error:
> > > 
> > > LSEEK(2):
> > > 
> > >     off_t lseek(int fd, off_t offset, int whence);
> > > 
> > >     [...]
> > > 
> > >     SEEK_HOLE
> > >               Adjust  the file offset to the next hole in the file greater
> > >               than or equal to offset.  If offset points into the middle of
> > >               a hole, then the file offset is set to offset.  If there is no
> > >               hole past offset, then the file offset is adjusted to the end
> > >               of the file (i.e., there is  an implicit hole at the end of
> > >               any file).
> > > 
> > >     [...]
> > > 
> > >     RETURN VALUE
> > >               Upon  successful  completion,  lseek()  returns  the resulting
> > >               offset location as measured in bytes from the beginning of the
> > >               file.  On error, the value (off_t) -1 is returned and errno is
> > >               set to indicate the error
> > > 
> > > However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a
> > > value less than the passed offset, yet greater than zero.
> > > 
> > > For instance, here are example values observed from this call:
> > > 
> > >     offs = glfs_lseek(s->fd, start, SEEK_HOLE);
> > >     if (offs < 0) {
> > >         return -errno;          /* D1 and (H3 or H4) */
> > >     }
> > > 
> > > start == 7608336384
> > > offs == 7607877632
> > > 
> > > This causes QEMU to abort on the assert test.  When this value is
> > > returned, errno is also 0.
> > > 
> > > This is a reported and known bug to glusterfs:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1425293
> > > 
> > > Although this is being fixed in gluster, we still should work around it
> > > in QEMU, given that multiple released versions of gluster behave this
> > > way.
> > 
> > Versions of GlusterFS 3.8.0 - 3.8.8 are affected, 3.8.9 has the fix
> > already and was released in February this year. We encourage users to
> > update to recent versions, and provide a stable (bugfix only) update
> > each month.
> 
> I am able to reproduce this bug on a server running glusterfs 3.11.0rc0.  Is
> that expected?

No, that really is not expected. The backport that was reported to fix
it for a 3.8.4 version is definitely part of 3.11 already. Could you
pass me some details about your Gluster environment and volume, either
by email or in a bug? I'll try to reproduce and debug it from the
Gluster side.

There is a holiday tomorrow (I'm in The Netherlands), and I'm travelling
the whole weekend. I might not be able to look into this before next
week. Sorry about that!

Thanks,
Niels


> 
> > 
> > The Red Hat Gluster Storage product that is often used in combination
> > with QEMU (+oVirt) does unfortunately not have an update where this is
> > fixed.
> > 
> > Using an unfixed Gluster storage environment can cause QEMU to segfault.
> > Although fixes exist for Gluster, not all users will have them
> > installed. Preventing the segfault in QEMU due to a broken storage
> > environment makes sense to me.
> > 
> > > This patch treats the return case of (offs < start) the same as if an
> > > error value other than ENXIO is returned; we will assume we learned
> > > nothing, and there are no holes in the file.
> > > 
> > > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > > ---
> > >  block/gluster.c | 18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/block/gluster.c b/block/gluster.c
> > > index 7c76cd0..c147909e 100644
> > > --- a/block/gluster.c
> > > +++ b/block/gluster.c
> > > @@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
> > >      if (offs < 0) {
> > >          return -errno;          /* D3 or D4 */
> > >      }
> > > -    assert(offs >= start);
> > > +
> > > +    if (offs < start) {
> > > +        /* This is not a valid return by lseek().  We are safe to just return
> > > +         * -EIO in this case, and we'll treat it like D4. Unfortunately some
> > > +         *  versions of libgfapi will return offs < start, so an assert here
> > > +         *  will unneccesarily abort QEMU. */
> > 
> > This is not really correct, the problem is not in libgfapi, but in the
> > protocol translator on the server-side. The version of libgfapi does not
> > matter here, it is the version on the server. But that might be too much
> > detail for the comment.
> > 
> > > +        return -EIO;
> > > +    }
> > >  
> > >      if (offs > start) {
> > >          /* D2: in hole, next data at offs */
> > > @@ -1307,7 +1314,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
> > >      if (offs < 0) {
> > >          return -errno;          /* D1 and (H3 or H4) */
> > >      }
> > > -    assert(offs >= start);
> > > +
> > > +    if (offs < start) {
> > > +        /* This is not a valid return by lseek().  We are safe to just return
> > > +         * -EIO in this case, and we'll treat it like H4. Unfortunately some
> > > +         *  versions of libgfapi will return offs < start, so an assert here
> > > +         *  will unneccesarily abort QEMU. */
> > > +        return -EIO;
> > > +    }
> > >  
> > >      if (offs > start) {
> > >          /*
> > > -- 
> > > 2.9.3
> > > 
> > 
> > You might want to explain the problem a little different in the commit
> > message. It is fine too if you think it would become too detailed, my
> > explanation is in the archives now in any case.
> > 
> > Reviewed-by: Niels de Vos <ndevos@redhat.com>

On Tue, May 23, 2017 at 01:27:50PM -0400, Jeff Cody wrote:
> On current released versions of glusterfs, glfs_lseek() will sometimes
> return invalid values for SEEK_DATA or SEEK_HOLE.  For SEEK_DATA and
> SEEK_HOLE, the returned value should be >= the passed offset, or < 0 in
> the case of error:
> 
> LSEEK(2):
> 
>     off_t lseek(int fd, off_t offset, int whence);
> 
>     [...]
> 
>     SEEK_HOLE
>               Adjust  the file offset to the next hole in the file greater
>               than or equal to offset.  If offset points into the middle of
>               a hole, then the file offset is set to offset.  If there is no
>               hole past offset, then the file offset is adjusted to the end
>               of the file (i.e., there is  an implicit hole at the end of
>               any file).
> 
>     [...]
> 
>     RETURN VALUE
>               Upon  successful  completion,  lseek()  returns  the resulting
>               offset location as measured in bytes from the beginning of the
>               file.  On error, the value (off_t) -1 is returned and errno is
>               set to indicate the error
> 
> However, occasionally glfs_lseek() for SEEK_HOLE/DATA will return a
> value less than the passed offset, yet greater than zero.
> 
> For instance, here are example values observed from this call:
> 
>     offs = glfs_lseek(s->fd, start, SEEK_HOLE);
>     if (offs < 0) {
>         return -errno;          /* D1 and (H3 or H4) */
>     }
> 
> start == 7608336384
> offs == 7607877632
> 
> This causes QEMU to abort on the assert test.  When this value is
> returned, errno is also 0.
> 
> This is a reported and known bug to glusterfs:
> https://bugzilla.redhat.com/show_bug.cgi?id=1425293
> 
> Although this is being fixed in gluster, we still should work around it
> in QEMU, given that multiple released versions of gluster behave this
> way.
> 
> This patch treats the return case of (offs < start) the same as if an
> error value other than ENXIO is returned; we will assume we learned
> nothing, and there are no holes in the file.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>  block/gluster.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index 7c76cd0..c147909e 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -1275,7 +1275,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
>      if (offs < 0) {
>          return -errno;          /* D3 or D4 */
>      }
> -    assert(offs >= start);
> +
> +    if (offs < start) {
> +        /* This is not a valid return by lseek().  We are safe to just return
> +         * -EIO in this case, and we'll treat it like D4. Unfortunately some
> +         *  versions of libgfapi will return offs < start, so an assert here
> +         *  will unneccesarily abort QEMU. */
> +        return -EIO;
> +    }
>  
>      if (offs > start) {
>          /* D2: in hole, next data at offs */
> @@ -1307,7 +1314,14 @@ static int find_allocation(BlockDriverState *bs, off_t start,
>      if (offs < 0) {
>          return -errno;          /* D1 and (H3 or H4) */
>      }
> -    assert(offs >= start);
> +
> +    if (offs < start) {
> +        /* This is not a valid return by lseek().  We are safe to just return
> +         * -EIO in this case, and we'll treat it like H4. Unfortunately some
> +         *  versions of libgfapi will return offs < start, so an assert here
> +         *  will unneccesarily abort QEMU. */
> +        return -EIO;
> +    }
>  
>      if (offs > start) {
>          /*
> -- 
> 2.9.3
> 

Thanks,

Applied (with comment fixes) to my block branch:

git://github.com/codyprime/qemu-kvm-jtc block

-Jeff