Re: WARNING: "test_clear_page_dirty" [fs/cifs/cifs.ko] undefined!

[Randy Dunlap <randy.dunlap@oracle.com>]

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