On Fri, 29 Jun 2007, Hugh Dickins wrote: > I stand by my page_mapping patch, and the remark I made before, > that page_mapping(page) is the correct place to check this. What is > page_mapping(page) for? Precisely to return the struct address_space* > from page->mapping when that's what's in there, and not when that field > has been reused for something else. > > So lines like > > + mapping = PageSlab(page) ? NULL : page_mapping(page); > seem to miss the point.
They certainly point out to the reader that one can expect a slab page here where one may not expect one since flush_dcache_page is a page cache function. > I agree that the only clash found yet has been in flush_dcache_page, > so some bytes and branches can indeed be saved by just doing the > test in there. Oh, but your VM_BUG_ON cancels out that saving. > And if we were to try to save bytes and branches there, it's the > synthetic swapper_space business (only required in a couple of > places) I'd be wanting to cut out. VM_BUG_ON is different from BUG_ON. BUG_ON is always checked. VM_BUG_ON depends on a debug config option. > To me this all seems like a big fuss to excuse your surprise: I am weirded out by the use of terms like "accusations" and "excuses" in these discussions. But if it makes you feel better.... Others seemed to have encountered the same surprises before me. So it is better to point out these issues in the sources. There is the danger of other pagecache functions getting called for slab pages in the I/O layer and the check in page_mapping provides some protection from such changes. The checks/comments in the functions where we allow slab page use help the ones enhancing the code to keep these issues in mind and help them to not be surprised in turn. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
