Re: 2.6.19 file content corruption on ext3

[Nick Piggin]

Peter Zijlstra wrote:
> On Tue, 2006-12-19 at 15:36 +1100, Nick Piggin wrote:
>
>
> > plain text document attachment (fs-fix.patch)
> > Index: linux-2.6/fs/buffer.c
> > ===================================================================
> > --- linux-2.6.orig/fs/buffer.c	2006-12-19 15:15:46.000000000 +1100
> > +++ linux-2.6/fs/buffer.c	2006-12-19 15:36:01.000000000 +1100
> > @@ -2852,7 +2852,17 @@ int try_to_free_buffers(struct page *pag
> > 		 * This only applies in the rare case where try_to_free_buffers
> > 		 * succeeds but the page is not freed.
> > 		 */
> > -		clear_page_dirty(page);
> > +
> > +		/*
> > +		 * If the page has been dirtied via the user mappings, then
> > +		 * clean buffers does not indicate the page data is actually
> > +		 * clean! Only clear the page dirty bit if there are no dirty
> > +		 * ptes either.
> > +		 *
> > +		 * If there are dirty ptes, then the page must be uptodate, so
> > +		 * the above concern does not apply.
> > +		 */
> > +		clear_page_dirty_sync_ptes(page);
> > 	}
> > out:
> > 	if (buffers_to_free) {
> > Index: linux-2.6/include/linux/page-flags.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/page-flags.h	2006-12-19 15:17:18.000000000 +1100
> > +++ linux-2.6/include/linux/page-flags.h	2006-12-19 15:34:24.000000000 +1100
> > @@ -254,6 +254,7 @@ static inline void SetPageUptodate(struc
> > struct page;	/* forward declaration */
> >
> > int test_clear_page_dirty(struct page *page);
> > +int test_clear_page_dirty_sync_ptes(struct page *page);
> > int test_clear_page_writeback(struct page *page);
> > int test_set_page_writeback(struct page *page);
> >
> > @@ -262,6 +263,11 @@ static inline void clear_page_dirty(stru
> > 	test_clear_page_dirty(page);
> > }
> >
> > +static inline void clear_page_dirty_sync_ptes(struct page *page)
> > +{
> > +	test_clear_page_dirty_sync_ptes(page);
> > +}
> > +
> > static inline void set_page_writeback(struct page *page)
> > {
> > 	test_set_page_writeback(page);
> > Index: linux-2.6/mm/page-writeback.c
> > ===================================================================
> > --- linux-2.6.orig/mm/page-writeback.c	2006-12-19 15:17:53.000000000 +1100
> > +++ linux-2.6/mm/page-writeback.c	2006-12-19 15:33:29.000000000 +1100
> > @@ -844,9 +844,10 @@ EXPORT_SYMBOL(set_page_dirty_lock);
> >
> > /*
> >  * Clear a page's dirty flag, while caring for dirty memory accounting. 
> > + * Does not clear pte dirty bits.
> >  * Returns true if the page was previously dirty.
> >  */
> > -int test_clear_page_dirty(struct page *page)
> > +static int test_clear_page_dirty_leave_ptes(struct page *page)
> > {
> > 	struct address_space *mapping = page_mapping(page);
> > 	unsigned long flags;
> > @@ -862,10 +863,8 @@ int test_clear_page_dirty(struct page *p
> > 			 * We can continue to use `mapping' here because the
> > 			 * page is locked, which pins the address_space
> > 			 */
> > -			if (mapping_cap_account_dirty(mapping)) {
> > -				page_mkclean(page);
> > +			if (mapping_cap_account_dirty(mapping))
> > 				dec_zone_page_state(page, NR_FILE_DIRTY);
> > -			}
> > 			return 1;
> > 		}
> > 		write_unlock_irqrestore(&mapping->tree_lock, flags);
> > @@ -873,9 +872,43 @@ int test_clear_page_dirty(struct page *p
> > 	}
> > 	return TestClearPageDirty(page);
> > }
> > +
> > +/*
> > + * As above, but does clear dirty bits from ptes
> > + */
> > +int test_clear_page_dirty(struct page *page)
> > +{
> > +	struct address_space *mapping = page_mapping(page);
> > +
> > +	if (test_clear_page_dirty_leave_ptes(page)) {
> > +		if (mapping_cap_account_dirty(mapping))
> > +			page_mkclean(page);
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> > EXPORT_SYMBOL(test_clear_page_dirty);
> >
> > /*
> > + * As above, but redirties page if any dirty ptes are found (and then only
> > + * if the mapping accounts dirty pages, otherwise dirty ptes are left dirty
> > + * but the page is cleaned).
> > + */
> > +int test_clear_page_dirty_sync_ptes(struct page *page)
> > +{
> > +	struct address_space *mapping = page_mapping(page);
> > +
> > +	if (test_clear_page_dirty_leave_ptes(page)) {
> > +		if (mapping_cap_account_dirty(mapping)) {
> > +			if (page_mkclean(page))
> > +				set_page_dirty(page);
> > +		}
> > +		return 1;
> > +	}
> > +	return 0;
> > +}
> > +
> > +/*
> >  * Clear a page's dirty flag, while caring for dirty memory accounting.
> >  * Returns true if the page was previously dirty.
> >  *
>
>
> Hmm, not quite; It certainly look better than the extra ,[01] tagged to
> test_clear_page_dirty() though. Although I would have expected it the
> other way around - test_clear_pages_dirty_sync_ptes to be the default
> case and test_clear_pages_dirty_clean_ptes to be used in
> clear_page_dirty_for_io().
>
> Anyway it has the same issues as the others. See what happens when you
> run two test_clear_page_dirty_sync_ptes() consecutively, you still loose
> PG_dirty even though the page might actually be dirty.

How can this happen? We'll only test_clear_page_dirty_sync_ptes again
after buffers have been reattached, and subsequently cleaned. And in
that case if the ptes are still clean at this point then the page really
is clean.

--
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/