[Ocfs2-devel] [PATCH 1/4] ocfs2/trivial: Move 'wanted' into parens of ocfs2_resmap_resv_bits.
Mark Fasheh
mfasheh at suse.com
Fri Aug 13 16:16:23 PDT 2010
Hi Tao,
On Mon, Aug 02, 2010 at 11:02:12AM +0800, Tao Ma wrote:
> The first time I read the function ocfs2_resmap_resv_bits, I consider
> about what 'wanted' will be used and consider about the comments.
> Then I find it is only used if the reservation is empty. ;)
>
> So we'd better move it to the parens so that it make the code more
> readable, what's more, ocfs2_resmap_resv_bits is used so frequently
> and we should save some cpus. The corresponding BUG_ON is also moved
> into parens since it is only meaningful after we reinit the resv.
>
> Cc: Mark Fasheh <mfasheh at suse.com>
> Signed-off-by: Tao Ma <tao.ma at oracle.com>
> ---
> fs/ocfs2/reservations.c | 26 ++++++++++++--------------
> 1 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/fs/ocfs2/reservations.c b/fs/ocfs2/reservations.c
> index d8b6e42..567c1a0 100644
> --- a/fs/ocfs2/reservations.c
> +++ b/fs/ocfs2/reservations.c
> @@ -732,25 +732,23 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
> struct ocfs2_alloc_reservation *resv,
> int *cstart, int *clen)
> {
> - unsigned int wanted = *clen;
> -
> if (resv == NULL || ocfs2_resmap_disabled(resmap))
> return -ENOSPC;
>
> spin_lock(&resv_lock);
>
> - /*
> - * We don't want to over-allocate for temporary
> - * windows. Otherwise, we run the risk of fragmenting the
> - * allocation space.
> - */
> - wanted = ocfs2_resv_window_bits(resmap, resv);
> - if ((resv->r_flags & OCFS2_RESV_FLAG_TMP) || wanted < *clen)
> - wanted = *clen;
> -
> if (ocfs2_resv_empty(resv)) {
> - mlog(0, "empty reservation, find new window\n");
> + /*
> + * We don't want to over-allocate for temporary
> + * windows. Otherwise, we run the risk of fragmenting the
> + * allocation space.
> + */
> + unsigned int wanted = ocfs2_resv_window_bits(resmap, resv);
> +
> + if ((resv->r_flags & OCFS2_RESV_FLAG_TMP) || wanted < *clen)
> + wanted = *clen;
>
> + mlog(0, "empty reservation, find new window\n");
> /*
> * Try to get a window here. If it works, we must fall
> * through and test the bitmap . This avoids some
> @@ -759,9 +757,9 @@ int ocfs2_resmap_resv_bits(struct ocfs2_reservation_map *resmap,
> * that inode.
> */
> ocfs2_resv_find_window(resmap, resv, wanted);
> - }
>
> - BUG_ON(ocfs2_resv_empty(resv));
> + BUG_ON(ocfs2_resv_empty(resv));
> + }
Can we leave the BUG_ON() outside the if (ocfs2_resv_empty(...)) { } block?
You're technically correct in that the only possibility right now is if
there's a bug in ocfs2_resv_find_window(). However, I think it still belongs
outside the block because we're returning an allocation at that point. The
code is self documenting then - "don't let it get here unless resv->r_start
and resv->r_len mean something". That way any future changes to the function
will be forced to take this BUG_ON() into consideration, and we are then
less likely to accidentally corrupt data.
Otherwise the patch looks great to me. Thanks for the cleanup in the first
hunk!
--Mark
--
Mark Fasheh
More information about the Ocfs2-devel
mailing list