Re: Relocking page in writepage

[Andrew Morton <akpm@osdl.org>]

Martin Jambor <jambormartin@gmail.com> wrote:
>
> Hi,
> 
> is it ok to do something like the following in aops->writepage (or in
> writepages after calling clear_page_dirty_for_io(page) for that
> matter):
> 
>         struct address_space mapping = page->mapping;
> 	page_cache_get(page);
> 	unlock_page(page);
> 
>         /* do something that might deadlock if page was locked */
> 
> 	lock_page(page);
> 	page_cache_release(page);
> 
> 	if (page->mapping != mapping) { /* truncated while doing the above */
> 		unlock_page(page);
> 		return 0;
> 	}
> 
>         /* continue as usual  (i.e. initiate writing, set writeback
> and unlock) */
> 
> A few practical tests showed no problem but I thought it would be
> better to ask anyway...
> I know it isn't a nice thing to do but otherwise I would probably need
> to implement some mechanism to remember the page I have locked and
> never try to lock (or page_cache_grab) it again. Both are cumbersome
> but this is easier, what do you think about it?
> 

I can't immediately think of a problem with that.

I guess it would be nicer to do:

	lock_page();
	if (page->mapping != mapping) {
		unlock_page();
		page_cache_release();

so the page get freed immediately if the race happened, rather than having
it drift down the LRU.

umm, actually you do need to handle the case where someone came in and
redirtied the page and potentially stated writeback against it.

	lock_page();
	wait_on_page_writeback();
	if (page->mapping != mapping) {
		unlock_page();
		page_cache_release();
		return;
	}

	Now, we don't know whether to write the page.  Someone else might have
	redirtied it and written it while w dropped the lock.

So you have to go off and write it, occasionally unnecessarily.

umm, no.  If you leave PageWriteback() set throughout, nobody will try to
restart a write.  So after relocking the page you should run
clear_page_dirty() to cover that case, then write it.

Tricky.