gluster-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Gluster-devel] [PATCH: glusterfs] fixed? dht and nufa


From: Amar Tumballi
Subject: Re: [Gluster-devel] [PATCH: glusterfs] fixed? dht and nufa
Date: Thu, 23 Apr 2009 01:20:05 -0700

Reviews inline:

This patch has dependency on previous/another patch on alu's disk-usage parsing.


---
 libglusterfs/src/xlator.c               |    2 +-
 scheduler/alu/src/alu.c                 |    8 +++--
 xlators/cluster/dht/src/dht-common.h    |    3 +-
 xlators/cluster/dht/src/dht-diskusage.c |   48 +++++++++++++++++++++++--------
 xlators/cluster/dht/src/dht.c           |   24 +++++++++------
 xlators/cluster/dht/src/nufa.c          |   24 ++++++++++-----
 6 files changed, 75 insertions(+), 34 deletions(-)

diff --git a/libglusterfs/src/xlator.c b/libglusterfs/src/xlator.c
index 5b5067a..0345037 100644
--- a/libglusterfs/src/xlator.c
+++ b/libglusterfs/src/xlator.c
@@ -434,7 +434,7 @@ _volume_option_value_validate (xlator_t *xl,
       case GF_OPTION_TYPE_PERCENTORSIZET:
       {
               uint32_t percent = 0;
-               uint32_t input_size = 0;
+               input_size = 0;

               /* Check if the value is valid percentage */
               if (gf_string2percent (pair->value->data,
diff --git a/scheduler/alu/src/alu.c b/scheduler/alu/src/alu.c
index 8939531..967eece 100644
--- a/scheduler/alu/src/alu.c
+++ b/scheduler/alu/src/alu.c
@@ -98,6 +98,7 @@ get_stats_free_disk (struct xlator_stats *this)
                       return this->free_disk;
               }
       }
+       return 0;
 }

 static int64_t
@@ -384,10 +385,11 @@ alu_init (xlator_t *xl)
               _limit_fn->next = tmp_limits;
               alu_sched->limits_fn = _limit_fn;

-               if (gf_string2percent (limits->data, &min_free_disk) == 0) {
-                       alu_sched->spec_limit.disk_unit = 'p';
-               } else {
+               if (gf_string2bytesize (limits->data, &min_free_disk)) {
                       alu_sched->spec_limit.disk_unit = 'b';
+               } else {
+                       gf_string2percent (limits->data, &min_free_disk);
+                       alu_sched->spec_limit.disk_unit = 'p';
               }

In all the places its logical to check for percent first and if fails then to look for bytesize.

Imagine a case where 'option min-free-disk 90' is given. (note that there is no % or 'MB/GB/TB' after 90).

User's intention would be set it to 'percent' based check, but both parser and translators assume it as bytesize and consider 90bytes as the limit to treat a disk as full :O

Other than this change (everywhere the logic is used), patch is good.

+                       gf_string2percent (limits->data, &min_free_disk);

'gf_string2percent() takes 'int32_t *' as second argument, hence it gives warning which will make our build scripts to fail. Instead you can use a temporary variable there, and initialize the min_free_disk variable.

Paul, can you resend the patch ? or do you want us to make this change and commit?

Thanks and Regards,

--
Amar Tumballi


reply via email to

[Prev in Thread] Current Thread [Next in Thread]