Re: [PATCH v2] ext4: fix data integrity sync in ordered mode

[Jan Kara <jack@suse.cz>]

On Tue 06-05-14 15:49:29, Namjae Jeon wrote:
> When we perform a data integrity sync we tag all the dirty pages with
> PAGECACHE_TAG_TOWRITE at start of ext4_da_writepages.
> Later we check for this tag in write_cache_pages_da and creates a
> struct mpage_da_data containing contiguously indexed pages tagged with this
> tag and sync these pages with a call to mpage_da_map_and_submit.
> This process is done in while loop until all the PAGECACHE_TAG_TOWRITE pages
> are synced. We also do journal start and stop in each iteration.
> journal_stop could initiate journal commit which would call ext4_writepage
> which in turn will call ext4_bio_write_page even for delayed OR unwritten
> buffers. When ext4_bio_write_page is called for such buffers, even though it
> does not sync them but it clears the PAGECACHE_TAG_TOWRITE of the corresponding
> page and hence these pages are also not synced by the currently running data
> integrity sync. We will end up with dirty pages although sync is completed.
> 
> This could cause a potential data loss when the sync call is followed by a
> truncate_pagecache call, which is exactly the case in collapse_range.
> (It will cause generic/127 failure in xfstests)
> 
> Cc: Jan Kara <jack@suse.cz>
> Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> Signed-off-by: Ashish Sangwan <a.sangwan@samsung.com>
  Just one comment below:

...
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 023cf08..f7358c5 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2438,6 +2438,43 @@ int test_set_page_writeback(struct page *page)
>  }
>  EXPORT_SYMBOL(test_set_page_writeback);
>  
> +int test_set_page_writeback_keepwrite(struct page *page)
> +{
> +	struct address_space *mapping = page_mapping(page);
> +	int ret;
> +	bool locked;
> +	unsigned long memcg_flags;
> +
> +	mem_cgroup_begin_update_page_stat(page, &locked, &memcg_flags);
> +	if (mapping) {
> +		struct backing_dev_info *bdi = mapping->backing_dev_info;
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&mapping->tree_lock, flags);
> +		ret = TestSetPageWriteback(page);
> +		if (!ret) {
> +			radix_tree_tag_set(&mapping->page_tree,
> +						page_index(page),
> +						PAGECACHE_TAG_WRITEBACK);
> +			if (bdi_cap_account_writeback(bdi))
> +				__inc_bdi_stat(bdi, BDI_WRITEBACK);
> +		}
> +		if (!PageDirty(page))
> +			radix_tree_tag_clear(&mapping->page_tree,
> +						page_index(page),
> +						PAGECACHE_TAG_DIRTY);
> +		spin_unlock_irqrestore(&mapping->tree_lock, flags);
> +	} else {
> +		ret = TestSetPageWriteback(page);
> +	}
> +	if (!ret)
> +		account_page_writeback(page);
> +	mem_cgroup_end_update_page_stat(page, &locked, &memcg_flags);
> +	return ret;
> +
> +}
> +EXPORT_SYMBOL(test_set_page_writeback_keepwrite);
> +
  Since the two variants of test_set_page_writeback() differ only a little
I'd rather have __test_set_page_writeback(page, keep_write) and then define
test_set_page_writeback() and test_set_page_writeback_keepwrite() as calls
to that function. Other than that the patch is OK. Thanks!

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR