ocfs2-devel.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [Ocfs2-devel] [PATCH] ocfs2: Fix data corruption after conversion from inline format
@ 2021-09-30  9:54 Jan Kara
  2021-10-09  3:46 ` Joseph Qi
  0 siblings, 1 reply; 3+ messages in thread
From: Jan Kara @ 2021-09-30  9:54 UTC (permalink / raw)
  To: ocfs2-devel; +Cc: Jan Kara, stable

Commit 6dbf7bb55598 ("fs: Don't invalidate page buffers in
block_write_full_page()") uncovered a latent bug in ocfs2 conversion
from inline inode format to a normal inode format. The code in
ocfs2_convert_inline_data_to_extents() attempts to zero out the whole
cluster allocated for file data by grabbing, zeroing, and dirtying all
pages covering this cluster. However these pages are beyond i_size, thus
writeback code generally ignores these dirty pages and no blocks were
ever actually zeroed on the disk. This oversight was fixed by commit
693c241a5f6a ("ocfs2: No need to zero pages past i_size.") for standard
ocfs2 write path, inline conversion path was apparently forgotten; the
commit log also has a reasoning why the zeroing actually is not needed.
After commit 6dbf7bb55598, things became worse as writeback code stopped
invalidating buffers on pages beyond i_size and thus these pages end up
with clean PageDirty bit but with buffers attached to these pages being
still dirty. So when a file is converted from inline format, then
writeback triggers, and then the file is grown so that these pages
become valid, the invalid dirtiness state is preserved,
mark_buffer_dirty() does nothing on these pages (buffers are already
dirty) but page is never written back because it is clean. So data
written to these pages is lost once pages are reclaimed.

Simple reproducer for the problem is:

xfs_io -f -c "pwrite 0 2000" -c "pwrite 2000 2000" -c "fsync" \
  -c "pwrite 4000 2000" ocfs2_file

After unmounting and mounting the fs again, you can observe that end of
'ocfs2_file' has lost its contents.

Fix the problem by not doing the pointless zeroing during conversion
from inline format similarly as in the standard write path.

CC: stable@vger.kernel.org
Fixes: 6dbf7bb55598 ("fs: Don't invalidate page buffers in block_write_full_page()")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ocfs2/alloc.c | 46 ++++++++++++----------------------------------
 1 file changed, 12 insertions(+), 34 deletions(-)

diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
index f1cc8258d34a..12aead0dabe2 100644
--- a/fs/ocfs2/alloc.c
+++ b/fs/ocfs2/alloc.c
@@ -7045,7 +7045,7 @@ void ocfs2_set_inode_data_inline(struct inode *inode, struct ocfs2_dinode *di)
 int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 					 struct buffer_head *di_bh)
 {
-	int ret, i, has_data, num_pages = 0;
+	int ret,has_data, num_pages = 0;
 	int need_free = 0;
 	u32 bit_off, num;
 	handle_t *handle;
@@ -7054,26 +7054,17 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
 	struct ocfs2_alloc_context *data_ac = NULL;
-	struct page **pages = NULL;
-	loff_t end = osb->s_clustersize;
+	struct page *page = NULL;
 	struct ocfs2_extent_tree et;
 	int did_quota = 0;
 
 	has_data = i_size_read(inode) ? 1 : 0;
 
 	if (has_data) {
-		pages = kcalloc(ocfs2_pages_per_cluster(osb->sb),
-				sizeof(struct page *), GFP_NOFS);
-		if (pages == NULL) {
-			ret = -ENOMEM;
-			mlog_errno(ret);
-			return ret;
-		}
-
 		ret = ocfs2_reserve_clusters(osb, 1, &data_ac);
 		if (ret) {
 			mlog_errno(ret);
-			goto free_pages;
+			goto out;
 		}
 	}
 
@@ -7093,7 +7084,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 	}
 
 	if (has_data) {
-		unsigned int page_end;
+		unsigned int page_end = min_t(unsigned, PAGE_SIZE,
+							osb->s_clustersize);
 		u64 phys;
 
 		ret = dquot_alloc_space_nodirty(inode,
@@ -7117,15 +7109,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 		 */
 		block = phys = ocfs2_clusters_to_blocks(inode->i_sb, bit_off);
 
-		/*
-		 * Non sparse file systems zero on extend, so no need
-		 * to do that now.
-		 */
-		if (!ocfs2_sparse_alloc(osb) &&
-		    PAGE_SIZE < osb->s_clustersize)
-			end = PAGE_SIZE;
-
-		ret = ocfs2_grab_eof_pages(inode, 0, end, pages, &num_pages);
+		ret = ocfs2_grab_eof_pages(inode, 0, page_end, &page,
+					   &num_pages);
 		if (ret) {
 			mlog_errno(ret);
 			need_free = 1;
@@ -7136,20 +7121,15 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 		 * This should populate the 1st page for us and mark
 		 * it up to date.
 		 */
-		ret = ocfs2_read_inline_data(inode, pages[0], di_bh);
+		ret = ocfs2_read_inline_data(inode, page, di_bh);
 		if (ret) {
 			mlog_errno(ret);
 			need_free = 1;
 			goto out_unlock;
 		}
 
-		page_end = PAGE_SIZE;
-		if (PAGE_SIZE > osb->s_clustersize)
-			page_end = osb->s_clustersize;
-
-		for (i = 0; i < num_pages; i++)
-			ocfs2_map_and_dirty_page(inode, handle, 0, page_end,
-						 pages[i], i > 0, &phys);
+		ocfs2_map_and_dirty_page(inode, handle, 0, page_end, page, 0,
+					 &phys);
 	}
 
 	spin_lock(&oi->ip_lock);
@@ -7180,8 +7160,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 	}
 
 out_unlock:
-	if (pages)
-		ocfs2_unlock_and_free_pages(pages, num_pages);
+	if (page)
+		ocfs2_unlock_and_free_pages(&page, num_pages);
 
 out_commit:
 	if (ret < 0 && did_quota)
@@ -7205,8 +7185,6 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
 out:
 	if (data_ac)
 		ocfs2_free_alloc_context(data_ac);
-free_pages:
-	kfree(pages);
 	return ret;
 }
 
-- 
2.26.2


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [Ocfs2-devel] [PATCH] ocfs2: Fix data corruption after conversion from inline format
  2021-09-30  9:54 [Ocfs2-devel] [PATCH] ocfs2: Fix data corruption after conversion from inline format Jan Kara
@ 2021-10-09  3:46 ` Joseph Qi
  2021-10-11  9:46   ` Gang He
  0 siblings, 1 reply; 3+ messages in thread
From: Joseph Qi @ 2021-10-09  3:46 UTC (permalink / raw)
  To: Jan Kara, akpm; +Cc: Markov, Andrey, ocfs2-devel



On 9/30/21 5:54 PM, Jan Kara wrote:
> Commit 6dbf7bb55598 ("fs: Don't invalidate page buffers in
> block_write_full_page()") uncovered a latent bug in ocfs2 conversion
> from inline inode format to a normal inode format. The code in
> ocfs2_convert_inline_data_to_extents() attempts to zero out the whole
> cluster allocated for file data by grabbing, zeroing, and dirtying all
> pages covering this cluster. However these pages are beyond i_size, thus
> writeback code generally ignores these dirty pages and no blocks were
> ever actually zeroed on the disk. This oversight was fixed by commit
> 693c241a5f6a ("ocfs2: No need to zero pages past i_size.") for standard
> ocfs2 write path, inline conversion path was apparently forgotten; the
> commit log also has a reasoning why the zeroing actually is not needed.
> After commit 6dbf7bb55598, things became worse as writeback code stopped
> invalidating buffers on pages beyond i_size and thus these pages end up
> with clean PageDirty bit but with buffers attached to these pages being
> still dirty. So when a file is converted from inline format, then
> writeback triggers, and then the file is grown so that these pages
> become valid, the invalid dirtiness state is preserved,
> mark_buffer_dirty() does nothing on these pages (buffers are already
> dirty) but page is never written back because it is clean. So data
> written to these pages is lost once pages are reclaimed.
> 
> Simple reproducer for the problem is:
> 
> xfs_io -f -c "pwrite 0 2000" -c "pwrite 2000 2000" -c "fsync" \
>   -c "pwrite 4000 2000" ocfs2_file
> 
> After unmounting and mounting the fs again, you can observe that end of
> 'ocfs2_file' has lost its contents.
> 
> Fix the problem by not doing the pointless zeroing during conversion
> from inline format similarly as in the standard write path.
> 
> CC: stable@vger.kernel.org
> Fixes: 6dbf7bb55598 ("fs: Don't invalidate page buffers in block_write_full_page()")
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ocfs2/alloc.c | 46 ++++++++++++----------------------------------
>  1 file changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
> index f1cc8258d34a..12aead0dabe2 100644
> --- a/fs/ocfs2/alloc.c
> +++ b/fs/ocfs2/alloc.c
> @@ -7045,7 +7045,7 @@ void ocfs2_set_inode_data_inline(struct inode *inode, struct ocfs2_dinode *di)
>  int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>  					 struct buffer_head *di_bh)
>  {
> -	int ret, i, has_data, num_pages = 0;
> +	int ret,has_data, num_pages = 0;

Should leave a space between 'ret' and 'has_data'.

Other looks good to me.
Reviewed-and-Tested-by: Joseph Qi <joseph.qi@linux.alibaba.com>

Cc Gang and Andrey Markov who have encountered the same issue before. 

>  	int need_free = 0;
>  	u32 bit_off, num;
>  	handle_t *handle;
> @@ -7054,26 +7054,17 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  	struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
>  	struct ocfs2_alloc_context *data_ac = NULL;
> -	struct page **pages = NULL;
> -	loff_t end = osb->s_clustersize;
> +	struct page *page = NULL;
>  	struct ocfs2_extent_tree et;
>  	int did_quota = 0;
>  
>  	has_data = i_size_read(inode) ? 1 : 0;
>  
>  	if (has_data) {
> -		pages = kcalloc(ocfs2_pages_per_cluster(osb->sb),
> -				sizeof(struct page *), GFP_NOFS);
> -		if (pages == NULL) {
> -			ret = -ENOMEM;
> -			mlog_errno(ret);
> -			return ret;
> -		}
> -
>  		ret = ocfs2_reserve_clusters(osb, 1, &data_ac);
>  		if (ret) {
>  			mlog_errno(ret);
> -			goto free_pages;
> +			goto out;
>  		}
>  	}
>  
> @@ -7093,7 +7084,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>  	}
>  
>  	if (has_data) {
> -		unsigned int page_end;
> +		unsigned int page_end = min_t(unsigned, PAGE_SIZE,
> +							osb->s_clustersize);
>  		u64 phys;
>  
>  		ret = dquot_alloc_space_nodirty(inode,
> @@ -7117,15 +7109,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>  		 */
>  		block = phys = ocfs2_clusters_to_blocks(inode->i_sb, bit_off);
>  
> -		/*
> -		 * Non sparse file systems zero on extend, so no need
> -		 * to do that now.
> -		 */
> -		if (!ocfs2_sparse_alloc(osb) &&
> -		    PAGE_SIZE < osb->s_clustersize)
> -			end = PAGE_SIZE;
> -
> -		ret = ocfs2_grab_eof_pages(inode, 0, end, pages, &num_pages);
> +		ret = ocfs2_grab_eof_pages(inode, 0, page_end, &page,
> +					   &num_pages);
>  		if (ret) {
>  			mlog_errno(ret);
>  			need_free = 1;
> @@ -7136,20 +7121,15 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>  		 * This should populate the 1st page for us and mark
>  		 * it up to date.
>  		 */
> -		ret = ocfs2_read_inline_data(inode, pages[0], di_bh);
> +		ret = ocfs2_read_inline_data(inode, page, di_bh);
>  		if (ret) {
>  			mlog_errno(ret);
>  			need_free = 1;
>  			goto out_unlock;
>  		}
>  
> -		page_end = PAGE_SIZE;
> -		if (PAGE_SIZE > osb->s_clustersize)
> -			page_end = osb->s_clustersize;
> -
> -		for (i = 0; i < num_pages; i++)
> -			ocfs2_map_and_dirty_page(inode, handle, 0, page_end,
> -						 pages[i], i > 0, &phys);
> +		ocfs2_map_and_dirty_page(inode, handle, 0, page_end, page, 0,
> +					 &phys);
>  	}
>  
>  	spin_lock(&oi->ip_lock);
> @@ -7180,8 +7160,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>  	}
>  
>  out_unlock:
> -	if (pages)
> -		ocfs2_unlock_and_free_pages(pages, num_pages);
> +	if (page)
> +		ocfs2_unlock_and_free_pages(&page, num_pages);
>  
>  out_commit:
>  	if (ret < 0 && did_quota)
> @@ -7205,8 +7185,6 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>  out:
>  	if (data_ac)
>  		ocfs2_free_alloc_context(data_ac);
> -free_pages:
> -	kfree(pages);
>  	return ret;
>  }
>  
> 

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Ocfs2-devel] [PATCH] ocfs2: Fix data corruption after conversion from inline format
  2021-10-09  3:46 ` Joseph Qi
@ 2021-10-11  9:46   ` Gang He
  0 siblings, 0 replies; 3+ messages in thread
From: Gang He @ 2021-10-11  9:46 UTC (permalink / raw)
  To: Joseph Qi, Jan Kara, akpm; +Cc: Markov, Andrey, ocfs2-devel

Hi Jan/Joseph,

The code change looks OK for me,
The testing result for ocfs2-test test suits is OK.

Thanks
Gang

On 2021/10/9 11:46, Joseph Qi wrote:
> 
> 
> On 9/30/21 5:54 PM, Jan Kara wrote:
>> Commit 6dbf7bb55598 ("fs: Don't invalidate page buffers in
>> block_write_full_page()") uncovered a latent bug in ocfs2 conversion
>> from inline inode format to a normal inode format. The code in
>> ocfs2_convert_inline_data_to_extents() attempts to zero out the whole
>> cluster allocated for file data by grabbing, zeroing, and dirtying all
>> pages covering this cluster. However these pages are beyond i_size, thus
>> writeback code generally ignores these dirty pages and no blocks were
>> ever actually zeroed on the disk. This oversight was fixed by commit
>> 693c241a5f6a ("ocfs2: No need to zero pages past i_size.") for standard
>> ocfs2 write path, inline conversion path was apparently forgotten; the
>> commit log also has a reasoning why the zeroing actually is not needed.
>> After commit 6dbf7bb55598, things became worse as writeback code stopped
>> invalidating buffers on pages beyond i_size and thus these pages end up
>> with clean PageDirty bit but with buffers attached to these pages being
>> still dirty. So when a file is converted from inline format, then
>> writeback triggers, and then the file is grown so that these pages
>> become valid, the invalid dirtiness state is preserved,
>> mark_buffer_dirty() does nothing on these pages (buffers are already
>> dirty) but page is never written back because it is clean. So data
>> written to these pages is lost once pages are reclaimed.
>>
>> Simple reproducer for the problem is:
>>
>> xfs_io -f -c "pwrite 0 2000" -c "pwrite 2000 2000" -c "fsync" \
>>    -c "pwrite 4000 2000" ocfs2_file
>>
>> After unmounting and mounting the fs again, you can observe that end of
>> 'ocfs2_file' has lost its contents.
>>
>> Fix the problem by not doing the pointless zeroing during conversion
>> from inline format similarly as in the standard write path.
>>
>> CC: stable@vger.kernel.org
>> Fixes: 6dbf7bb55598 ("fs: Don't invalidate page buffers in block_write_full_page()")
>> Signed-off-by: Jan Kara <jack@suse.cz>
>> ---
>>   fs/ocfs2/alloc.c | 46 ++++++++++++----------------------------------
>>   1 file changed, 12 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/ocfs2/alloc.c b/fs/ocfs2/alloc.c
>> index f1cc8258d34a..12aead0dabe2 100644
>> --- a/fs/ocfs2/alloc.c
>> +++ b/fs/ocfs2/alloc.c
>> @@ -7045,7 +7045,7 @@ void ocfs2_set_inode_data_inline(struct inode *inode, struct ocfs2_dinode *di)
>>   int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>>   					 struct buffer_head *di_bh)
>>   {
>> -	int ret, i, has_data, num_pages = 0;
>> +	int ret,has_data, num_pages = 0;
> 
> Should leave a space between 'ret' and 'has_data'.
> 
> Other looks good to me.
> Reviewed-and-Tested-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> 
> Cc Gang and Andrey Markov who have encountered the same issue before.
> 
>>   	int need_free = 0;
>>   	u32 bit_off, num;
>>   	handle_t *handle;
>> @@ -7054,26 +7054,17 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>>   	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>>   	struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
>>   	struct ocfs2_alloc_context *data_ac = NULL;
>> -	struct page **pages = NULL;
>> -	loff_t end = osb->s_clustersize;
>> +	struct page *page = NULL;
>>   	struct ocfs2_extent_tree et;
>>   	int did_quota = 0;
>>   
>>   	has_data = i_size_read(inode) ? 1 : 0;
>>   
>>   	if (has_data) {
>> -		pages = kcalloc(ocfs2_pages_per_cluster(osb->sb),
>> -				sizeof(struct page *), GFP_NOFS);
>> -		if (pages == NULL) {
>> -			ret = -ENOMEM;
>> -			mlog_errno(ret);
>> -			return ret;
>> -		}
>> -
>>   		ret = ocfs2_reserve_clusters(osb, 1, &data_ac);
>>   		if (ret) {
>>   			mlog_errno(ret);
>> -			goto free_pages;
>> +			goto out;
>>   		}
>>   	}
>>   
>> @@ -7093,7 +7084,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>>   	}
>>   
>>   	if (has_data) {
>> -		unsigned int page_end;
>> +		unsigned int page_end = min_t(unsigned, PAGE_SIZE,
>> +							osb->s_clustersize);
>>   		u64 phys;
>>   
>>   		ret = dquot_alloc_space_nodirty(inode,
>> @@ -7117,15 +7109,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>>   		 */
>>   		block = phys = ocfs2_clusters_to_blocks(inode->i_sb, bit_off);
>>   
>> -		/*
>> -		 * Non sparse file systems zero on extend, so no need
>> -		 * to do that now.
>> -		 */
>> -		if (!ocfs2_sparse_alloc(osb) &&
>> -		    PAGE_SIZE < osb->s_clustersize)
>> -			end = PAGE_SIZE;
>> -
>> -		ret = ocfs2_grab_eof_pages(inode, 0, end, pages, &num_pages);
>> +		ret = ocfs2_grab_eof_pages(inode, 0, page_end, &page,
>> +					   &num_pages);
>>   		if (ret) {
>>   			mlog_errno(ret);
>>   			need_free = 1;
>> @@ -7136,20 +7121,15 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>>   		 * This should populate the 1st page for us and mark
>>   		 * it up to date.
>>   		 */
>> -		ret = ocfs2_read_inline_data(inode, pages[0], di_bh);
>> +		ret = ocfs2_read_inline_data(inode, page, di_bh);
>>   		if (ret) {
>>   			mlog_errno(ret);
>>   			need_free = 1;
>>   			goto out_unlock;
>>   		}
>>   
>> -		page_end = PAGE_SIZE;
>> -		if (PAGE_SIZE > osb->s_clustersize)
>> -			page_end = osb->s_clustersize;
>> -
>> -		for (i = 0; i < num_pages; i++)
>> -			ocfs2_map_and_dirty_page(inode, handle, 0, page_end,
>> -						 pages[i], i > 0, &phys);
>> +		ocfs2_map_and_dirty_page(inode, handle, 0, page_end, page, 0,
>> +					 &phys);
>>   	}
>>   
>>   	spin_lock(&oi->ip_lock);
>> @@ -7180,8 +7160,8 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>>   	}
>>   
>>   out_unlock:
>> -	if (pages)
>> -		ocfs2_unlock_and_free_pages(pages, num_pages);
>> +	if (page)
>> +		ocfs2_unlock_and_free_pages(&page, num_pages);
>>   
>>   out_commit:
>>   	if (ret < 0 && did_quota)
>> @@ -7205,8 +7185,6 @@ int ocfs2_convert_inline_data_to_extents(struct inode *inode,
>>   out:
>>   	if (data_ac)
>>   		ocfs2_free_alloc_context(data_ac);
>> -free_pages:
>> -	kfree(pages);
>>   	return ret;
>>   }
>>   
>>
> 


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-11  9:50 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-30  9:54 [Ocfs2-devel] [PATCH] ocfs2: Fix data corruption after conversion from inline format Jan Kara
2021-10-09  3:46 ` Joseph Qi
2021-10-11  9:46   ` Gang He

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).