On Wed, May 13, 2009 at 01:56:56PM -0500, Eric Sandeen wrote: > > > > I think it is good. But one thing missing in the commit message is, > > what happens if we do a write to prealloc space. Since a > > get_block(create = 1) is now split into __get_block(create = 0 ) and > > __get_block(create = 1). That would mean if we pass a buffer head with > > BH_Unwritten cleared we will have > > > > > > 1) buffer_head as BH_Unwritten cleared. > > > > 2) __get_block(create = 0 ) -> Since it is prealloc space we will have > > BH_Unwritten set . > > Why do we need to set BH_Unwritten on a !create call at all? > > Or maybe another way of asking is, are there any !create callers of > get_block who -want- BH_Unwritten set? > > Which is to say, should we just not be setting BH_Unwritten in get_block > in the !create case, ever? It should only be set in the !create case. With create == 1, we would have already converted the uninitialized extent to initialized one and the buffer_head should not be unwritten at all. My understanding is unwritten flag is used to indicate the buffer_head state between a write_begin and write_page phase with delayed allocation. ie, when we write to fallocate space, since we have delalloc enabled, we just do a block lookup (get_block with create = 0). The buffer_head returned in the above case should have unwritten set so that during writepage we do the actual block allocation (get_block writh create = 1) looking at the flag. > > The comment: > > /* > + * The above get_blocks can cause the buffer to be > + * marked unwritten. So clear the same. > + */ > + clear_buffer_unwritten(bh); > > is imho not helpful; to me it says "we -just- set this, so clear it!" > It leaves me scratching my head. > > > 3) __get_block(create = 1) -> get the blocks out of prealloc space. > > and retun with BH_Mapped set. > > > > That would imply we have BH_Unwritten and BH_Mapped set in the above > > case which is wrong. So we need a BH_Unwritten clear between (2) and > > (3). The patch does the same. May be we need to capture it in commit > > message. > > Better in comments, I think. :) > -aneesh -- 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
