Re: [PATCH v3] f2fs: cleanup the disk level filename updating

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 03/14, Kinglong Mee wrote:
> On 3/14/2017 02:23, Jaegeuk Kim wrote:
> > On 03/10, Kinglong Mee wrote:
> >> As discuss with Jaegeuk and Chao,
> >> "Once checkpoint is done, f2fs doesn't need to update there-in filename at all."
> >>
> >> The disk-level filename is used only one case,
> >> 1. create a file A under a dir
> >> 2. sync A
> >> 3. godown
> >> 4. umount
> >> 5. mount (roll_forward)
> >>
> >> Only the rename/cross_rename changes the filename, if it happens,
> >> a. between step 1 and 2, the sync A will caused checkpoint, so that,
> >>    the roll_forward at step 5 never happens.
> >> b. after step 2, the roll_forward happens, file A will roll forward
> >>    to the result as after step 1.
> > 
> > I've checked the roll-forward recovery again and found, if pino is recovered,
> > it tries to recover dentry with the written filename.
> > 
> > So,
> > 1. create a
> > 2. rename a b
> > 3. fsync b, will trigger checkpoint and recover pino
> > 4. fsync b
> > 5. godown
> > 
> > Then, roll-forward recovery will do recover_dentry with "a". So, correct pino
> > should have valid filename.
> 
> Is it the right operation? but the f2fs code doesn't do like that, see below.
> 
> > 
> > Thoughts?
> 
> If b isn't exist when renaming, f2fs creates a new directory entry
> (f2fs_add_link with name "b"), but no new inode or nid created.
> 
> The recover_dentry depends on FSYNC_BIT_SHIFT and DENT_BIT_SHIFT, as your procedures,
> a roll-forward recovery will do, but no recover_dentry happens.

Yeah, right. We already did checkpoint. ;)
So, it means correct pino allows us to do roll-forward recovery in fsync.
And, we lose it by link/rename and only recover it after checkpoint in fsync.

Thanks,

> 
> [ 3607.068713] do_read_inode: ino 3, name (0:0)
> [ 3607.090464] do_read_inode: ino 56398, name (0:1) b
> [ 3607.110743] F2FS-fs (sdb1): recover_inode: ino = dc4e, name = b
> [ 3607.111802] F2FS-fs (sdb1): recover_data: ino = dc4e (i_size: recover) recovered = 0, err = 0
> [ 3607.117374] F2FS-fs (sdb1): checkpoint: version = 31d5d90f
> [ 3607.118384] F2FS-fs (sdb1): Mounted with checkpoint version = 31d5d90f
> [ 3607.288552] NFSD: starting 90-second grace period (net ffffffffbeefd140)
> 
> After the first fsync at step 3, the IS_CHECKPOINTED is set, after that,
> IS_CHECKPOINTED will exist in the nat entry forever, so DENT_BIT_SHIFT never be set.
> 
> 1397 int fsync_node_pages(struct f2fs_sb_info *sbi, struct inode *inode,
> 1398                         struct writeback_control *wbc, bool atomic)
> 
> 1460                         if (!atomic || page == last_page) {
> 1461                                 set_fsync_mark(page, 1);
> 1462                                 if (IS_INODE(page)) {
> 1463                                         if (is_inode_flag_set(inode,
> 1464                                                                 FI_DIRTY_INODE))
> 1465                                                 update_inode(inode, page);
> 1466                                         set_dentry_mark(page,
> 1467                                                 need_dentry_mark(sbi, ino));
> 1468                                 }
> 1469                                 /*  may be written by other thread */
> 1470                                 if (!PageDirty(page))
> 1471                                         set_page_dirty(page);
> 1472                         }
> 
>  195 int need_dentry_mark(struct f2fs_sb_info *sbi, nid_t nid)
>  196 {
>  197         struct f2fs_nm_info *nm_i = NM_I(sbi);
>  198         struct nat_entry *e;
>  199         bool need = false;
>  200
>  201         down_read(&nm_i->nat_tree_lock);
>  202         e = __lookup_nat_cache(nm_i, nid);
>  203         if (e) {
>  204                 if (!get_nat_flag(e, IS_CHECKPOINTED) &&
>  205                                 !get_nat_flag(e, HAS_FSYNCED_INODE))
>  206                         need = true;
>  207         }
>  208         up_read(&nm_i->nat_tree_lock);
>  209         return need;
>  210 }
> 
> thanks,
> Kinglong Mee
> 
> > 
> > Thanks,
> > 
> >>
> >> So that, any updating the disk filename is useless, just cleanup it.
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >> ---
> >>  fs/f2fs/dir.c    | 25 ++++---------------------
> >>  fs/f2fs/f2fs.h   |  2 --
> >>  fs/f2fs/file.c   |  8 --------
> >>  fs/f2fs/inline.c |  2 --
> >>  fs/f2fs/namei.c  | 29 -----------------------------
> >>  5 files changed, 4 insertions(+), 62 deletions(-)
> >>
> >> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> >> index 8d5c62b..058c4f3 100644
> >> --- a/fs/f2fs/dir.c
> >> +++ b/fs/f2fs/dir.c
> >> @@ -337,24 +337,6 @@ static void init_dent_inode(const struct qstr *name, struct page *ipage)
> >>  	set_page_dirty(ipage);
> >>  }
> >>  
> >> -int update_dent_inode(struct inode *inode, struct inode *to,
> >> -					const struct qstr *name)
> >> -{
> >> -	struct page *page;
> >> -
> >> -	if (file_enc_name(to))
> >> -		return 0;
> >> -
> >> -	page = get_node_page(F2FS_I_SB(inode), inode->i_ino);
> >> -	if (IS_ERR(page))
> >> -		return PTR_ERR(page);
> >> -
> >> -	init_dent_inode(name, page);
> >> -	f2fs_put_page(page, 1);
> >> -
> >> -	return 0;
> >> -}
> >> -
> >>  void do_make_empty_dir(struct inode *inode, struct inode *parent,
> >>  					struct f2fs_dentry_ptr *d)
> >>  {
> >> @@ -438,8 +420,11 @@ struct page *init_inode_metadata(struct inode *inode, struct inode *dir,
> >>  		set_cold_node(inode, page);
> >>  	}
> >>  
> >> -	if (new_name)
> >> +	if (new_name) {
> >>  		init_dent_inode(new_name, page);
> >> +		if (f2fs_encrypted_inode(dir))
> >> +			file_set_enc_name(inode);
> >> +	}
> >>  
> >>  	/*
> >>  	 * This file should be checkpointed during fsync.
> >> @@ -599,8 +584,6 @@ int f2fs_add_regular_entry(struct inode *dir, const struct qstr *new_name,
> >>  			err = PTR_ERR(page);
> >>  			goto fail;
> >>  		}
> >> -		if (f2fs_encrypted_inode(dir))
> >> -			file_set_enc_name(inode);
> >>  	}
> >>  
> >>  	make_dentry_ptr(NULL, &d, (void *)dentry_blk, 1);
> >> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >> index 7edb3be..5dbc0c0 100644
> >> --- a/fs/f2fs/f2fs.h
> >> +++ b/fs/f2fs/f2fs.h
> >> @@ -2093,8 +2093,6 @@ ino_t f2fs_inode_by_name(struct inode *dir, const struct qstr *qstr,
> >>  			struct page **page);
> >>  void f2fs_set_link(struct inode *dir, struct f2fs_dir_entry *de,
> >>  			struct page *page, struct inode *inode);
> >> -int update_dent_inode(struct inode *inode, struct inode *to,
> >> -			const struct qstr *name);
> >>  void f2fs_update_dentry(nid_t ino, umode_t mode, struct f2fs_dentry_ptr *d,
> >>  			const struct qstr *name, f2fs_hash_t name_hash,
> >>  			unsigned int bit_pos);
> >> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >> index 4ec764e..8c7923f 100644
> >> --- a/fs/f2fs/file.c
> >> +++ b/fs/f2fs/file.c
> >> @@ -110,20 +110,12 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
> >>  {
> >>  	struct dentry *dentry;
> >>  
> >> -	if (file_enc_name(inode))
> >> -		return 0;
> >> -
> >>  	inode = igrab(inode);
> >>  	dentry = d_find_any_alias(inode);
> >>  	iput(inode);
> >>  	if (!dentry)
> >>  		return 0;
> >>  
> >> -	if (update_dent_inode(inode, inode, &dentry->d_name)) {
> >> -		dput(dentry);
> >> -		return 0;
> >> -	}
> >> -
> >>  	*pino = parent_ino(dentry);
> >>  	dput(dentry);
> >>  	return 1;
> >> diff --git a/fs/f2fs/inline.c b/fs/f2fs/inline.c
> >> index e32a9e5..41fe27d 100644
> >> --- a/fs/f2fs/inline.c
> >> +++ b/fs/f2fs/inline.c
> >> @@ -527,8 +527,6 @@ int f2fs_add_inline_entry(struct inode *dir, const struct qstr *new_name,
> >>  			err = PTR_ERR(page);
> >>  			goto fail;
> >>  		}
> >> -		if (f2fs_encrypted_inode(dir))
> >> -			file_set_enc_name(inode);
> >>  	}
> >>  
> >>  	f2fs_wait_on_page_writeback(ipage, NODE, true);
> >> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> >> index 25c073f..8906c9f 100644
> >> --- a/fs/f2fs/namei.c
> >> +++ b/fs/f2fs/namei.c
> >> @@ -720,13 +720,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>  		if (err)
> >>  			goto put_out_dir;
> >>  
> >> -		err = update_dent_inode(old_inode, new_inode,
> >> -						&new_dentry->d_name);
> >> -		if (err) {
> >> -			release_orphan_inode(sbi);
> >> -			goto put_out_dir;
> >> -		}
> >> -
> >>  		f2fs_set_link(new_dir, new_entry, new_page, old_inode);
> >>  
> >>  		new_inode->i_ctime = current_time(new_inode);
> >> @@ -779,8 +772,6 @@ static int f2fs_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>  
> >>  	down_write(&F2FS_I(old_inode)->i_sem);
> >>  	file_lost_pino(old_inode);
> >> -	if (new_inode && file_enc_name(new_inode))
> >> -		file_set_enc_name(old_inode);
> >>  	up_write(&F2FS_I(old_inode)->i_sem);
> >>  
> >>  	old_inode->i_ctime = current_time(old_inode);
> >> @@ -917,18 +908,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>  
> >>  	f2fs_lock_op(sbi);
> >>  
> >> -	err = update_dent_inode(old_inode, new_inode, &new_dentry->d_name);
> >> -	if (err)
> >> -		goto out_unlock;
> >> -	if (file_enc_name(new_inode))
> >> -		file_set_enc_name(old_inode);
> >> -
> >> -	err = update_dent_inode(new_inode, old_inode, &old_dentry->d_name);
> >> -	if (err)
> >> -		goto out_undo;
> >> -	if (file_enc_name(old_inode))
> >> -		file_set_enc_name(new_inode);
> >> -
> >>  	/* update ".." directory entry info of old dentry */
> >>  	if (old_dir_entry)
> >>  		f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
> >> @@ -972,14 +951,6 @@ static int f2fs_cross_rename(struct inode *old_dir, struct dentry *old_dentry,
> >>  	if (IS_DIRSYNC(old_dir) || IS_DIRSYNC(new_dir))
> >>  		f2fs_sync_fs(sbi->sb, 1);
> >>  	return 0;
> >> -out_undo:
> >> -	/*
> >> -	 * Still we may fail to recover name info of f2fs_inode here
> >> -	 * Drop it, once its name is set as encrypted
> >> -	 */
> >> -	update_dent_inode(old_inode, old_inode, &old_dentry->d_name);
> >> -out_unlock:
> >> -	f2fs_unlock_op(sbi);
> >>  out_new_dir:
> >>  	if (new_dir_entry) {
> >>  		f2fs_dentry_kunmap(new_inode, new_dir_page);
> >> -- 
> >> 2.9.3
> > 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot