Hi, > -----Original Message----- > From: Jaegeuk Kim [mailto:jaegeuk@xxxxxxxxxx] > Sent: Monday, June 23, 2014 9:13 AM > To: linux-kernel@xxxxxxxxxxxxxxx; linux-fsdevel@xxxxxxxxxxxxxxx; > linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx > Cc: Jaegeuk Kim; Chao Yu > Subject: [PATCH 1/2] f2fs: introduce f2fs_do_tmpfile for code consistency > > This patch adds f2fs_do_tmpfile to eliminate the redundant init_inode_metadata > flow. > Throught this, we can provide the consistent lock usage, e.g., fi->i_sem, and > this will enable better debugging stuffs. > > Cc: Chao Yu <chao2.yu@xxxxxxxxxxx> > Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx> > --- > fs/f2fs/dir.c | 27 ++++++++++++++++++++++++--- > fs/f2fs/f2fs.h | 1 + > fs/f2fs/namei.c | 30 ++++++++---------------------- > 3 files changed, 33 insertions(+), 25 deletions(-) > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index 09760d5..fe6aaca 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -337,8 +337,7 @@ static struct page *init_inode_metadata(struct inode *inode, > struct page *page; > int err; > > - if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE) && > - inode->i_nlink) { > + if (is_inode_flag_set(F2FS_I(inode), FI_NEW_INODE)) { > page = new_inode_page(inode, name); > if (IS_ERR(page)) > return page; > @@ -364,7 +363,8 @@ static struct page *init_inode_metadata(struct inode *inode, > set_cold_node(inode, page); > } > > - init_dent_inode(name, page); > + if (name) > + init_dent_inode(name, page); > > /* > * This file should be checkpointed during fsync. > @@ -537,6 +537,27 @@ fail: > return err; > } > > +int f2fs_do_tmpfile(struct inode *inode, struct inode *dir) > +{ > + struct page *page; > + int err = 0; > + > + down_write(&F2FS_I(inode)->i_sem); Here I did not find nlink/pino/xattr_ver which we should protect by ->i_sem, Am I missing something? > + page = init_inode_metadata(inode, dir, NULL); > + if (IS_ERR(page)) { > + err = PTR_ERR(page); > + goto fail; > + } > + /* we don't need to mark_inode_dirty now */ > + update_inode(inode, page); > + f2fs_put_page(page, 1); > + > + update_parent_metadata(dir, inode, F2FS_I(dir)->i_current_depth); Things we did in update_parent_metadata() is: 1.clear FI_NEW_INODE of inode flag, 2. update mtime&ctime of dir and mark it dirty. IMO, we do not need the update op because we do not access/modify current dir as we did normally through f2fs_add_link path. So how about using clear_inode_flag instead of update_parent_metadata and leave current dir as clean? Thanks, Yu > +fail: > + up_write(&F2FS_I(inode)->i_sem); > + return err; > +} > + > /* > * It only removes the dentry from the dentry page,corresponding name > * entry in name page does not need to be touched during deletion. > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 58df97e..ca421a8 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1136,6 +1136,7 @@ void f2fs_set_link(struct inode *, struct f2fs_dir_entry *, > int update_dent_inode(struct inode *, const struct qstr *); > int __f2fs_add_link(struct inode *, const struct qstr *, struct inode *); > void f2fs_delete_entry(struct f2fs_dir_entry *, struct page *, struct inode *); > +int f2fs_do_tmpfile(struct inode *, struct inode *); > int f2fs_make_empty(struct inode *, struct inode *); > bool f2fs_empty_dir(struct inode *); > > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > index 0d55517..9de4ab3 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -494,7 +494,6 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t > mode) > struct super_block *sb = dir->i_sb; > struct f2fs_sb_info *sbi = F2FS_SB(sb); > struct inode *inode; > - struct page *page; > int err; > > inode = f2fs_new_inode(dir, mode); > @@ -509,38 +508,25 @@ static int f2fs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t > mode) > err = acquire_orphan_inode(sbi); > if (err) > goto out; > + > + err = f2fs_do_tmpfile(inode, dir); > + if (err) > + goto release_out; > + > /* > * add this non-linked tmpfile to orphan list, in this way we could > * remove all unused data of tmpfile after abnormal power-off. > */ > add_orphan_inode(sbi, inode->i_ino); > - > - page = new_inode_page(inode, NULL); > - if (IS_ERR(page)) { > - err = PTR_ERR(page); > - goto remove_out; > - } > - > - err = f2fs_init_acl(inode, dir, page); > - if (err) > - goto unlock_out; > - > - err = f2fs_init_security(inode, dir, NULL, page); > - if (err) > - goto unlock_out; > - > - f2fs_put_page(page, 1); > f2fs_unlock_op(sbi); > > alloc_nid_done(sbi, inode->i_ino); > - mark_inode_dirty(inode); > d_tmpfile(dentry, inode); > unlock_new_inode(inode); > return 0; > -unlock_out: > - f2fs_put_page(page, 1); > -remove_out: > - remove_orphan_inode(sbi, inode->i_ino); > + > +release_out: > + release_orphan_inode(sbi); > out: > f2fs_unlock_op(sbi); > clear_nlink(inode); > -- > 1.8.5.2 (Apple Git-48) -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
