linux-ext4 - Re: [PATCH] ext4: add rb_tree cache to struct ext4_group

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 3 Apr 2010 22:06:56 -0400
From:	tytso@....edu
To:	jing zhang <zj.barak@...il.com>
Cc:	linux-ext4 <linux-ext4@...r.kernel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Andreas Dilger <adilger@....com>,
	Dave Kleikamp <shaggy@...ux.vnet.ibm.com>,
	"Aneesh Kumar K. V" <aneesh.kumar@...ux.vnet.ibm.com>
Subject: Re: [PATCH] ext4: add rb_tree cache to struct ext4_group_info

On Sun, Apr 04, 2010 at 09:26:26AM +0800, jing zhang wrote:
> > That being said, I'm not convinced ext4_mb_generate_from_freelist() is
> > (a) necessary, or (b) bug-free, either.  The whole point of having
> > extents in bb_free_root tree is that those extents aren't safe to be
> > placed in the buddy bitmap.  And ext4_mb_generate_from_freelist()
> > isn't freeing the nodes from the rbtree.  Fortunately it looks like
> > ext4_mb_generate_from_freelist is only getting called when the buddy
> > bitmap is being set up, so the rbtree should be empty during those
> > times.
> >
> > I need to do some more investigation, but I think the function can be
> > removed entirely.
> 
> Do you mean that ext4_mb_generate_from_freelist() can be removed entirely?

Maybe.  I need to do more investigation to be sure.  The code in
mballoc is subtle, and in some places there is stuff which is vestigal
or misnamed, but it means that I'm going to be very careful before
changing things.

It also means that if you submit patches, you need to be very clear
what you think the surrounding code is doing, why you think it's
wrong, and how your patch make things better.  For example, this:

    The function, ext4_mb_discard_group_preallocations(), works alone as
    hard as possible without correct understanding its caller's good
    thinking.

    And now try to relieve it in simple way.

is almost useless as a comment.  It doesn't help me understand the
code.  "hard as possible"?  Huh?  "without correct understanding"?
How can code, unless it's artificially intelligent, have
understanding?  And if you meant the original author had no
understanding, how do you know that?  "caller's good thinking"?  Same
question; the calling code doesn't think.

This sort of explanation isn't helpful in understanding the patch.

							- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html