linux-ext4 - Re: [PATCH, RFC] ext4: Replace hackish ext4_mb_poll_new

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20081017122552.GC21503@mit.edu>
Date:	Fri, 17 Oct 2008 08:25:52 -0400
From:	Theodore Tso <tytso@....edu>
To:	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	linux-ext4@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH, RFC] ext4: Replace hackish
	ext4_mb_poll_new_transaction with commit callback

On Fri, Oct 17, 2008 at 06:02:14AM -0400, Theodore Tso wrote:
> > 
> > Why not use journal commit callback patch from andreas 
> > (MID:20080929201752.GN10950@...ber.adilger.int
> > http://article.gmane.org/gmane.comp.file-systems.ext4/9148)
> > 
> > The patch you sent will allows only one call back to be registered.
> 
> Thanks for reminding me about that; I had completely forgotten about
> Andreas' patch.  Sure, I'll respin the patch to use his extension.

I looked more closely at Andreas' patch, and it's really not a good
fit for what we want to do.  The problem is that it is designed to
attach arbitrary callbacks on a per transaction basis.  Each time you
add a callback you need to allocate a stucture, and then it gets
chained onto a inked list.

For what we need for mballoc, not only is it massive overkill, but
every single time we try to free blocks, we would have to and then
search the list to see the remove_committed_blocks() callback was or
wasn't on the list yet, and if it wasn't then allocate a chunk of
memory to hold the struct journal_callback data structure and call
journal_callback_set() function.   

Furthermore, to get the locking right and to avoid race conditions,
we'd have to add a _journal_callback_set() function which doesn't take
the spinlock, and then take the spinlock ourselves, search the linked
list, allocate the memory, call _journal_callback_set(), and then
release the spinlock.

It was at about this point that I decided it Just Wasn't Worth It.

What I added was a dead-simple per-journal commit callback, with no
additional memory allocations (and requirement to do error handling if
the memory allocation fails), no need to take a spinlock before
manually adding the call back to each transaction handle, no need to
search the linked list to see if we have an entry on the linked list
already, etc.

If in the future we need a true per-transaction handle commit
callback, we can add this; but I think it still makes more sense to
keep the per-journal commit callback.  After all, as it stands the
current patch results in a net reduction of 46 lines of code.  Adding
all of this machinery would erase take far more than the savings by
removing ext4_mb_poll_new_transaction().

							- 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