From: Eric Sandeen <sandeen@redhat.com>
To: "Theodore Ts'o" <tytso@mit.edu>
Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>
Subject: Re: [PATCH 1/3] libext2fs: remove pointless indirection in rbtree bitmaps
Date: Sat, 06 Oct 2012 10:52:38 -0500 [thread overview]
Message-ID: <507053C6.2010809@redhat.com> (raw)
In-Reply-To: <1349489052-18657-1-git-send-email-tytso@mit.edu>
On 10/5/12 9:04 PM, Theodore Ts'o wrote:
> The code was previously allocating a single 4 or 8 byte pointer for
> the rcursor and wcursor fields in the ext2fs_rb_private structure;
> this added two extra memory allocations (which could fail), and extra
> indirections, for no good reason. Removing the extra indirection also
> makes the code more readable, so it's all upside and no downside.
>
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Makes sense to me!
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
> lib/ext2fs/blkmap64_rb.c | 46 +++++++++++++++++++---------------------------
> 1 file changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/lib/ext2fs/blkmap64_rb.c b/lib/ext2fs/blkmap64_rb.c
> index c9006f8..900c0d3 100644
> --- a/lib/ext2fs/blkmap64_rb.c
> +++ b/lib/ext2fs/blkmap64_rb.c
> @@ -38,8 +38,8 @@ struct bmap_rb_extent {
>
> struct ext2fs_rb_private {
> struct rb_root root;
> - struct bmap_rb_extent **wcursor;
> - struct bmap_rb_extent **rcursor;
> + struct bmap_rb_extent *wcursor;
> + struct bmap_rb_extent *rcursor;
> #ifdef BMAP_STATS_OPS
> __u64 mark_hit;
> __u64 test_hit;
> @@ -148,10 +148,10 @@ inline
> static void rb_free_extent(struct ext2fs_rb_private *bp,
> struct bmap_rb_extent *ext)
> {
> - if (*bp->wcursor == ext)
> - *bp->wcursor = NULL;
> - if (*bp->rcursor == ext)
> - *bp->rcursor = NULL;
> + if (bp->wcursor == ext)
> + bp->wcursor = NULL;
> + if (bp->rcursor == ext)
> + bp->rcursor = NULL;
> ext2fs_free_mem(&ext);
> }
>
> @@ -165,14 +165,8 @@ static errcode_t rb_alloc_private_data (ext2fs_generic_bitmap bitmap)
> return retval;
>
> bp->root = RB_ROOT;
> - retval = ext2fs_get_mem(sizeof(struct bmap_rb_extent *), &bp->rcursor);
> - if (retval)
> - return retval;
> - retval = ext2fs_get_mem(sizeof(struct bmap_rb_extent *), &bp->wcursor);
> - if (retval)
> - return retval;
> - *bp->rcursor = NULL;
> - *bp->wcursor = NULL;
> + bp->rcursor = NULL;
> + bp->wcursor = NULL;
>
> #ifdef BMAP_STATS_OPS
> bp->test_hit = 0;
> @@ -215,8 +209,6 @@ static void rb_free_bmap(ext2fs_generic_bitmap bitmap)
> bp = (struct ext2fs_rb_private *) bitmap->private;
>
> rb_free_tree(&bp->root);
> - ext2fs_free_mem(&bp->rcursor);
> - ext2fs_free_mem(&bp->wcursor);
> ext2fs_free_mem(&bp);
> bp = 0;
> }
> @@ -235,8 +227,8 @@ static errcode_t rb_copy_bmap(ext2fs_generic_bitmap src,
>
> src_bp = (struct ext2fs_rb_private *) src->private;
> dest_bp = (struct ext2fs_rb_private *) dest->private;
> - *src_bp->rcursor = NULL;
> - *dest_bp->rcursor = NULL;
> + src_bp->rcursor = NULL;
> + dest_bp->rcursor = NULL;
>
> src_node = ext2fs_rb_first(&src_bp->root);
> while (src_node) {
> @@ -299,8 +291,8 @@ static errcode_t rb_resize_bmap(ext2fs_generic_bitmap bmap,
> }
>
> bp = (struct ext2fs_rb_private *) bmap->private;
> - *bp->rcursor = NULL;
> - *bp->wcursor = NULL;
> + bp->rcursor = NULL;
> + bp->wcursor = NULL;
>
> /* truncate tree to new_real_end size */
> rb_truncate(new_real_end, &bp->root);
> @@ -319,7 +311,7 @@ rb_test_bit(struct ext2fs_rb_private *bp, __u64 bit)
> struct rb_node **n = &bp->root.rb_node;
> struct bmap_rb_extent *ext;
>
> - rcursor = *bp->rcursor;
> + rcursor = bp->rcursor;
> if (!rcursor)
> goto search_tree;
>
> @@ -342,7 +334,7 @@ rb_test_bit(struct ext2fs_rb_private *bp, __u64 bit)
> }
> }
>
> - rcursor = *bp->wcursor;
> + rcursor = bp->wcursor;
> if (!rcursor)
> goto search_tree;
>
> @@ -359,7 +351,7 @@ search_tree:
> else if (bit >= (ext->start + ext->count))
> n = &(*n)->rb_right;
> else {
> - *bp->rcursor = ext;
> + bp->rcursor = ext;
> return 1;
> }
> }
> @@ -376,7 +368,7 @@ static int rb_insert_extent(__u64 start, __u64 count,
> struct bmap_rb_extent *ext;
> int retval = 0;
>
> - ext = *bp->wcursor;
> + ext = bp->wcursor;
> if (ext) {
> if (start >= ext->start &&
> start <= (ext->start + ext->count)) {
> @@ -419,7 +411,7 @@ got_extent:
> new_node = &new_ext->node;
> ext2fs_rb_link_node(new_node, parent, n);
> ext2fs_rb_insert_color(new_node, root);
> - *bp->wcursor = new_ext;
> + bp->wcursor = new_ext;
>
> node = ext2fs_rb_prev(new_node);
> if (node) {
> @@ -745,8 +737,8 @@ static void rb_clear_bmap(ext2fs_generic_bitmap bitmap)
> bp = (struct ext2fs_rb_private *) bitmap->private;
>
> rb_free_tree(&bp->root);
> - *bp->rcursor = NULL;
> - *bp->wcursor = NULL;
> + bp->rcursor = NULL;
> + bp->wcursor = NULL;
> }
>
> #ifdef BMAP_STATS
>
next prev parent reply other threads:[~2012-10-06 15:52 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-05 3:44 [PATCH 1/2] e2freefrag: use 64-bit rbtree bitmaps Theodore Ts'o
2012-10-05 3:44 ` [PATCH 2/2] libext2fs: optimize rb_test_bit Theodore Ts'o
2012-10-06 2:04 ` [PATCH 1/3] libext2fs: remove pointless indirection in rbtree bitmaps Theodore Ts'o
2012-10-06 2:04 ` [PATCH 2/3] libext2fs: further optimize rb_test_bit Theodore Ts'o
2012-10-06 2:04 ` [PATCH 3/3] Fix makefiles to compile e2freefrag with profiling Theodore Ts'o
2012-10-06 15:54 ` Eric Sandeen
2012-10-06 15:52 ` Eric Sandeen [this message]
2012-10-08 8:08 ` [PATCH 2/2] libext2fs: optimize rb_test_bit Lukáš Czerner
2012-10-08 8:25 ` Lukáš Czerner
2012-10-08 18:17 ` Theodore Ts'o
2012-10-09 7:18 ` Lukáš Czerner
2012-10-09 19:55 ` Theodore Ts'o
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=507053C6.2010809@redhat.com \
--to=sandeen@redhat.com \
--cc=linux-ext4@vger.kernel.org \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).