diff mbox series

[f2fs-dev,v4,2/6] f2fs: compress: fix to cover normal cluster write with cp_rwsem

Message ID 20240111064208.2969599-2-chao@kernel.org (mailing list archive)
State Superseded
Headers show
Series [f2fs-dev,v4,1/6] f2fs: compress: fix to guarantee persisting compressed blocks by CP | expand

Commit Message

Chao Yu Jan. 11, 2024, 6:42 a.m. UTC 
When we overwrite compressed cluster w/ normal cluster, we should
not unlock cp_rwsem during f2fs_write_raw_pages(), otherwise data
will be corrupted if partial blocks were persisted before CP & SPOR,
due to cluster metadata wasn't updated atomically.

Fixes: 4c8ff7095bef ("f2fs: support data compression")
Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/compress.c | 20 ++++++++++++++------
 fs/f2fs/data.c     |  3 ++-
 2 files changed, 16 insertions(+), 7 deletions(-)

Comments

Daeho Jeong Jan. 12, 2024, 10:15 p.m. UTC | #1 
Reviewed-by: Daeho Jeong <daehojeong@google.com>

On Wed, Jan 10, 2024 at 10:43 PM Chao Yu <chao@kernel.org> wrote:
>
> When we overwrite compressed cluster w/ normal cluster, we should
> not unlock cp_rwsem during f2fs_write_raw_pages(), otherwise data
> will be corrupted if partial blocks were persisted before CP & SPOR,
> due to cluster metadata wasn't updated atomically.
>
> Fixes: 4c8ff7095bef ("f2fs: support data compression")
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/compress.c | 20 ++++++++++++++------
>  fs/f2fs/data.c     |  3 ++-
>  2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 9940b7886e5d..bf4cfab67aec 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1448,7 +1448,8 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>                                         enum iostat_type io_type)
>  {
>         struct address_space *mapping = cc->inode->i_mapping;
> -       int _submitted, compr_blocks, ret, i;
> +       struct f2fs_sb_info *sbi = F2FS_M_SB(mapping);
> +       int _submitted, compr_blocks, ret = 0, i;
>
>         compr_blocks = f2fs_compressed_blocks(cc);
>
> @@ -1463,6 +1464,10 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>         if (compr_blocks < 0)
>                 return compr_blocks;
>
> +       /* overwrite compressed cluster w/ normal cluster */
> +       if (compr_blocks > 0)
> +               f2fs_lock_op(sbi);
> +
>         for (i = 0; i < cc->cluster_size; i++) {
>                 if (!cc->rpages[i])
>                         continue;
> @@ -1495,26 +1500,29 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>                                 unlock_page(cc->rpages[i]);
>                                 ret = 0;
>                         } else if (ret == -EAGAIN) {
> +                               ret = 0;
>                                 /*
>                                  * for quota file, just redirty left pages to
>                                  * avoid deadlock caused by cluster update race
>                                  * from foreground operation.
>                                  */
>                                 if (IS_NOQUOTA(cc->inode))
> -                                       return 0;
> -                               ret = 0;
> +                                       goto out;
>                                 f2fs_io_schedule_timeout(DEFAULT_IO_TIMEOUT);
>                                 goto retry_write;
>                         }
> -                       return ret;
> +                       goto out;
>                 }
>
>                 *submitted += _submitted;
>         }
>
> -       f2fs_balance_fs(F2FS_M_SB(mapping), true);
> +out:
> +       if (compr_blocks > 0)
> +               f2fs_unlock_op(sbi);
>
> -       return 0;
> +       f2fs_balance_fs(sbi, true);
> +       return ret;
>  }
>
>  int f2fs_write_multi_pages(struct compress_ctx *cc,
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 81f9e2cc49e2..b171a9980f6a 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2839,7 +2839,7 @@ int f2fs_write_single_data_page(struct page *page, int *submitted,
>                 .encrypted_page = NULL,
>                 .submitted = 0,
>                 .compr_blocks = compr_blocks,
> -               .need_lock = LOCK_RETRY,
> +               .need_lock = compr_blocks ? LOCK_DONE : LOCK_RETRY,
>                 .post_read = f2fs_post_read_required(inode) ? 1 : 0,
>                 .io_type = io_type,
>                 .io_wbc = wbc,
> @@ -2920,6 +2920,7 @@ int f2fs_write_single_data_page(struct page *page, int *submitted,
>         if (err == -EAGAIN) {
>                 err = f2fs_do_write_data_page(&fio);
>                 if (err == -EAGAIN) {
> +                       f2fs_bug_on(sbi, compr_blocks);
>                         fio.need_lock = LOCK_REQ;
>                         err = f2fs_do_write_data_page(&fio);
>                 }
> --
> 2.40.1
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Jaegeuk Kim Jan. 13, 2024, 1:39 a.m. UTC | #2 
Cleaned up a bit:

--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1443,13 +1443,14 @@ void f2fs_compress_write_end_io(struct bio *bio, struct page *page)
 }

 static int f2fs_write_raw_pages(struct compress_ctx *cc,
-                                       int *submitted,
+                                       int *submitted_p,
                                        struct writeback_control *wbc,
                                        enum iostat_type io_type)
 {
        struct address_space *mapping = cc->inode->i_mapping;
        struct f2fs_sb_info *sbi = F2FS_M_SB(mapping);
-       int _submitted, compr_blocks, ret = 0, i;
+       int submitted, compr_blocks, i;
+       int ret = 0;

        compr_blocks = f2fs_compressed_blocks(cc);

@@ -1492,7 +1493,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
                if (!clear_page_dirty_for_io(cc->rpages[i]))
                        goto continue_unlock;

-               ret = f2fs_write_single_data_page(cc->rpages[i], &_submitted,
+               ret = f2fs_write_single_data_page(cc->rpages[i], &submitted,
                                                NULL, NULL, wbc, io_type,
                                                compr_blocks, false);
                if (ret) {
@@ -1514,7 +1515,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
                        goto out;
                }

-               *submitted += _submitted;
+               *submitted_p += submitted;
        }

 out:

On 01/11, Chao Yu wrote:
> When we overwrite compressed cluster w/ normal cluster, we should
> not unlock cp_rwsem during f2fs_write_raw_pages(), otherwise data
> will be corrupted if partial blocks were persisted before CP & SPOR,
> due to cluster metadata wasn't updated atomically.
> 
> Fixes: 4c8ff7095bef ("f2fs: support data compression")
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/compress.c | 20 ++++++++++++++------
>  fs/f2fs/data.c     |  3 ++-
>  2 files changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 9940b7886e5d..bf4cfab67aec 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1448,7 +1448,8 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>  					enum iostat_type io_type)
>  {
>  	struct address_space *mapping = cc->inode->i_mapping;
> -	int _submitted, compr_blocks, ret, i;
> +	struct f2fs_sb_info *sbi = F2FS_M_SB(mapping);
> +	int _submitted, compr_blocks, ret = 0, i;
>  
>  	compr_blocks = f2fs_compressed_blocks(cc);
>  
> @@ -1463,6 +1464,10 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>  	if (compr_blocks < 0)
>  		return compr_blocks;
>  
> +	/* overwrite compressed cluster w/ normal cluster */
> +	if (compr_blocks > 0)
> +		f2fs_lock_op(sbi);
> +
>  	for (i = 0; i < cc->cluster_size; i++) {
>  		if (!cc->rpages[i])
>  			continue;
> @@ -1495,26 +1500,29 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>  				unlock_page(cc->rpages[i]);
>  				ret = 0;
>  			} else if (ret == -EAGAIN) {
> +				ret = 0;
>  				/*
>  				 * for quota file, just redirty left pages to
>  				 * avoid deadlock caused by cluster update race
>  				 * from foreground operation.
>  				 */
>  				if (IS_NOQUOTA(cc->inode))
> -					return 0;
> -				ret = 0;
> +					goto out;
>  				f2fs_io_schedule_timeout(DEFAULT_IO_TIMEOUT);
>  				goto retry_write;
>  			}
> -			return ret;
> +			goto out;
>  		}
>  
>  		*submitted += _submitted;
>  	}
>  
> -	f2fs_balance_fs(F2FS_M_SB(mapping), true);
> +out:
> +	if (compr_blocks > 0)
> +		f2fs_unlock_op(sbi);
>  
> -	return 0;
> +	f2fs_balance_fs(sbi, true);
> +	return ret;
>  }
>  
>  int f2fs_write_multi_pages(struct compress_ctx *cc,
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 81f9e2cc49e2..b171a9980f6a 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -2839,7 +2839,7 @@ int f2fs_write_single_data_page(struct page *page, int *submitted,
>  		.encrypted_page = NULL,
>  		.submitted = 0,
>  		.compr_blocks = compr_blocks,
> -		.need_lock = LOCK_RETRY,
> +		.need_lock = compr_blocks ? LOCK_DONE : LOCK_RETRY,
>  		.post_read = f2fs_post_read_required(inode) ? 1 : 0,
>  		.io_type = io_type,
>  		.io_wbc = wbc,
> @@ -2920,6 +2920,7 @@ int f2fs_write_single_data_page(struct page *page, int *submitted,
>  	if (err == -EAGAIN) {
>  		err = f2fs_do_write_data_page(&fio);
>  		if (err == -EAGAIN) {
> +			f2fs_bug_on(sbi, compr_blocks);
>  			fio.need_lock = LOCK_REQ;
>  			err = f2fs_do_write_data_page(&fio);
>  		}
> -- 
> 2.40.1
Chao Yu Jan. 13, 2024, 2:33 a.m. UTC | #3 
Thanks, let me resend v5 w/ blow cleanups.

On 2024/1/13 9:39, Jaegeuk Kim wrote:
> Cleaned up a bit:
> 
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1443,13 +1443,14 @@ void f2fs_compress_write_end_io(struct bio *bio, struct page *page)
>   }
> 
>   static int f2fs_write_raw_pages(struct compress_ctx *cc,
> -                                       int *submitted,
> +                                       int *submitted_p,
>                                          struct writeback_control *wbc,
>                                          enum iostat_type io_type)
>   {
>          struct address_space *mapping = cc->inode->i_mapping;
>          struct f2fs_sb_info *sbi = F2FS_M_SB(mapping);
> -       int _submitted, compr_blocks, ret = 0, i;
> +       int submitted, compr_blocks, i;
> +       int ret = 0;
> 
>          compr_blocks = f2fs_compressed_blocks(cc);
> 
> @@ -1492,7 +1493,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>                  if (!clear_page_dirty_for_io(cc->rpages[i]))
>                          goto continue_unlock;
> 
> -               ret = f2fs_write_single_data_page(cc->rpages[i], &_submitted,
> +               ret = f2fs_write_single_data_page(cc->rpages[i], &submitted,
>                                                  NULL, NULL, wbc, io_type,
>                                                  compr_blocks, false);
>                  if (ret) {
> @@ -1514,7 +1515,7 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>                          goto out;
>                  }
> 
> -               *submitted += _submitted;
> +               *submitted_p += submitted;
>          }
> 
>   out:
> 
> On 01/11, Chao Yu wrote:
>> When we overwrite compressed cluster w/ normal cluster, we should
>> not unlock cp_rwsem during f2fs_write_raw_pages(), otherwise data
>> will be corrupted if partial blocks were persisted before CP & SPOR,
>> due to cluster metadata wasn't updated atomically.
>>
>> Fixes: 4c8ff7095bef ("f2fs: support data compression")
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>>   fs/f2fs/compress.c | 20 ++++++++++++++------
>>   fs/f2fs/data.c     |  3 ++-
>>   2 files changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
>> index 9940b7886e5d..bf4cfab67aec 100644
>> --- a/fs/f2fs/compress.c
>> +++ b/fs/f2fs/compress.c
>> @@ -1448,7 +1448,8 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>>   					enum iostat_type io_type)
>>   {
>>   	struct address_space *mapping = cc->inode->i_mapping;
>> -	int _submitted, compr_blocks, ret, i;
>> +	struct f2fs_sb_info *sbi = F2FS_M_SB(mapping);
>> +	int _submitted, compr_blocks, ret = 0, i;
>>   
>>   	compr_blocks = f2fs_compressed_blocks(cc);
>>   
>> @@ -1463,6 +1464,10 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>>   	if (compr_blocks < 0)
>>   		return compr_blocks;
>>   
>> +	/* overwrite compressed cluster w/ normal cluster */
>> +	if (compr_blocks > 0)
>> +		f2fs_lock_op(sbi);
>> +
>>   	for (i = 0; i < cc->cluster_size; i++) {
>>   		if (!cc->rpages[i])
>>   			continue;
>> @@ -1495,26 +1500,29 @@ static int f2fs_write_raw_pages(struct compress_ctx *cc,
>>   				unlock_page(cc->rpages[i]);
>>   				ret = 0;
>>   			} else if (ret == -EAGAIN) {
>> +				ret = 0;
>>   				/*
>>   				 * for quota file, just redirty left pages to
>>   				 * avoid deadlock caused by cluster update race
>>   				 * from foreground operation.
>>   				 */
>>   				if (IS_NOQUOTA(cc->inode))
>> -					return 0;
>> -				ret = 0;
>> +					goto out;
>>   				f2fs_io_schedule_timeout(DEFAULT_IO_TIMEOUT);
>>   				goto retry_write;
>>   			}
>> -			return ret;
>> +			goto out;
>>   		}
>>   
>>   		*submitted += _submitted;
>>   	}
>>   
>> -	f2fs_balance_fs(F2FS_M_SB(mapping), true);
>> +out:
>> +	if (compr_blocks > 0)
>> +		f2fs_unlock_op(sbi);
>>   
>> -	return 0;
>> +	f2fs_balance_fs(sbi, true);
>> +	return ret;
>>   }
>>   
>>   int f2fs_write_multi_pages(struct compress_ctx *cc,
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 81f9e2cc49e2..b171a9980f6a 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -2839,7 +2839,7 @@ int f2fs_write_single_data_page(struct page *page, int *submitted,
>>   		.encrypted_page = NULL,
>>   		.submitted = 0,
>>   		.compr_blocks = compr_blocks,
>> -		.need_lock = LOCK_RETRY,
>> +		.need_lock = compr_blocks ? LOCK_DONE : LOCK_RETRY,
>>   		.post_read = f2fs_post_read_required(inode) ? 1 : 0,
>>   		.io_type = io_type,
>>   		.io_wbc = wbc,
>> @@ -2920,6 +2920,7 @@ int f2fs_write_single_data_page(struct page *page, int *submitted,
>>   	if (err == -EAGAIN) {
>>   		err = f2fs_do_write_data_page(&fio);
>>   		if (err == -EAGAIN) {
>> +			f2fs_bug_on(sbi, compr_blocks);
>>   			fio.need_lock = LOCK_REQ;
>>   			err = f2fs_do_write_data_page(&fio);
>>   		}
>> -- 
>> 2.40.1
diff mbox series

Patch

diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
index 9940b7886e5d..bf4cfab67aec 100644
--- a/fs/f2fs/compress.c
+++ b/fs/f2fs/compress.c
@@ -1448,7 +1448,8 @@  static int f2fs_write_raw_pages(struct compress_ctx *cc,
 					enum iostat_type io_type)
 {
 	struct address_space *mapping = cc->inode->i_mapping;
-	int _submitted, compr_blocks, ret, i;
+	struct f2fs_sb_info *sbi = F2FS_M_SB(mapping);
+	int _submitted, compr_blocks, ret = 0, i;
 
 	compr_blocks = f2fs_compressed_blocks(cc);
 
@@ -1463,6 +1464,10 @@  static int f2fs_write_raw_pages(struct compress_ctx *cc,
 	if (compr_blocks < 0)
 		return compr_blocks;
 
+	/* overwrite compressed cluster w/ normal cluster */
+	if (compr_blocks > 0)
+		f2fs_lock_op(sbi);
+
 	for (i = 0; i < cc->cluster_size; i++) {
 		if (!cc->rpages[i])
 			continue;
@@ -1495,26 +1500,29 @@  static int f2fs_write_raw_pages(struct compress_ctx *cc,
 				unlock_page(cc->rpages[i]);
 				ret = 0;
 			} else if (ret == -EAGAIN) {
+				ret = 0;
 				/*
 				 * for quota file, just redirty left pages to
 				 * avoid deadlock caused by cluster update race
 				 * from foreground operation.
 				 */
 				if (IS_NOQUOTA(cc->inode))
-					return 0;
-				ret = 0;
+					goto out;
 				f2fs_io_schedule_timeout(DEFAULT_IO_TIMEOUT);
 				goto retry_write;
 			}
-			return ret;
+			goto out;
 		}
 
 		*submitted += _submitted;
 	}
 
-	f2fs_balance_fs(F2FS_M_SB(mapping), true);
+out:
+	if (compr_blocks > 0)
+		f2fs_unlock_op(sbi);
 
-	return 0;
+	f2fs_balance_fs(sbi, true);
+	return ret;
 }
 
 int f2fs_write_multi_pages(struct compress_ctx *cc,
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 81f9e2cc49e2..b171a9980f6a 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -2839,7 +2839,7 @@  int f2fs_write_single_data_page(struct page *page, int *submitted,
 		.encrypted_page = NULL,
 		.submitted = 0,
 		.compr_blocks = compr_blocks,
-		.need_lock = LOCK_RETRY,
+		.need_lock = compr_blocks ? LOCK_DONE : LOCK_RETRY,
 		.post_read = f2fs_post_read_required(inode) ? 1 : 0,
 		.io_type = io_type,
 		.io_wbc = wbc,
@@ -2920,6 +2920,7 @@  int f2fs_write_single_data_page(struct page *page, int *submitted,
 	if (err == -EAGAIN) {
 		err = f2fs_do_write_data_page(&fio);
 		if (err == -EAGAIN) {
+			f2fs_bug_on(sbi, compr_blocks);
 			fio.need_lock = LOCK_REQ;
 			err = f2fs_do_write_data_page(&fio);
 		}