Alex Tomas wrote:
Amit K Arora (AKA) writes:AKA> On Tue, Jan 02, 2007 at 12:25:21PM +0300, Alex Tomas (AT) wrote: >> >>>>> Amit K Arora (AKA) writes: >> AKA> The ext4_ext_get_blocks() and ext4_ext_insert_extent() routines do not AKA> check for extent overlap, when a new extent needs to be inserted in an AKA> inode. An overlap is possible when the new extent being inserted has AKA> ee_block that is not part of any of the existing extents, but the AKA> tail/center portion of this new extent _is_. This is possible only when AKA> we are writing/preallocating blocks across a hole. >> AT> not sure I understand ... you shouldn't insert an extent that overlap AT> any existing extent. when you write block(s), you first check is AT> it already allocated and insert new extent only if it's not. AKA> You are right. That is what this patch does. AKA> The current ext4 code is inserting an overlapped extent in a particular AKA> scenario (explained above). The suggested patch fixes this by having a AKA> check in get_blocks() for _not_ inserting an extent that may overlap AKA> with an existing one. I think that stuff that converts uninitialized blocks to initialized ones should be a separate codepath and shouldn't be done in the insert path. and an insert (basic tree manipulation) should BUG_ON() one tries to add extent with a block which is already covered by the tree. IMHO, get_blocks() should look like: path = find_path() if (found extent covers request block(s)) { if (extent is uninitialized) { convert(); } } where function convert() { /* adopt existing extent so that it * doesn't cover requested blocks */ /* insert head or tail of existing * extent, if necessary */ /* insert new extent of initialized blocks */ } thanks, Alex
I was thing about the same thing. The current ext4_ext_get_blocks() function becomes very bulky. The code to convert uninitialized blocks to initialized ones is pretty selfcontained, and worth the effort to put it into a seperate function.
But the bug Amit pointed here is unrelated to the code convert uninitialized blocks to initialized ones. Rather, it's related to do multiple block allocation across on a window with parts already have blocks allocated. Without the check, the current code just simply allocate the requested extent and insert it into the tree which might overlap with existing extent.
Mingming - To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
