[Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Niels de Vos]

From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
function that returns additional 'struct stat' structures to enable
advanced caching of attributes. This is useful for file servers, not so
much for QEMU. Nevertheless, the API has changed and needs to be
adopted.

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Signed-off-by: Niels de Vos <ndevos@redhat.com>

---
v2: do a compile check as suggested by Eric Blake
v3: define old backwards compatible glfs_ftruncate() macro, from Eric Blake
---
 block/gluster.c | 11 +++++++++--
 configure       | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/block/gluster.c b/block/gluster.c
index 4fd55a9cc5..9bd6525b41 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -20,6 +20,10 @@
 #include "qemu/option.h"
 #include "qemu/cutils.h"
 
+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
+# define glfs_ftruncate(fd, offset, _u1, _u2) glfs_ftruncate(fd, offset)
+#endif
+
 #define GLUSTER_OPT_FILENAME        "filename"
 #define GLUSTER_OPT_VOLUME          "volume"
 #define GLUSTER_OPT_PATH            "path"
@@ -997,6 +1001,7 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
                                     PreallocMode prealloc, Error **errp)
 {
     int64_t current_length;
+    int ret;
 
     current_length = glfs_lseek(fd, 0, SEEK_END);
     if (current_length < 0) {
@@ -1024,7 +1029,8 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
 #endif /* CONFIG_GLUSTERFS_FALLOCATE */
 #ifdef CONFIG_GLUSTERFS_ZEROFILL
     case PREALLOC_MODE_FULL:
-        if (glfs_ftruncate(fd, offset)) {
+        ret = glfs_ftruncate(fd, offset, NULL, NULL);
+        if (ret) {
             error_setg_errno(errp, errno, "Could not resize file");
             return -errno;
         }
@@ -1035,7 +1041,8 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
         break;
 #endif /* CONFIG_GLUSTERFS_ZEROFILL */
     case PREALLOC_MODE_OFF:
-        if (glfs_ftruncate(fd, offset)) {
+        ret = glfs_ftruncate(fd, offset, NULL, NULL);
+        if (ret) {
             error_setg_errno(errp, errno, "Could not resize file");
             return -errno;
         }
diff --git a/configure b/configure
index 2a7796ea80..f3c0918d6b 100755
--- a/configure
+++ b/configure
@@ -451,6 +451,7 @@ glusterfs_xlator_opt="no"
 glusterfs_discard="no"
 glusterfs_fallocate="no"
 glusterfs_zerofill="no"
+glusterfs_legacy_ftruncate="no"
 gtk=""
 gtkabi=""
 gtk_gl="no"
@@ -3947,6 +3948,19 @@ if test "$glusterfs" != "no" ; then
       glusterfs_fallocate="yes"
       glusterfs_zerofill="yes"
     fi
+    cat > $TMPC << EOF
+#include <glusterfs/api/glfs.h>
+
+int
+main(void)
+{
+	/* new glfs_ftruncate() passes two additional args */
+	return glfs_ftruncate(NULL, 0 /*, NULL, NULL */);
+}
+EOF
+    if compile_prog "$glusterfs_cflags" "$glusterfs_libs" ; then
+      glusterfs_legacy_ftruncate="yes"
+    fi
   else
     if test "$glusterfs" = "yes" ; then
       feature_not_found "GlusterFS backend support" \
@@ -6644,6 +6658,10 @@ if test "$glusterfs_zerofill" = "yes" ; then
   echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
 fi
 
+if test "$glusterfs_legacy_ftruncate" = "yes" ; then
+  echo "CONFIG_GLUSTERFS_LEGACY_FTRUNCATE=y" >> $config_host_mak
+fi
+
 if test "$libssh2" = "yes" ; then
   echo "CONFIG_LIBSSH2=m" >> $config_host_mak
   echo "LIBSSH2_CFLAGS=$libssh2_cflags" >> $config_host_mak
-- 
2.17.1

Re: [Qemu-devel] [PATCH v3 for-3.0] block/gluster: Handle changed glfs_ftruncate signature

[Eric Blake]

On 07/27/2018 03:19 AM, Niels de Vos wrote:
> From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> 
> New versions of Glusters libgfapi.so have an updated glfs_ftruncate()

s/Glusters/Gluster's/

> function that returns additional 'struct stat' structures to enable
> advanced caching of attributes. This is useful for file servers, not so
> much for QEMU. Nevertheless, the API has changed and needs to be
> adopted.
> 
> Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> Signed-off-by: Niels de Vos <ndevos@redhat.com>
> 
> ---
> v2: do a compile check as suggested by Eric Blake
> v3: define old backwards compatible glfs_ftruncate() macro, from Eric Blake
> ---
>   block/gluster.c | 11 +++++++++--
>   configure       | 18 ++++++++++++++++++
>   2 files changed, 27 insertions(+), 2 deletions(-)

Seems reasonable to get into 3.0 if we still can (for fewer platforms 
with failed builds); but not the end of the world if it slips into 3.1.

> @@ -997,6 +1001,7 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
>                                       PreallocMode prealloc, Error **errp)
>   {
>       int64_t current_length;
> +    int ret;
>   
>       current_length = glfs_lseek(fd, 0, SEEK_END);
>       if (current_length < 0) {
> @@ -1024,7 +1029,8 @@ static int qemu_gluster_do_truncate(struct glfs_fd *fd, int64_t offset,
>   #endif /* CONFIG_GLUSTERFS_FALLOCATE */
>   #ifdef CONFIG_GLUSTERFS_ZEROFILL
>       case PREALLOC_MODE_FULL:
> -        if (glfs_ftruncate(fd, offset)) {
> +        ret = glfs_ftruncate(fd, offset, NULL, NULL);
> +        if (ret) {

Adding 'ret' is not strictly necessary (checking the return directly in 
the 'if' still works), but doesn't hurt. So,

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Eric Blake]

On 07/27/2018 03:19 AM, Niels de Vos wrote:
> From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> 
> New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
> function that returns additional 'struct stat' structures to enable
> advanced caching of attributes. This is useful for file servers, not so
> much for QEMU. Nevertheless, the API has changed and needs to be
> adopted.
> 

Oh, one other comment.

> +++ b/block/gluster.c
> @@ -20,6 +20,10 @@
>   #include "qemu/option.h"
>   #include "qemu/cutils.h"
>   
> +#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> +# define glfs_ftruncate(fd, offset, _u1, _u2) glfs_ftruncate(fd, offset)
> +#endif

Someday, when we can assume new enough gluster everywhere, we can drop 
this hunk...

> +++ b/configure

> +	/* new glfs_ftruncate() passes two additional args */
> +	return glfs_ftruncate(NULL, 0 /*, NULL, NULL */);
> +}
> +EOF
> +    if compile_prog "$glusterfs_cflags" "$glusterfs_libs" ; then
> +      glusterfs_legacy_ftruncate="yes"
> +    fi

...but it will be easier to remember to do so if this comment in 
configure calls out the upstream gluster version that no longer requires 
the legacy workaround, as our hint for when...

>     else
>       if test "$glusterfs" = "yes" ; then
>         feature_not_found "GlusterFS backend support" \
> @@ -6644,6 +6658,10 @@ if test "$glusterfs_zerofill" = "yes" ; then
>     echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
>   fi
>   
> +if test "$glusterfs_legacy_ftruncate" = "yes" ; then
> +  echo "CONFIG_GLUSTERFS_LEGACY_FTRUNCATE=y" >> $config_host_mak

...this #define is no longer necessary.

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

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Jeff Cody]

On Fri, Jul 27, 2018 at 08:24:05AM -0500, Eric Blake wrote:
> On 07/27/2018 03:19 AM, Niels de Vos wrote:
> >From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> >
> >New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
> >function that returns additional 'struct stat' structures to enable
> >advanced caching of attributes. This is useful for file servers, not so
> >much for QEMU. Nevertheless, the API has changed and needs to be
> >adopted.
> >
> 
> Oh, one other comment.
> 
> >+++ b/block/gluster.c
> >@@ -20,6 +20,10 @@
> >  #include "qemu/option.h"
> >  #include "qemu/cutils.h"
> >+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> >+# define glfs_ftruncate(fd, offset, _u1, _u2) glfs_ftruncate(fd, offset)
> >+#endif
> 
> Someday, when we can assume new enough gluster everywhere, we can drop this
> hunk...
> 

Part of me wishes that libgfapi had just created a new function
'glfs_ftruncate2', so that existing users don't need to handle the api
change.  But I guess in the grand scheme, not a huge deal either way.

> >+++ b/configure
> 
> >+	/* new glfs_ftruncate() passes two additional args */
> >+	return glfs_ftruncate(NULL, 0 /*, NULL, NULL */);
> >+}
> >+EOF
> >+    if compile_prog "$glusterfs_cflags" "$glusterfs_libs" ; then
> >+      glusterfs_legacy_ftruncate="yes"
> >+    fi
> 
> ...but it will be easier to remember to do so if this comment in configure
> calls out the upstream gluster version that no longer requires the legacy
> workaround, as our hint for when...
> 

I can go ahead and add that to the comment in my branch after applying, if
Niels can let me know what that version is/will be (if known).

> >    else
> >      if test "$glusterfs" = "yes" ; then
> >        feature_not_found "GlusterFS backend support" \
> >@@ -6644,6 +6658,10 @@ if test "$glusterfs_zerofill" = "yes" ; then
> >    echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
> >  fi
> >+if test "$glusterfs_legacy_ftruncate" = "yes" ; then
> >+  echo "CONFIG_GLUSTERFS_LEGACY_FTRUNCATE=y" >> $config_host_mak
> 
> ...this #define is no longer necessary.
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Niels de Vos]

On Sat, Jul 28, 2018 at 12:18:39AM -0400, Jeff Cody wrote:
> On Fri, Jul 27, 2018 at 08:24:05AM -0500, Eric Blake wrote:
> > On 07/27/2018 03:19 AM, Niels de Vos wrote:
> > >From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > >
> > >New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
> > >function that returns additional 'struct stat' structures to enable
> > >advanced caching of attributes. This is useful for file servers, not so
> > >much for QEMU. Nevertheless, the API has changed and needs to be
> > >adopted.
> > >
> > 
> > Oh, one other comment.
> > 
> > >+++ b/block/gluster.c
> > >@@ -20,6 +20,10 @@
> > >  #include "qemu/option.h"
> > >  #include "qemu/cutils.h"
> > >+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> > >+# define glfs_ftruncate(fd, offset, _u1, _u2) glfs_ftruncate(fd, offset)
> > >+#endif
> > 
> > Someday, when we can assume new enough gluster everywhere, we can drop this
> > hunk...
> > 
> 
> Part of me wishes that libgfapi had just created a new function
> 'glfs_ftruncate2', so that existing users don't need to handle the api
> change.  But I guess in the grand scheme, not a huge deal either way.

Gluster uses versioned symbols, so older binaries will keep working with
new libraries. It is (hopefully) rare that existing symbols get updated.
We try to send patches for these kind of changes to the projects we know
well in advance, reducing the number of surprises.

> > >+++ b/configure
> > 
> > >+	/* new glfs_ftruncate() passes two additional args */
> > >+	return glfs_ftruncate(NULL, 0 /*, NULL, NULL */);
> > >+}
> > >+EOF
> > >+    if compile_prog "$glusterfs_cflags" "$glusterfs_libs" ; then
> > >+      glusterfs_legacy_ftruncate="yes"
> > >+    fi
> > 
> > ...but it will be easier to remember to do so if this comment in configure
> > calls out the upstream gluster version that no longer requires the legacy
> > workaround, as our hint for when...
> > 
> 
> I can go ahead and add that to the comment in my branch after applying, if
> Niels can let me know what that version is/will be (if known).

The new glfs_ftruncate() will be part of glusterfs-5 (planned for
October). We're changing the numbering scheme, it was expected to come
in glusterfs-4.2, but that is a version that never will be released.

Thanks for correcting the last bits of the patch!
Niels


> > >    else
> > >      if test "$glusterfs" = "yes" ; then
> > >        feature_not_found "GlusterFS backend support" \
> > >@@ -6644,6 +6658,10 @@ if test "$glusterfs_zerofill" = "yes" ; then
> > >    echo "CONFIG_GLUSTERFS_ZEROFILL=y" >> $config_host_mak
> > >  fi
> > >+if test "$glusterfs_legacy_ftruncate" = "yes" ; then
> > >+  echo "CONFIG_GLUSTERFS_LEGACY_FTRUNCATE=y" >> $config_host_mak
> > 
> > ...this #define is no longer necessary.
> > 
> > -- 
> > Eric Blake, Principal Software Engineer
> > Red Hat, Inc.           +1-919-301-3266
> > Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Eric Blake]

On 07/28/2018 02:50 AM, Niels de Vos wrote:
>>
>> Part of me wishes that libgfapi had just created a new function
>> 'glfs_ftruncate2', so that existing users don't need to handle the api
>> change.  But I guess in the grand scheme, not a huge deal either way.
> 
> Gluster uses versioned symbols, so older binaries will keep working with
> new libraries. It is (hopefully) rare that existing symbols get updated.
> We try to send patches for these kind of changes to the projects we know
> well in advance, reducing the number of surprises.

>> I can go ahead and add that to the comment in my branch after applying, if
>> Niels can let me know what that version is/will be (if known).
> 
> The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> October). We're changing the numbering scheme, it was expected to come
> in glusterfs-4.2, but that is a version that never will be released.
> 

Wait - so you're saying gluster has not yet released the incompatible 
change? Now would be the right time to get rid of the API breakage, 
before you bake it in, rather than relying solely on the versioned 
symbols to avoid an ABI breakage but forcing all clients to compensate 
to the API breakage.

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

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Jeff Cody]

On Mon, Jul 30, 2018 at 10:07:27AM -0500, Eric Blake wrote:
> On 07/28/2018 02:50 AM, Niels de Vos wrote:
> >>
> >>Part of me wishes that libgfapi had just created a new function
> >>'glfs_ftruncate2', so that existing users don't need to handle the api
> >>change.  But I guess in the grand scheme, not a huge deal either way.
> >
> >Gluster uses versioned symbols, so older binaries will keep working with
> >new libraries. It is (hopefully) rare that existing symbols get updated.
> >We try to send patches for these kind of changes to the projects we know
> >well in advance, reducing the number of surprises.
> 
> >>I can go ahead and add that to the comment in my branch after applying, if
> >>Niels can let me know what that version is/will be (if known).
> >
> >The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> >October). We're changing the numbering scheme, it was expected to come
> >in glusterfs-4.2, but that is a version that never will be released.
> >
> 
> Wait - so you're saying gluster has not yet released the incompatible
> change? Now would be the right time to get rid of the API breakage, before
> you bake it in, rather than relying solely on the versioned symbols to avoid
> an ABI breakage but forcing all clients to compensate to the API breakage.
> 

If this is not yet in a released version of Gluster, I'm not real eager to
pollute the QEMU driver codebase with #ifdef's, especially if there is a
possibility the API change may not actually materialize.

Is there any reason that this change is being approached in a way that
breaks API usage, and is it too late in the Gluster development pipeline to
change that?

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Niels de Vos]

On Mon, Jul 30, 2018 at 03:27:29PM -0400, Jeff Cody wrote:
> On Mon, Jul 30, 2018 at 10:07:27AM -0500, Eric Blake wrote:
> > On 07/28/2018 02:50 AM, Niels de Vos wrote:
> > >>
> > >>Part of me wishes that libgfapi had just created a new function
> > >>'glfs_ftruncate2', so that existing users don't need to handle the api
> > >>change.  But I guess in the grand scheme, not a huge deal either way.
> > >
> > >Gluster uses versioned symbols, so older binaries will keep working with
> > >new libraries. It is (hopefully) rare that existing symbols get updated.
> > >We try to send patches for these kind of changes to the projects we know
> > >well in advance, reducing the number of surprises.
> > 
> > >>I can go ahead and add that to the comment in my branch after applying, if
> > >>Niels can let me know what that version is/will be (if known).
> > >
> > >The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> > >October). We're changing the numbering scheme, it was expected to come
> > >in glusterfs-4.2, but that is a version that never will be released.
> > >
> > 
> > Wait - so you're saying gluster has not yet released the incompatible
> > change? Now would be the right time to get rid of the API breakage, before
> > you bake it in, rather than relying solely on the versioned symbols to avoid
> > an ABI breakage but forcing all clients to compensate to the API breakage.
> > 
> 
> If this is not yet in a released version of Gluster, I'm not real eager to
> pollute the QEMU driver codebase with #ifdef's, especially if there is a
> possibility the API change may not actually materialize.
> 
> Is there any reason that this change is being approached in a way that
> breaks API usage, and is it too late in the Gluster development pipeline to
> change that?

There recently have been a few changes like this in libgfapi. These have
been introduced to improve performance in common use-cases where an
updated 'struct stat' is needed after an operation. Some functions have
been adapted in previous releases, glfs_ftruncate() landed too late for
that. I hope we'll get a complete coherent API with glusterfs-5 again.

For QEMU this means additional changes will come related to
glfs_fallocate(), glfs_zerofill() and probably more. The current
glusterfs-4.1 version will be maintained for one year, after which the
detection/#ifdef can be removed as the than maintained versions should
all have the updated API. I'm sorry for the inconvenience that this
causes.

If you prefer, I can wait with sending patches for QEMU with future
Gluster releases until additional changes have landed in libgfapi.

Thanks,
Niels

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Jeff Cody]

On Tue, Jul 31, 2018 at 11:18:02AM +0200, Niels de Vos wrote:
> On Mon, Jul 30, 2018 at 03:27:29PM -0400, Jeff Cody wrote:
> > On Mon, Jul 30, 2018 at 10:07:27AM -0500, Eric Blake wrote:
> > > On 07/28/2018 02:50 AM, Niels de Vos wrote:
> > > >>
> > > >>Part of me wishes that libgfapi had just created a new function
> > > >>'glfs_ftruncate2', so that existing users don't need to handle the api
> > > >>change.  But I guess in the grand scheme, not a huge deal either way.
> > > >
> > > >Gluster uses versioned symbols, so older binaries will keep working with
> > > >new libraries. It is (hopefully) rare that existing symbols get updated.
> > > >We try to send patches for these kind of changes to the projects we know
> > > >well in advance, reducing the number of surprises.
> > > 
> > > >>I can go ahead and add that to the comment in my branch after applying, if
> > > >>Niels can let me know what that version is/will be (if known).
> > > >
> > > >The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> > > >October). We're changing the numbering scheme, it was expected to come
> > > >in glusterfs-4.2, but that is a version that never will be released.
> > > >
> > > 
> > > Wait - so you're saying gluster has not yet released the incompatible
> > > change? Now would be the right time to get rid of the API breakage, before
> > > you bake it in, rather than relying solely on the versioned symbols to avoid
> > > an ABI breakage but forcing all clients to compensate to the API breakage.
> > > 
> > 
> > If this is not yet in a released version of Gluster, I'm not real eager to
> > pollute the QEMU driver codebase with #ifdef's, especially if there is a
> > possibility the API change may not actually materialize.
> > 
> > Is there any reason that this change is being approached in a way that
> > breaks API usage, and is it too late in the Gluster development pipeline to
> > change that?
> 
> There recently have been a few changes like this in libgfapi. These have
> been introduced to improve performance in common use-cases where an
> updated 'struct stat' is needed after an operation. Some functions have
> been adapted in previous releases, glfs_ftruncate() landed too late for
> that. I hope we'll get a complete coherent API with glusterfs-5 again.
> 

Am I correct in assuming the API changes that happened in previous libgfapi
release are for APIs that QEMU does not use (i.e. no action needed from
us?)

> For QEMU this means additional changes will come related to
> glfs_fallocate(), glfs_zerofill() and probably more. The current
> glusterfs-4.1 version will be maintained for one year, after which the
> detection/#ifdef can be removed as the than maintained versions should
> all have the updated API. I'm sorry for the inconvenience that this
> causes.
> 

Understood.  I don't want to make a huge deal out of it.  I guess my
recommendation/request is that API enhancements happen in a way that don't
break existing APIs (e.g. use parallel APIs), because QEMU version usage and
supported glusterfs usage may not always follow supported versions in the
wild.  I know versioned symbols helps with this, but it still complicates
the code (and couples QEMU's code to gluster support windows).


> If you prefer, I can wait with sending patches for QEMU with future
> Gluster releases until additional changes have landed in libgfapi.
>

I think generally, we don't want to start introducing #ifdef's and new APi
usage for unreleased versions of libgfapi if possible (as unreleased APIs,
it could technically change, and then QEMU releases would have 'dead' API
support in it).

If glusterfs-5 releases in October, that may line up with 3.1 or 3.2.


-Jeff

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Niels de Vos]

On Tue, Jul 31, 2018 at 03:51:22PM -0400, Jeff Cody wrote:
> On Tue, Jul 31, 2018 at 11:18:02AM +0200, Niels de Vos wrote:
> > On Mon, Jul 30, 2018 at 03:27:29PM -0400, Jeff Cody wrote:
> > > On Mon, Jul 30, 2018 at 10:07:27AM -0500, Eric Blake wrote:
> > > > On 07/28/2018 02:50 AM, Niels de Vos wrote:
> > > > >>
> > > > >>Part of me wishes that libgfapi had just created a new function
> > > > >>'glfs_ftruncate2', so that existing users don't need to handle the api
> > > > >>change.  But I guess in the grand scheme, not a huge deal either way.
> > > > >
> > > > >Gluster uses versioned symbols, so older binaries will keep working with
> > > > >new libraries. It is (hopefully) rare that existing symbols get updated.
> > > > >We try to send patches for these kind of changes to the projects we know
> > > > >well in advance, reducing the number of surprises.
> > > > 
> > > > >>I can go ahead and add that to the comment in my branch after applying, if
> > > > >>Niels can let me know what that version is/will be (if known).
> > > > >
> > > > >The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> > > > >October). We're changing the numbering scheme, it was expected to come
> > > > >in glusterfs-4.2, but that is a version that never will be released.
> > > > >
> > > > 
> > > > Wait - so you're saying gluster has not yet released the incompatible
> > > > change? Now would be the right time to get rid of the API breakage, before
> > > > you bake it in, rather than relying solely on the versioned symbols to avoid
> > > > an ABI breakage but forcing all clients to compensate to the API breakage.
> > > > 
> > > 
> > > If this is not yet in a released version of Gluster, I'm not real eager to
> > > pollute the QEMU driver codebase with #ifdef's, especially if there is a
> > > possibility the API change may not actually materialize.
> > > 
> > > Is there any reason that this change is being approached in a way that
> > > breaks API usage, and is it too late in the Gluster development pipeline to
> > > change that?
> > 
> > There recently have been a few changes like this in libgfapi. These have
> > been introduced to improve performance in common use-cases where an
> > updated 'struct stat' is needed after an operation. Some functions have
> > been adapted in previous releases, glfs_ftruncate() landed too late for
> > that. I hope we'll get a complete coherent API with glusterfs-5 again.
> > 
> 
> Am I correct in assuming the API changes that happened in previous libgfapi
> release are for APIs that QEMU does not use (i.e. no action needed from
> us?)

Yes, that is correct.

> > For QEMU this means additional changes will come related to
> > glfs_fallocate(), glfs_zerofill() and probably more. The current
> > glusterfs-4.1 version will be maintained for one year, after which the
> > detection/#ifdef can be removed as the than maintained versions should
> > all have the updated API. I'm sorry for the inconvenience that this
> > causes.
> > 
> 
> Understood.  I don't want to make a huge deal out of it.  I guess my
> recommendation/request is that API enhancements happen in a way that don't
> break existing APIs (e.g. use parallel APIs), because QEMU version usage and
> supported glusterfs usage may not always follow supported versions in the
> wild.  I know versioned symbols helps with this, but it still complicates
> the code (and couples QEMU's code to gluster support windows).

Once the last Gluster version that has the old API goes out of
maintenance, we will remove the legacy functions. At that point, we can
also provide a new patch to QEMU (and other projcets) that removes the
compatibility #ifdef's.

> > If you prefer, I can wait with sending patches for QEMU with future
> > Gluster releases until additional changes have landed in libgfapi.
> >
> 
> I think generally, we don't want to start introducing #ifdef's and new APi
> usage for unreleased versions of libgfapi if possible (as unreleased APIs,
> it could technically change, and then QEMU releases would have 'dead' API
> support in it).
> 
> If glusterfs-5 releases in October, that may line up with 3.1 or 3.2.

Ok, I'll keep these kind of patches in my tree as work-in-progress and
when the glusterfs-5 gets tagged for alpha/beta send it again.

Even if not optimal, would you accept this as a way going forward?

Thanks,
Niels

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Eric Blake]

On 08/01/2018 06:47 AM, Niels de Vos wrote:
>> If glusterfs-5 releases in October, that may line up with 3.1 or 3.2.
> 
> Ok, I'll keep these kind of patches in my tree as work-in-progress and
> when the glusterfs-5 gets tagged for alpha/beta send it again.
> 
> Even if not optimal, would you accept this as a way going forward?

It will work. But even better would be getting gluster to the point that 
it has a stable back-compat API guarantee (no future API removals or 
signature changes on existing functions - all new functionality is added 
via new API).  The more clients a library gains, the more pain you cause 
yourself and all your clients when you choose not to maintain a 
backwards-compatible stable API.

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

Re: [Qemu-devel] [PATCH v3] block/gluster: Handle changed glfs_ftruncate signature

[Jeff Cody]

On Sat, Jul 28, 2018 at 09:50:05AM +0200, Niels de Vos wrote:
> On Sat, Jul 28, 2018 at 12:18:39AM -0400, Jeff Cody wrote:
> > On Fri, Jul 27, 2018 at 08:24:05AM -0500, Eric Blake wrote:
> > > On 07/27/2018 03:19 AM, Niels de Vos wrote:
> > > >From: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
> > > >
> > > >New versions of Glusters libgfapi.so have an updated glfs_ftruncate()
> > > >function that returns additional 'struct stat' structures to enable
> > > >advanced caching of attributes. This is useful for file servers, not so
> > > >much for QEMU. Nevertheless, the API has changed and needs to be
> > > >adopted.
> > > >
> > > 
> > > Oh, one other comment.
> > > 
> > > >+++ b/block/gluster.c
> > > >@@ -20,6 +20,10 @@
> > > >  #include "qemu/option.h"
> > > >  #include "qemu/cutils.h"
> > > >+#ifdef CONFIG_GLUSTERFS_LEGACY_FTRUNCATE
> > > >+# define glfs_ftruncate(fd, offset, _u1, _u2) glfs_ftruncate(fd, offset)
> > > >+#endif
> > > 
> > > Someday, when we can assume new enough gluster everywhere, we can drop this
> > > hunk...
> > > 
> > 
> > Part of me wishes that libgfapi had just created a new function
> > 'glfs_ftruncate2', so that existing users don't need to handle the api
> > change.  But I guess in the grand scheme, not a huge deal either way.
> 
> Gluster uses versioned symbols, so older binaries will keep working with
> new libraries. It is (hopefully) rare that existing symbols get updated.
> We try to send patches for these kind of changes to the projects we know
> well in advance, reducing the number of surprises.
> 
> > > >+++ b/configure
> > > 
> > > >+	/* new glfs_ftruncate() passes two additional args */
> > > >+	return glfs_ftruncate(NULL, 0 /*, NULL, NULL */);
> > > >+}
> > > >+EOF
> > > >+    if compile_prog "$glusterfs_cflags" "$glusterfs_libs" ; then
> > > >+      glusterfs_legacy_ftruncate="yes"
> > > >+    fi
> > > 
> > > ...but it will be easier to remember to do so if this comment in configure
> > > calls out the upstream gluster version that no longer requires the legacy
> > > workaround, as our hint for when...
> > > 
> > 
> > I can go ahead and add that to the comment in my branch after applying, if
> > Niels can let me know what that version is/will be (if known).
> 
> The new glfs_ftruncate() will be part of glusterfs-5 (planned for
> October). We're changing the numbering scheme, it was expected to come
> in glusterfs-4.2, but that is a version that never will be released.
> 
> Thanks for correcting the last bits of the patch!
> Niels

So that there is no confusion or miscommunication: I'm not going to pull
this patch in through my tree for 3.0 (or 3.1 yet), since the new API isn't
part of a released version of gluster yet.  (And hopefully we won't ever
need it, if the gluster changes can happen without breaking the existing
API)

-Jeff