Re: [f2fs-dev] [PATCH v2] f2fs crypto: fix incorrect positioning for GCing encrypted data page

[Chao Yu <chao@kernel.org>]

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
>