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

[Chao Yu]

On 2016/2/22 18:38, Chao Yu wrote:
> For now, flow of GCing an encrypted data page will be:
> 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@xxxxxxxxxxx>
> ---
>  fs/f2fs/gc.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 4af3d80..428159b 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,13 @@ 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);
> +	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)
> -		goto put_out;
> +		goto invalid_block;
>  
>  	err = f2fs_submit_page_bio(&fio);
>  	if (err)
> @@ -607,9 +609,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 +622,8 @@ 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);
> +invalid_block:
> +	invalidate_blocks(fio.sbi, newaddr);

Need to recover old blkaddr here rather than just invalidate new blkaddr. I will
fix this and resend the patch tomorrow. Sorry for the noise.

Thanks,

>  put_out:
>  	f2fs_put_dnode(&dn);
>  out:
>