From: Andre Luis Pereira dos Santos <[email protected]>
Hi.
Small refactoring of the code in order to make minor enhancements to critical areas.
The notation x + 1 has been replaced by more efficient notation x + +.
Signed-off-by: Andre Luis Pereira dos Santos <[email protected]>
---
Signed-off-by: Andre Luis Pereira dos Santos <[email protected]>
--- linux-2.6.37-rc1/fs/ext4/extents.c 2010-11-01 09:54:12.000000000 -0200
+++ linux-2.6.37-rc1-patched/fs/ext4/extents.c 2010-11-04 19:54:26.000000000 -0200
@@ -555,9 +555,9 @@ ext4_ext_binsearch(struct inode *inode,
while (l <= r) {
m = l + (r - l) / 2;
if (block < le32_to_cpu(m->ee_block))
- r = m - 1;
+ r = m--;
else
- l = m + 1;
+ l = m++;
ext_debug("%p(%u):%p(%u):%p(%u) ", l, le32_to_cpu(l->ee_block),
m, le32_to_cpu(m->ee_block),
r, le32_to_cpu(r->ee_block));
@@ -1557,7 +1557,7 @@ static int ext4_ext_try_to_merge(struct
if (ext4_ext_is_uninitialized(ex))
uninitialized = 1;
ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex)
- + ext4_ext_get_actual_len(ex + 1));
+ + ext4_ext_get_actual_len(ex++));
if (uninitialized)
ext4_ext_mark_uninitialized(ex);
On Thu, 4 Nov 2010, André Luis Pereira dos Santos - BSRSoft wrote:
> From: Andre Luis Pereira dos Santos <[email protected]>
>
> Hi.
> Small refactoring of the code in order to make minor enhancements to critical areas.
> The notation x + 1 has been replaced by more efficient notation x + +.
>
> Signed-off-by: Andre Luis Pereira dos Santos <[email protected]>
> ---
> Signed-off-by: Andre Luis Pereira dos Santos <[email protected]>
> --- linux-2.6.37-rc1/fs/ext4/extents.c 2010-11-01 09:54:12.000000000 -0200
> +++ linux-2.6.37-rc1-patched/fs/ext4/extents.c 2010-11-04 19:54:26.000000000 -0200
> @@ -555,9 +555,9 @@ ext4_ext_binsearch(struct inode *inode,
> while (l <= r) {
> m = l + (r - l) / 2;
> if (block < le32_to_cpu(m->ee_block))
> - r = m - 1;
> + r = m--;
> else
> - l = m + 1;
> + l = m++;
These do not give identical results.
foo = bar + 1; assigns (bar + 1) to foo.
foo = bar--; assigns bar to foo then decrements bar.
foo = --bar; decrements bar then assigns bar to foo.
So your change both change the value that will be assigned to 'r' and 'l'
and also modify 'm' which was not previously modified.
> ext_debug("%p(%u):%p(%u):%p(%u) ", l, le32_to_cpu(l->ee_block),
> m, le32_to_cpu(m->ee_block),
> r, le32_to_cpu(r->ee_block));
> @@ -1557,7 +1557,7 @@ static int ext4_ext_try_to_merge(struct
> if (ext4_ext_is_uninitialized(ex))
> uninitialized = 1;
> ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex)
> - + ext4_ext_get_actual_len(ex + 1));
> + + ext4_ext_get_actual_len(ex++));
After your change gcc complains:
fs/ext4/extents.c:1559:16: warning: operation on ‘ex’ may be undefined
fs/ext4/extents.c:1559:16: warning: operation on ‘ex’ may be undefined
which it is correct in doing since you are now modifying the value of the
pointer which is dereferenced in the assignment. Previously the value of
(ex+1) was simply passed to ext4_ext_get_actual_len(), but now you are
passing the value of (ex) to ext4_ext_get_actual_len() and then
subsequently incrementing 'ex' itself.
> if (uninitialized)
> ext4_ext_mark_uninitialized(ex);
>
>
Was this patch even compile tested?
--
Jesper Juhl <[email protected]> http://www.chaosbits.net/
Plain text mails only, please http://www.expita.com/nomime.html
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
2010/11/4 Andr? Luis Pereira dos Santos - BSRSoft <[email protected]>:
> From: Andre Luis Pereira dos Santos <[email protected]>
>
> Hi.
> Small refactoring of the code in order to make minor enhancements to critical areas.
> The notation x + 1 has been replaced by more efficient notation x + +.
>
> Signed-off-by: Andre Luis Pereira dos Santos <[email protected]>
> ---
> Signed-off-by: Andre Luis Pereira dos Santos <[email protected]>
> --- linux-2.6.37-rc1/fs/ext4/extents.c ?2010-11-01 09:54:12.000000000 -0200
> +++ linux-2.6.37-rc1-patched/fs/ext4/extents.c ?2010-11-04 19:54:26.000000000 -0200
> @@ -555,9 +555,9 @@ ext4_ext_binsearch(struct inode *inode,
> ? ? ? ?while (l <= r) {
> ? ? ? ? ? ? ? ?m = l + (r - l) / 2;
> ? ? ? ? ? ? ? ?if (block < le32_to_cpu(m->ee_block))
> - ? ? ? ? ? ? ? ? ? ? ? r = m - 1;
> + ? ? ? ? ? ? ? ? ? ? ? r = m--;
> ? ? ? ? ? ? ? ?else
> - ? ? ? ? ? ? ? ? ? ? ? l = m + 1;
> + ? ? ? ? ? ? ? ? ? ? ? l = m++;
> ? ? ? ? ? ? ? ?ext_debug("%p(%u):%p(%u):%p(%u) ", l, le32_to_cpu(l->ee_block),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?m, le32_to_cpu(m->ee_block),
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?r, le32_to_cpu(r->ee_block));
> @@ -1557,7 +1557,7 @@ static int ext4_ext_try_to_merge(struct
> ? ? ? ? ? ? ? ?if (ext4_ext_is_uninitialized(ex))
> ? ? ? ? ? ? ? ? ? ? ? ?uninitialized = 1;
> ? ? ? ? ? ? ? ?ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex)
> - ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + ext4_ext_get_actual_len(ex + 1));
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? + ext4_ext_get_actual_len(ex++));
> ? ? ? ? ? ? ? ?if (uninitialized)
> ? ? ? ? ? ? ? ? ? ? ? ?ext4_ext_mark_uninitialized(ex);
But
r = m - 1;
is not equivalent to
r = m--;
At a minimum your code no longer passes the same value to of m to ext_debug.
NAK
Greg
Hello.
This is true.
Despite having gone through all compilation and I have not noticed apparent problems, my mistake is evident in this case.
I had not realized that the side effect would really be changing variables incremented and decremented.
Thank you.
I'll think more about this type of side effect. :)
On Thu, 4 Nov 2010, André Luis Pereira dos Santos - BSRSoft wrote:
> From: Andre Luis Pereira dos Santos <[email protected]>
>
> Hi.
> Small refactoring of the code in order to make minor enhancements to critical areas.
> The notation x + 1 has been replaced by more efficient notation x + +.
>
> Signed-off-by: Andre Luis Pereira dos Santos <[email protected]>
> ---
> Signed-off-by: Andre Luis Pereira dos Santos <[email protected]>
> --- linux-2.6.37-rc1/fs/ext4/extents.c 2010-11-01 09:54:12.000000000 -0200
> +++ linux-2.6.37-rc1-patched/fs/ext4/extents.c 2010-11-04 19:54:26.000000000 -0200
> @@ -555,9 +555,9 @@ ext4_ext_binsearch(struct inode *inode,
> while (l <= r) {
> m = l + (r - l) / 2;
> if (block < le32_to_cpu(m->ee_block))
> - r = m - 1;
> + r = m--;
> else
> - l = m + 1;
> + l = m++;
These do not give identical results.
foo = bar + 1; assigns (bar + 1) to foo.
foo = bar--; assigns bar to foo then decrements bar.
foo = --bar; decrements bar then assigns bar to foo.
So your change both change the value that will be assigned to 'r' and 'l'
and also modify 'm' which was not previously modified.
> ext_debug("%p(%u):%p(%u):%p(%u) ", l, le32_to_cpu(l->ee_block),
> m, le32_to_cpu(m->ee_block),
> r, le32_to_cpu(r->ee_block));
> @@ -1557,7 +1557,7 @@ static int ext4_ext_try_to_merge(struct
> if (ext4_ext_is_uninitialized(ex))
> uninitialized = 1;
> ex->ee_len = cpu_to_le16(ext4_ext_get_actual_len(ex)
> - + ext4_ext_get_actual_len(ex + 1));
> + + ext4_ext_get_actual_len(ex++));
After your change gcc complains:
fs/ext4/extents.c:1559:16: warning: operation on ‘ex’ may be undefined
fs/ext4/extents.c:1559:16: warning: operation on ‘ex’ may be undefined
which it is correct in doing since you are now modifying the value of the
pointer which is dereferenced in the assignment. Previously the value of
(ex+1) was simply passed to ext4_ext_get_actual_len(), but now you are
passing the value of (ex) to ext4_ext_get_actual_len() and then
subsequently incrementing 'ex' itself.
> if (uninitialized)
> ext4_ext_mark_uninitialized(ex);
>
>
Was this patch even compile tested?
On Thu, Nov 04, 2010 at 04:34:26PM -0700, Andr? Luis Pereira dos Santos - BSRSoft wrote:
>
> I had not realized that the side effect would really be changing
> variables incremented and decremented.
You're also wrong if you think that "i--" is more efficient than "i =
i - 1". Any good compiler will do the right thing with either
expression.
- Ted