diff mbox series

[RFC] block/gluster: limit the transfer size to 512 MiB

Message ID 20190328105227.74725-1-sgarzare@redhat.com (mailing list archive)
State New, archived
Headers show
Series [RFC] block/gluster: limit the transfer size to 512 MiB | expand

Commit Message

Stefano Garzarella March 28, 2019, 10:52 a.m. UTC
Several versions of GlusterFS (3.12? -> 6.0.1) fail when the
transfer size is greater or equal to 1024 MiB, so we are
limiting the transfer size to 512 MiB to avoid this rare issue.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1691320
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---

RFC:
Should I add a parameter to allow the user to modify the max_transfer
variable?


 block/gluster.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Niels de Vos March 28, 2019, 11:47 a.m. UTC | #1
On Thu, Mar 28, 2019 at 11:52:27AM +0100, Stefano Garzarella wrote:
> Several versions of GlusterFS (3.12? -> 6.0.1) fail when the
> transfer size is greater or equal to 1024 MiB, so we are
> limiting the transfer size to 512 MiB to avoid this rare issue.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1691320
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> RFC:
> Should I add a parameter to allow the user to modify the max_transfer
> variable?

No, that is not needed IMHO. In most environments where large files are
used, sharding is enabled on the Gluster volumes. For VM images shards
of 256MB are recommeneded (I think). When sharding is enabled, the
writes will be limited to that size as well, hence the problem is not
noticed on (most) production deployments.

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

> 
> 
>  block/gluster.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index af64330211..e1e4eaa525 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include <glusterfs/api/glfs.h>
>  #include "block/block_int.h"
>  #include "block/qdict.h"
> @@ -37,6 +38,12 @@
>  #define GLUSTER_DEBUG_MAX           9
>  #define GLUSTER_OPT_LOGFILE         "logfile"
>  #define GLUSTER_LOGFILE_DEFAULT     "-" /* handled in libgfapi as /dev/stderr */
> +/*
> + * Several versions of GlusterFS (3.12? -> 6.0.1) fail when the transfer size
> + * is greater or equal to 1024 MiB, so we are limiting the transfer size to 512
> + * MiB to avoid this rare issue.
> + */
> +#define GLUSTER_MAX_TRANSFER        (512 * MiB)
>  
>  #define GERR_INDEX_HINT "hint: check in 'server' array index '%d'\n"
>  
> @@ -879,6 +886,11 @@ out:
>      return ret;
>  }
>  
> +static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
> +{
> +    bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
> +}
> +
>  static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
>                                         BlockReopenQueue *queue, Error **errp)
>  {
> @@ -1536,6 +1548,7 @@ static BlockDriver bdrv_gluster = {
>      .bdrv_co_pwrite_zeroes        = qemu_gluster_co_pwrite_zeroes,
>  #endif
>      .bdrv_co_block_status         = qemu_gluster_co_block_status,
> +    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
>      .create_opts                  = &qemu_gluster_create_opts,
>      .strong_runtime_opts          = gluster_strong_open_opts,
>  };
> @@ -1566,6 +1579,7 @@ static BlockDriver bdrv_gluster_tcp = {
>      .bdrv_co_pwrite_zeroes        = qemu_gluster_co_pwrite_zeroes,
>  #endif
>      .bdrv_co_block_status         = qemu_gluster_co_block_status,
> +    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
>      .create_opts                  = &qemu_gluster_create_opts,
>      .strong_runtime_opts          = gluster_strong_open_opts,
>  };
> @@ -1596,6 +1610,7 @@ static BlockDriver bdrv_gluster_unix = {
>      .bdrv_co_pwrite_zeroes        = qemu_gluster_co_pwrite_zeroes,
>  #endif
>      .bdrv_co_block_status         = qemu_gluster_co_block_status,
> +    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
>      .create_opts                  = &qemu_gluster_create_opts,
>      .strong_runtime_opts          = gluster_strong_open_opts,
>  };
> @@ -1632,6 +1647,7 @@ static BlockDriver bdrv_gluster_rdma = {
>      .bdrv_co_pwrite_zeroes        = qemu_gluster_co_pwrite_zeroes,
>  #endif
>      .bdrv_co_block_status         = qemu_gluster_co_block_status,
> +    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
>      .create_opts                  = &qemu_gluster_create_opts,
>      .strong_runtime_opts          = gluster_strong_open_opts,
>  };
> -- 
> 2.20.1
> 
>
Kevin Wolf March 28, 2019, 11:59 a.m. UTC | #2
Am 28.03.2019 um 11:52 hat Stefano Garzarella geschrieben:
> Several versions of GlusterFS (3.12? -> 6.0.1) fail when the
> transfer size is greater or equal to 1024 MiB, so we are
> limiting the transfer size to 512 MiB to avoid this rare issue.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1691320
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> RFC:
> Should I add a parameter to allow the user to modify the max_transfer
> variable?

Ideally, gluster would have provided a way to detect automatically
whether the bug is present. But since you suggest a user option, I'm
afraid they haven't?

Kevin
Stefano Garzarella March 28, 2019, 1:35 p.m. UTC | #3
On Thu, Mar 28, 2019 at 12:59:55PM +0100, Kevin Wolf wrote:
> Am 28.03.2019 um 11:52 hat Stefano Garzarella geschrieben:
> > Several versions of GlusterFS (3.12? -> 6.0.1) fail when the
> > transfer size is greater or equal to 1024 MiB, so we are
> > limiting the transfer size to 512 MiB to avoid this rare issue.
> > 
> > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1691320
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > 
> > RFC:
> > Should I add a parameter to allow the user to modify the max_transfer
> > variable?
> 
> Ideally, gluster would have provided a way to detect automatically
> whether the bug is present. But since you suggest a user option, I'm
> afraid they haven't?

Unfortunately, all version that I tried have this bug, also the latest
available, for this reason, I hard coded the limit in the gluster driver.
I also think that this limit is not reached very often for standard
operations.

Maybe I should check the version of the library during the configuration,
when they will fix the issue.

What do you suggest?

Thanks,
Stefano
Kevin Wolf March 28, 2019, 2:42 p.m. UTC | #4
Am 28.03.2019 um 14:35 hat Stefano Garzarella geschrieben:
> On Thu, Mar 28, 2019 at 12:59:55PM +0100, Kevin Wolf wrote:
> > Am 28.03.2019 um 11:52 hat Stefano Garzarella geschrieben:
> > > Several versions of GlusterFS (3.12? -> 6.0.1) fail when the
> > > transfer size is greater or equal to 1024 MiB, so we are
> > > limiting the transfer size to 512 MiB to avoid this rare issue.
> > > 
> > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1691320
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > > 
> > > RFC:
> > > Should I add a parameter to allow the user to modify the max_transfer
> > > variable?
> > 
> > Ideally, gluster would have provided a way to detect automatically
> > whether the bug is present. But since you suggest a user option, I'm
> > afraid they haven't?
> 
> Unfortunately, all version that I tried have this bug, also the latest
> available, for this reason, I hard coded the limit in the gluster driver.
> I also think that this limit is not reached very often for standard
> operations.
> 
> Maybe I should check the version of the library during the configuration,
> when they will fix the issue.
> 
> What do you suggest?

Oh, sorry, I should have actually looked at the BZ. I assumed it was
already fixed, but it's not. Then I think what you have is fine for now.

Maybe you could suggest to the gluster people that they add some kind of
a feature flag or something somewhere together with the fix so we can
find out at runtime whether the installed gluster libs are fixed or not.

Kevin
Stefano Garzarella March 28, 2019, 3:27 p.m. UTC | #5
On Thu, Mar 28, 2019 at 03:42:49PM +0100, Kevin Wolf wrote:
> Am 28.03.2019 um 14:35 hat Stefano Garzarella geschrieben:
> > On Thu, Mar 28, 2019 at 12:59:55PM +0100, Kevin Wolf wrote:
> > > Am 28.03.2019 um 11:52 hat Stefano Garzarella geschrieben:
> > > > Several versions of GlusterFS (3.12? -> 6.0.1) fail when the
> > > > transfer size is greater or equal to 1024 MiB, so we are
> > > > limiting the transfer size to 512 MiB to avoid this rare issue.
> > > > 
> > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1691320
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > ---
> > > > 
> > > > RFC:
> > > > Should I add a parameter to allow the user to modify the max_transfer
> > > > variable?
> > > 
> > > Ideally, gluster would have provided a way to detect automatically
> > > whether the bug is present. But since you suggest a user option, I'm
> > > afraid they haven't?
> > 
> > Unfortunately, all version that I tried have this bug, also the latest
> > available, for this reason, I hard coded the limit in the gluster driver.
> > I also think that this limit is not reached very often for standard
> > operations.
> > 
> > Maybe I should check the version of the library during the configuration,
> > when they will fix the issue.
> > 
> > What do you suggest?
> 
> Oh, sorry, I should have actually looked at the BZ. I assumed it was
> already fixed, but it's not. Then I think what you have is fine for now.

Good :)
Should I resend without RFC or it can go as is through your block tree?

> 
> Maybe you could suggest to the gluster people that they add some kind of
> a feature flag or something somewhere together with the fix so we can
> find out at runtime whether the installed gluster libs are fixed or not.

Thanks! I'll forward this useful suggestion!

Stefano
Kevin Wolf March 28, 2019, 3:42 p.m. UTC | #6
Am 28.03.2019 um 16:27 hat Stefano Garzarella geschrieben:
> On Thu, Mar 28, 2019 at 03:42:49PM +0100, Kevin Wolf wrote:
> > Am 28.03.2019 um 14:35 hat Stefano Garzarella geschrieben:
> > > On Thu, Mar 28, 2019 at 12:59:55PM +0100, Kevin Wolf wrote:
> > > > Am 28.03.2019 um 11:52 hat Stefano Garzarella geschrieben:
> > > > > Several versions of GlusterFS (3.12? -> 6.0.1) fail when the
> > > > > transfer size is greater or equal to 1024 MiB, so we are
> > > > > limiting the transfer size to 512 MiB to avoid this rare issue.
> > > > > 
> > > > > Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1691320
> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > ---
> > > > > 
> > > > > RFC:
> > > > > Should I add a parameter to allow the user to modify the max_transfer
> > > > > variable?
> > > > 
> > > > Ideally, gluster would have provided a way to detect automatically
> > > > whether the bug is present. But since you suggest a user option, I'm
> > > > afraid they haven't?
> > > 
> > > Unfortunately, all version that I tried have this bug, also the latest
> > > available, for this reason, I hard coded the limit in the gluster driver.
> > > I also think that this limit is not reached very often for standard
> > > operations.
> > > 
> > > Maybe I should check the version of the library during the configuration,
> > > when they will fix the issue.
> > > 
> > > What do you suggest?
> > 
> > Oh, sorry, I should have actually looked at the BZ. I assumed it was
> > already fixed, but it's not. Then I think what you have is fine for now.
> 
> Good :)
> Should I resend without RFC or it can go as is through your block tree?

No need to resend, I'll apply it right away.

> > Maybe you could suggest to the gluster people that they add some kind of
> > a feature flag or something somewhere together with the fix so we can
> > find out at runtime whether the installed gluster libs are fixed or not.
> 
> Thanks! I'll forward this useful suggestion!

Thanks!

Kevin
diff mbox series

Patch

diff --git a/block/gluster.c b/block/gluster.c
index af64330211..e1e4eaa525 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -9,6 +9,7 @@ 
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include <glusterfs/api/glfs.h>
 #include "block/block_int.h"
 #include "block/qdict.h"
@@ -37,6 +38,12 @@ 
 #define GLUSTER_DEBUG_MAX           9
 #define GLUSTER_OPT_LOGFILE         "logfile"
 #define GLUSTER_LOGFILE_DEFAULT     "-" /* handled in libgfapi as /dev/stderr */
+/*
+ * Several versions of GlusterFS (3.12? -> 6.0.1) fail when the transfer size
+ * is greater or equal to 1024 MiB, so we are limiting the transfer size to 512
+ * MiB to avoid this rare issue.
+ */
+#define GLUSTER_MAX_TRANSFER        (512 * MiB)
 
 #define GERR_INDEX_HINT "hint: check in 'server' array index '%d'\n"
 
@@ -879,6 +886,11 @@  out:
     return ret;
 }
 
+static void qemu_gluster_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    bs->bl.max_transfer = GLUSTER_MAX_TRANSFER;
+}
+
 static int qemu_gluster_reopen_prepare(BDRVReopenState *state,
                                        BlockReopenQueue *queue, Error **errp)
 {
@@ -1536,6 +1548,7 @@  static BlockDriver bdrv_gluster = {
     .bdrv_co_pwrite_zeroes        = qemu_gluster_co_pwrite_zeroes,
 #endif
     .bdrv_co_block_status         = qemu_gluster_co_block_status,
+    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
     .create_opts                  = &qemu_gluster_create_opts,
     .strong_runtime_opts          = gluster_strong_open_opts,
 };
@@ -1566,6 +1579,7 @@  static BlockDriver bdrv_gluster_tcp = {
     .bdrv_co_pwrite_zeroes        = qemu_gluster_co_pwrite_zeroes,
 #endif
     .bdrv_co_block_status         = qemu_gluster_co_block_status,
+    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
     .create_opts                  = &qemu_gluster_create_opts,
     .strong_runtime_opts          = gluster_strong_open_opts,
 };
@@ -1596,6 +1610,7 @@  static BlockDriver bdrv_gluster_unix = {
     .bdrv_co_pwrite_zeroes        = qemu_gluster_co_pwrite_zeroes,
 #endif
     .bdrv_co_block_status         = qemu_gluster_co_block_status,
+    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
     .create_opts                  = &qemu_gluster_create_opts,
     .strong_runtime_opts          = gluster_strong_open_opts,
 };
@@ -1632,6 +1647,7 @@  static BlockDriver bdrv_gluster_rdma = {
     .bdrv_co_pwrite_zeroes        = qemu_gluster_co_pwrite_zeroes,
 #endif
     .bdrv_co_block_status         = qemu_gluster_co_block_status,
+    .bdrv_refresh_limits          = qemu_gluster_refresh_limits,
     .create_opts                  = &qemu_gluster_create_opts,
     .strong_runtime_opts          = gluster_strong_open_opts,
 };