Relocking page in writepage

Relocking page in writepage

[Martin Jambor]

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?

Thank you very much for any insight,

Martin Jambor

Re: Relocking page in writepage

[Andrew Morton]

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.

Re: Relocking page in writepage

[Martin Jambor]

On 2/25/06, Andrew Morton <akpm@osdl.org> wrote:
> 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.

Well, I set writeback much later, only after I know there wouldn't be
any errors.
However, I was thinking if the following alternative solution would be ok:

set_page_writeback(page);
unlock_page(page)

/* do something that might deadlock */

lock_page(page);

if (error) {        /* -EIO, WRITEPAGE_ACTIVATE etc... */
        test_clear_page_writeback(page);
	wake_up_page(page, PG_writeback);
        return error;
}
/* continue as usual */

The problem with it is that test_clear_page_writeback() and
wake_up_page() are not exported for modules (which is a slight problem
at this stage of our project). On the other hand a page under
writeback shouldn't be stripped off its mapping or rewritten etc.
which simplifies things a lot.

Thanks for the input and any further comments, meanwhile I'll play
around with it and see what I can get away with.

Martin