Re: [RFC][PATCH 1/3] tracking dirty pages in shared mappings -V4

[Nick Piggin <piggin@cyberone.com.au>]

Andrew Morton wrote:

>Nick Piggin <piggin@cyberone.com.au> wrote:
>
>> >So let's see.  We take a write fault, we mark the page dirty then we return
>> >to userspace which will proceed with the write and will mark the pte dirty.
>> >
>> >Later, the VM will write the page out.
>> >
>> >Later still, the pte will get cleaned by reclaim or by munmap or whatever
>> >and the page will be marked dirty and the page will again be written out. 
>> >Potentially needlessly.
>> >
>>
>> page_wrprotect also marks the page clean,
>>
>
>Oh.  I missed that when reading the comment which describes
>page_wrprotect() (I do go on).
>

I guess page_wrprotect isn't a good name, because it would suggest it
can be used in situations where it would cause data loss.

page_mappings_mkclean or page_mkclean might be better (the wrprotect
is just a side effect of the fact that clean,shared mappings are
readonly).

>
>>so this window is very small.
>> The window is that the fault path might set_page_dirty, then throttle
>> on writeout, and the page gets written out before it really gets dirtied
>> by the application (which then has to fault again).
>>
>
>: int test_clear_page_dirty(struct page *page)
>: {
>: 	struct address_space *mapping = page_mapping(page);
>: 	unsigned long flags;
>: 
>: 	if (mapping) {
>: 		write_lock_irqsave(&mapping->tree_lock, flags);
>: 		if (TestClearPageDirty(page)) {
>: 			radix_tree_tag_clear(&mapping->page_tree,
>: 						page_index(page),
>: 						PAGECACHE_TAG_DIRTY);
>: 			write_unlock_irqrestore(&mapping->tree_lock, flags);
>: 			/*
>: 			 * We can continue to use `mapping' here because the
>: 			 * page is locked, which pins the address_space
>: 			 */
>
>So if userspace modifies the page right here, and marks the pte dirty.
>
>: 			if (mapping_cap_account_dirty(mapping)) {
>: 				page_wrprotect(page);
>
>We just lost that pte dirty bit, and hence the user's data.
>
>: 				dec_page_state(nr_dirty);
>: 			}
>: 			return 1;
>: 		}
>: 		write_unlock_irqrestore(&mapping->tree_lock, flags);
>: 		return 0;
>: 	}
>: 	return TestClearPageDirty(page);
>: }
>: 
>
>Which is just the sort of subtle and nasty problem I was referring to...
>
>If that's correct then I guess we need the
>
>                if (ptep_clear_flush_dirty(vma, addr, pte) ||
>                                page_test_and_clear_dirty(page))
>                        ret += set_page_dirty(page);
>
>treatment in page_wrprotect().
>
>Now I suppose it's not really a dataloss race, because in practice the
>kernel is about to write this page to backing store anwyay.  I guess.  I
>cannot immediately think of any clear_page_dirty() callers for whom that
>won't be true.
>

If they do a clear_page_dirty, then fail to clean the page, then fail to
subsequently set_page_dirty again, it is a data-loss situation anyway.

If they do set_page_dirty (which they have to, for correctness), then the
page has PG_dirty set again; true dirty bits are moved out of the ptes,
but that's no problem.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>