On Sat, 23 Dec 2006 10:30:40 -0800 (PST) Linus Torvalds wrote: > > [ Andrew - I'm cc'ing you, because you caused the requirement that people > use "set_page_writeback()" in their writepage() routine that CIFS seems > to have been ignoring all these years. That was introduced more than > two years ago, back in April 11, 2004: > > [PATCH] fdatasync integrity fix > > fdatasync can fail to wait on some pages due to a race. > ... > > and as far as I can see, ever since then, any filesystem that didn't do > a "set_page_writeback()" to sync up the TAG_DIRTY bit would have this > CPU usage problem. Please double-check whether I'm right or barking up > the wrong tree. > > Afaik, the lack of doing the page writeback bit handling properly would > seem to not cause any actual visible _semantic_ problems, it would just > cause fdatasync to not necessarily be entirely reliable (which I guess > is semantic, but very hard to see) and just wasted CPU cycles when we > look up pages that are marked dirty in the radix tree, but aren't > actually really dirty. > > Correct? Who else is implicated in all of this? ] > > On Fri, 22 Dec 2006, Linus Torvalds wrote: > > > > CIFS _should_ be using "clear_page_dirty_for_io()" in that place, and that > > will fix the build. However, the reason I didn't just do that myself is > > that I can't test the end result, and for the life of me, I can't see > > where CIFS does the "end_page_writeback()" that it needs to do at IO > > completion time. > > Ok, I spent some more time looking at this. > > The reason cifs didn't do an "end_page_writeback()" was that it didn't > even do the "set_page_writeback()" to mark the page under writeback in the > first place. > > Now, you might think that since it didn't do a set_page_writeback(), it > doesn't need to do the matching end_page_writeback() at all, and instead > just continue to use the old (_really_ old) way of just unlocking the page > when it is done. > > However, you'd be wrong. The thing is, a "writepage()" function will be > called with the dirty bit cleared in the "struct page *", but the mapping > radix trees will still have the dirty bit set, exactly because the VM > _requires_ the filesystem to tell it what the h*ll it is doing with the > page. So a low-level filesystem must always do one of two things in it's > "writepage()" function. Either: > > - "set_page_writeback()" (and then an "end_page_writeback()" when > finished, of course) > > OR > > - "redirty_page_for_writepage()" to tell the VM to move the page to the > back of the LRU queues because it can't be cleaned (eg, some temporary > problem with write ordering or similar, or something fundamental like > "I'm ramfs, and I don't _have_ any backing store"). > > and if the low-level filesystem doesn't do either of those, then the > status bits in the radix tree that contains the mapping information will > never be updated, so the page that got cleaned will continue to be marked > "dirty" in the radix tree (which admittedly will generally be invisible, > except for "sync()" and friends spending inordinate amounts of time > looking at pages that aren't even dirty any more - since they look things > up by the radix tree tags). > > So I think the old code happened to work, but it was definitely incorrect, > and would leave the dirty tags in the radix tree very confused indeed (it > so happened that "cifs_writepages()" - with an "s" at the end - because it > used "test_clear_page_dirty()" - would also clear the dirty tag, but any > page that went through the generic VM routines and the single-page > "cifs_writepage()" - without an "s" at the end - would then be forever > marked dirty in the radix tree even though it was clean. > > Somebody should check me, though. > > This fairly mindless patch adds the proper "set_page_writeback()" calls > (and the "clear_page_writeback()" ones I had already added before I looked > more closely at this). > > I added a comment in "cifs_writepage()" (the single-page case) for why > this all is the case,
BTW, reiserfs has similar build problems: it uses clear_page_dirty() so it won't build. fs/built-in.o: In function `reiserfs_cut_from_item': (.text.reiserfs_cut_from_item+0x868): undefined reference to `clear_page_dirty' --- ~Randy - 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/
