On Wed, May 13, 2009 at 01:56:56PM -0500, Eric Sandeen wrote: > 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. I've updated it the comment to say this instead. /* * When we call get_blocks without the create flag, the * BH_Unwritten flag could have gotten set if the blocks * requested were part of a uninitialized extent. We need to * clear this flag now that we are committed to convert all or * part of the uninitialized extent to be an initialized * extent. This is because we need to avoid the combination * of BH_Unwritten and BH_Mapped flags being simultaneously * set on the buffer_head. */ I'm still not entirely clear what is the downside of having BH_Unwritten and BH_Mapped at the same time; whether it is just a CPU time-waster that will cause some unneeded calls to get_blocks() in the write_begin path which "just" wastes a little CPU, or whether there are other massive disasters that take place on the combination of BH_Unwritten and BH_Mapped. What we *desperately* need is documentation that describes which functions sets these flags, and whether they are intended for long-term use (either stored in buffer heads attached to a page, or in the buffer cache, and if so, which), or just as short-term hacks to pass information between two functions, or multiple functions deep in a call stack, and if so, what the implications are of the bits, and when they should be cleared --- and then we should add assert's or BUG_ON's to enforce what we think the abstraction invariant should be. Magic flags attached to objects that are really there because we don't want to change function signatures are very scary; for example, i_delalloc_reserved_flag in ext4_inode_info --- it's not clear to me exactly what this is supposed to do, but I note that mballoc.c does a lot more with the flag than balloc.c. So it's not clear to me if there is magic semantics associated with that flag which are being done in mballoc.c, but we never got around to implementing them in balloc.c. This leads to the kind of code fragility that I've been complaining about for some time. The fact that Aneesh missed one of these magic code paths in his patch attests to why this style of programming is really bad and not long-term maintainable. So we need to document all of this mess, and then gradually clean it up, one tiny patch at a time, each one describing what we were doing before, and what the new way of doing this should be, and why it's safe to make that change. > > 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. :) The comprehensive description of all of this should be in comments, yes. See the example of the sort of comments that I've added in the clear-unwritten-bh-flag and initialize-map_bh-state. If we don't like such verbose comments, then we should use simpler programming semantics when passing information between the various abstracton layers. :-) - Ted -- 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
