linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] f2fs crypto: fix incorrect positioning for GCing encrypted data page
@ 2016-02-23  9:52 Chao Yu
  2016-02-26 19:50 ` Jaegeuk Kim
  0 siblings, 1 reply; 3+ messages in thread
From: Chao Yu @ 2016-02-23  9:52 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel

For now, flow of GCing an encrypted data page:
1) try to grab meta page in meta inode's mapping with index of old block
address of that data page
2) load data of ciphertext into meta page
3) allocate new block address
4) write the meta page into new block address
5) update block address pointer in direct node page.

Other reader/writer will use f2fs_wait_on_encrypted_page_writeback to
check and wait on GCed encrypted data cached in meta page writebacked
in order to avoid inconsistence among data page cache, meta page cache
and data on-disk when updating.

However, we will use new block address updated in step 5) as an index to
lookup meta page in inner bio buffer. That would be wrong, and we will
never find the GCing meta page, since we use the old block address as
index of that page in step 1).

This patch fixes the issue by adjust the order of step 1) and step 3),
and in step 1) grab page with index generated in step 3).

Signed-off-by: Chao Yu <chao2.yu@samsung.com>
---
 v2:
 - fix to recover data page to old block address when error occurs.

 fs/f2fs/f2fs.h    |  2 ++
 fs/f2fs/gc.c      | 25 +++++++++++++++++--------
 fs/f2fs/segment.c |  3 +--
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index b57dee6..a7197b3 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1821,6 +1821,8 @@ void write_meta_page(struct f2fs_sb_info *, struct page *);
 void write_node_page(unsigned int, struct f2fs_io_info *);
 void write_data_page(struct dnode_of_data *, struct f2fs_io_info *);
 void rewrite_data_page(struct f2fs_io_info *);
+void __f2fs_replace_block(struct f2fs_sb_info *, struct f2fs_summary *,
+					block_t, block_t, bool, bool);
 void f2fs_replace_block(struct f2fs_sb_info *, struct dnode_of_data *,
 				block_t, block_t, unsigned char, bool, bool);
 void allocate_data_block(struct f2fs_sb_info *, struct page *,
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 4af3d80..c7cb1e6 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -549,6 +549,7 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
 	struct f2fs_summary sum;
 	struct node_info ni;
 	struct page *page;
+	block_t newaddr;
 	int err;
 
 	/* do not read out */
@@ -579,12 +580,15 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
 	fio.page = page;
 	fio.new_blkaddr = fio.old_blkaddr = dn.data_blkaddr;
 
-	fio.encrypted_page = pagecache_get_page(META_MAPPING(fio.sbi),
-					fio.new_blkaddr,
-					FGP_LOCK|FGP_CREAT,
-					GFP_NOFS);
-	if (!fio.encrypted_page)
-		goto put_out;
+	allocate_data_block(fio.sbi, NULL, fio.old_blkaddr, &newaddr,
+							&sum, CURSEG_COLD_DATA);
+
+	fio.encrypted_page = pagecache_get_page(META_MAPPING(fio.sbi), newaddr,
+					FGP_LOCK | FGP_CREAT, GFP_NOFS);
+	if (!fio.encrypted_page) {
+		err = -ENOMEM;
+		goto recover_block;
+	}
 
 	err = f2fs_submit_page_bio(&fio);
 	if (err)
@@ -593,6 +597,7 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
 	/* write page */
 	lock_page(fio.encrypted_page);
 
+	err = -EIO;
 	if (unlikely(!PageUptodate(fio.encrypted_page)))
 		goto put_page_out;
 	if (unlikely(fio.encrypted_page->mapping != META_MAPPING(fio.sbi)))
@@ -607,9 +612,9 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
 
 	/* allocate block address */
 	f2fs_wait_on_page_writeback(dn.node_page, NODE, true);
-	allocate_data_block(fio.sbi, NULL, fio.old_blkaddr, &fio.new_blkaddr,
-							&sum, CURSEG_COLD_DATA);
+
 	fio.rw = WRITE_SYNC;
+	fio.new_blkaddr = newaddr;
 	f2fs_submit_page_mbio(&fio);
 
 	dn.data_blkaddr = fio.new_blkaddr;
@@ -620,6 +625,10 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
 		set_inode_flag(F2FS_I(inode), FI_FIRST_BLOCK_WRITTEN);
 put_page_out:
 	f2fs_put_page(fio.encrypted_page, 1);
+recover_block:
+	if (err)
+		__f2fs_replace_block(fio.sbi, &sum, newaddr, fio.old_blkaddr,
+								true, true);
 put_out:
 	f2fs_put_dnode(&dn);
 out:
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index d8e13b0..639c4ff 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1439,8 +1439,7 @@ void rewrite_data_page(struct f2fs_io_info *fio)
 	f2fs_submit_page_mbio(fio);
 }
 
-static void __f2fs_replace_block(struct f2fs_sb_info *sbi,
-				struct f2fs_summary *sum,
+void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
 				block_t old_blkaddr, block_t new_blkaddr,
 				bool recover_curseg, bool recover_newaddr)
 {
-- 
2.7.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] f2fs crypto: fix incorrect positioning for GCing encrypted data page
  2016-02-23  9:52 [PATCH v2] f2fs crypto: fix incorrect positioning for GCing encrypted data page Chao Yu
@ 2016-02-26 19:50 ` Jaegeuk Kim
  2016-02-27  1:01   ` [f2fs-dev] " Chao Yu
  0 siblings, 1 reply; 3+ messages in thread
From: Jaegeuk Kim @ 2016-02-26 19:50 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel, linux-kernel

Hi Chao,

On Tue, Feb 23, 2016 at 05:52:43PM +0800, Chao Yu wrote:
> For now, flow of GCing an encrypted data page:
> 1) try to grab meta page in meta inode's mapping with index of old block
> address of that data page
> 2) load data of ciphertext into meta page
> 3) allocate new block address
> 4) write the meta page into new block address
> 5) update block address pointer in direct node page.
> 
> Other reader/writer will use f2fs_wait_on_encrypted_page_writeback to
> check and wait on GCed encrypted data cached in meta page writebacked
> in order to avoid inconsistence among data page cache, meta page cache
> and data on-disk when updating.
> 
> However, we will use new block address updated in step 5) as an index to
> lookup meta page in inner bio buffer. That would be wrong, and we will
> never find the GCing meta page, since we use the old block address as
> index of that page in step 1).
> 
> This patch fixes the issue by adjust the order of step 1) and step 3),
> and in step 1) grab page with index generated in step 3).
> 
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> ---
>  v2:
>  - fix to recover data page to old block address when error occurs.
> 
>  fs/f2fs/f2fs.h    |  2 ++
>  fs/f2fs/gc.c      | 25 +++++++++++++++++--------
>  fs/f2fs/segment.c |  3 +--
>  3 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index b57dee6..a7197b3 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1821,6 +1821,8 @@ void write_meta_page(struct f2fs_sb_info *, struct page *);
>  void write_node_page(unsigned int, struct f2fs_io_info *);
>  void write_data_page(struct dnode_of_data *, struct f2fs_io_info *);
>  void rewrite_data_page(struct f2fs_io_info *);
> +void __f2fs_replace_block(struct f2fs_sb_info *, struct f2fs_summary *,
> +					block_t, block_t, bool, bool);
>  void f2fs_replace_block(struct f2fs_sb_info *, struct dnode_of_data *,
>  				block_t, block_t, unsigned char, bool, bool);
>  void allocate_data_block(struct f2fs_sb_info *, struct page *,
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 4af3d80..c7cb1e6 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -549,6 +549,7 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>  	struct f2fs_summary sum;
>  	struct node_info ni;
>  	struct page *page;
> +	block_t newaddr;
>  	int err;
>  
>  	/* do not read out */
> @@ -579,12 +580,15 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>  	fio.page = page;
>  	fio.new_blkaddr = fio.old_blkaddr = dn.data_blkaddr;
>  
> -	fio.encrypted_page = pagecache_get_page(META_MAPPING(fio.sbi),
> -					fio.new_blkaddr,
> -					FGP_LOCK|FGP_CREAT,
> -					GFP_NOFS);
> -	if (!fio.encrypted_page)
> -		goto put_out;
> +	allocate_data_block(fio.sbi, NULL, fio.old_blkaddr, &newaddr,
> +							&sum, CURSEG_COLD_DATA);
> +
> +	fio.encrypted_page = pagecache_get_page(META_MAPPING(fio.sbi), newaddr,
> +					FGP_LOCK | FGP_CREAT, GFP_NOFS);
> +	if (!fio.encrypted_page) {
> +		err = -ENOMEM;
> +		goto recover_block;
> +	}
>  
>  	err = f2fs_submit_page_bio(&fio);
>  	if (err)
> @@ -593,6 +597,7 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>  	/* write page */
>  	lock_page(fio.encrypted_page);
>  
> +	err = -EIO;

I found out that this will trigger error all the time, even in the successful
case.
I'll move assigning this to the below error cases only.

Thanks,


>  	if (unlikely(!PageUptodate(fio.encrypted_page)))
>  		goto put_page_out;
>  	if (unlikely(fio.encrypted_page->mapping != META_MAPPING(fio.sbi)))
> @@ -607,9 +612,9 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>  
>  	/* allocate block address */
>  	f2fs_wait_on_page_writeback(dn.node_page, NODE, true);
> -	allocate_data_block(fio.sbi, NULL, fio.old_blkaddr, &fio.new_blkaddr,
> -							&sum, CURSEG_COLD_DATA);
> +
>  	fio.rw = WRITE_SYNC;
> +	fio.new_blkaddr = newaddr;
>  	f2fs_submit_page_mbio(&fio);
>  
>  	dn.data_blkaddr = fio.new_blkaddr;
> @@ -620,6 +625,10 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>  		set_inode_flag(F2FS_I(inode), FI_FIRST_BLOCK_WRITTEN);
>  put_page_out:
>  	f2fs_put_page(fio.encrypted_page, 1);
> +recover_block:
> +	if (err)
> +		__f2fs_replace_block(fio.sbi, &sum, newaddr, fio.old_blkaddr,
> +								true, true);
>  put_out:
>  	f2fs_put_dnode(&dn);
>  out:
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index d8e13b0..639c4ff 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1439,8 +1439,7 @@ void rewrite_data_page(struct f2fs_io_info *fio)
>  	f2fs_submit_page_mbio(fio);
>  }
>  
> -static void __f2fs_replace_block(struct f2fs_sb_info *sbi,
> -				struct f2fs_summary *sum,
> +void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>  				block_t old_blkaddr, block_t new_blkaddr,
>  				bool recover_curseg, bool recover_newaddr)
>  {
> -- 
> 2.7.0
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs crypto: fix incorrect positioning for GCing encrypted data page
  2016-02-26 19:50 ` Jaegeuk Kim
@ 2016-02-27  1:01   ` Chao Yu
  0 siblings, 0 replies; 3+ messages in thread
From: Chao Yu @ 2016-02-27  1:01 UTC (permalink / raw)
  To: Jaegeuk Kim, Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

Hi Jaegeuk,

On 2016/2/27 3:50, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On Tue, Feb 23, 2016 at 05:52:43PM +0800, Chao Yu wrote:
>> For now, flow of GCing an encrypted data page:
>> 1) try to grab meta page in meta inode's mapping with index of old block
>> address of that data page
>> 2) load data of ciphertext into meta page
>> 3) allocate new block address
>> 4) write the meta page into new block address
>> 5) update block address pointer in direct node page.
>>
>> Other reader/writer will use f2fs_wait_on_encrypted_page_writeback to
>> check and wait on GCed encrypted data cached in meta page writebacked
>> in order to avoid inconsistence among data page cache, meta page cache
>> and data on-disk when updating.
>>
>> However, we will use new block address updated in step 5) as an index to
>> lookup meta page in inner bio buffer. That would be wrong, and we will
>> never find the GCing meta page, since we use the old block address as
>> index of that page in step 1).
>>
>> This patch fixes the issue by adjust the order of step 1) and step 3),
>> and in step 1) grab page with index generated in step 3).
>>
>> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
>> ---
>>  v2:
>>  - fix to recover data page to old block address when error occurs.
>>
>>  fs/f2fs/f2fs.h    |  2 ++
>>  fs/f2fs/gc.c      | 25 +++++++++++++++++--------
>>  fs/f2fs/segment.c |  3 +--
>>  3 files changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index b57dee6..a7197b3 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -1821,6 +1821,8 @@ void write_meta_page(struct f2fs_sb_info *, struct page *);
>>  void write_node_page(unsigned int, struct f2fs_io_info *);
>>  void write_data_page(struct dnode_of_data *, struct f2fs_io_info *);
>>  void rewrite_data_page(struct f2fs_io_info *);
>> +void __f2fs_replace_block(struct f2fs_sb_info *, struct f2fs_summary *,
>> +					block_t, block_t, bool, bool);
>>  void f2fs_replace_block(struct f2fs_sb_info *, struct dnode_of_data *,
>>  				block_t, block_t, unsigned char, bool, bool);
>>  void allocate_data_block(struct f2fs_sb_info *, struct page *,
>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>> index 4af3d80..c7cb1e6 100644
>> --- a/fs/f2fs/gc.c
>> +++ b/fs/f2fs/gc.c
>> @@ -549,6 +549,7 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>>  	struct f2fs_summary sum;
>>  	struct node_info ni;
>>  	struct page *page;
>> +	block_t newaddr;
>>  	int err;
>>  
>>  	/* do not read out */
>> @@ -579,12 +580,15 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>>  	fio.page = page;
>>  	fio.new_blkaddr = fio.old_blkaddr = dn.data_blkaddr;
>>  
>> -	fio.encrypted_page = pagecache_get_page(META_MAPPING(fio.sbi),
>> -					fio.new_blkaddr,
>> -					FGP_LOCK|FGP_CREAT,
>> -					GFP_NOFS);
>> -	if (!fio.encrypted_page)
>> -		goto put_out;
>> +	allocate_data_block(fio.sbi, NULL, fio.old_blkaddr, &newaddr,
>> +							&sum, CURSEG_COLD_DATA);
>> +
>> +	fio.encrypted_page = pagecache_get_page(META_MAPPING(fio.sbi), newaddr,
>> +					FGP_LOCK | FGP_CREAT, GFP_NOFS);
>> +	if (!fio.encrypted_page) {
>> +		err = -ENOMEM;
>> +		goto recover_block;
>> +	}
>>  
>>  	err = f2fs_submit_page_bio(&fio);
>>  	if (err)
>> @@ -593,6 +597,7 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>>  	/* write page */
>>  	lock_page(fio.encrypted_page);
>>  
>> +	err = -EIO;
> 
> I found out that this will trigger error all the time, even in the successful
> case.
> I'll move assigning this to the below error cases only.

My bad, thanks for fixing it. :)

Thanks,

> 
> Thanks,
> 
> 
>>  	if (unlikely(!PageUptodate(fio.encrypted_page)))
>>  		goto put_page_out;
>>  	if (unlikely(fio.encrypted_page->mapping != META_MAPPING(fio.sbi)))
>> @@ -607,9 +612,9 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>>  
>>  	/* allocate block address */
>>  	f2fs_wait_on_page_writeback(dn.node_page, NODE, true);
>> -	allocate_data_block(fio.sbi, NULL, fio.old_blkaddr, &fio.new_blkaddr,
>> -							&sum, CURSEG_COLD_DATA);
>> +
>>  	fio.rw = WRITE_SYNC;
>> +	fio.new_blkaddr = newaddr;
>>  	f2fs_submit_page_mbio(&fio);
>>  
>>  	dn.data_blkaddr = fio.new_blkaddr;
>> @@ -620,6 +625,10 @@ static void move_encrypted_block(struct inode *inode, block_t bidx)
>>  		set_inode_flag(F2FS_I(inode), FI_FIRST_BLOCK_WRITTEN);
>>  put_page_out:
>>  	f2fs_put_page(fio.encrypted_page, 1);
>> +recover_block:
>> +	if (err)
>> +		__f2fs_replace_block(fio.sbi, &sum, newaddr, fio.old_blkaddr,
>> +								true, true);
>>  put_out:
>>  	f2fs_put_dnode(&dn);
>>  out:
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index d8e13b0..639c4ff 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -1439,8 +1439,7 @@ void rewrite_data_page(struct f2fs_io_info *fio)
>>  	f2fs_submit_page_mbio(fio);
>>  }
>>  
>> -static void __f2fs_replace_block(struct f2fs_sb_info *sbi,
>> -				struct f2fs_summary *sum,
>> +void __f2fs_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
>>  				block_t old_blkaddr, block_t new_blkaddr,
>>  				bool recover_curseg, bool recover_newaddr)
>>  {
>> -- 
>> 2.7.0
>>
> 
> ------------------------------------------------------------------------------
> Site24x7 APM Insight: Get Deep Visibility into Application Performance
> APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
> Monitor end-to-end web transactions and take corrective actions now
> Troubleshoot faster and improve end-user experience. Signup Now!
> http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-02-27  1:01 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-23  9:52 [PATCH v2] f2fs crypto: fix incorrect positioning for GCing encrypted data page Chao Yu
2016-02-26 19:50 ` Jaegeuk Kim
2016-02-27  1:01   ` [f2fs-dev] " Chao Yu

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).