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

[Amar Tumballi]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://supercolony.gluster.org/pipermail/gluster-devel/attachments/20090423/ac817969/attachment-0003.html>