On 08/01/2013 03:58 PM, Jaegeuk Kim wrote: > This patch fixes mishandling of the sbi->n_orphans variable. > > If users request lots of f2fs_unlink(), check_orphan_space() could be > contended. > In such the case, sbi->n_orphans can be read incorrectly so that f2fs_unlink() > would fall into the wrong state which results in the failure of > add_orphan_inode(). > > So, let's increment sbi->n_orphans virtually prior to the actual orphan inode > stuffs. After that, let's release sbi->n_orphans by calling > release_orphan_inode > or remove_orphan_inode.
Hi Kim, The key point is that we did not reduce sbi->n_orphans when we release/remove orphan inode, so just adding the reduction step can fix this issue. But why moving the increment of sbi->n_orphans before we add orphan inode? It seems that we can not get benefit from it, and it makes the procedure a bit complex, because we should reduce the sbi->n_orphans in some fail pathes before we really add orphan inode. Thanks, Gu > > Signed-off-by: Jaegeuk Kim <[email protected]> > --- > fs/f2fs/checkpoint.c | 13 ++++++++++--- > fs/f2fs/dir.c | 2 ++ > fs/f2fs/f2fs.h | 3 ++- > fs/f2fs/namei.c | 19 ++++++++++++++----- > 4 files changed, 28 insertions(+), 9 deletions(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index fe91773..c5a5c39 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -182,7 +182,7 @@ const struct address_space_operations f2fs_meta_aops = { > .set_page_dirty = f2fs_set_meta_page_dirty, > }; > > -int check_orphan_space(struct f2fs_sb_info *sbi) > +int acquire_orphan_inode(struct f2fs_sb_info *sbi) > { > unsigned int max_orphans; > int err = 0; > @@ -197,10 +197,19 @@ int check_orphan_space(struct f2fs_sb_info *sbi) > mutex_lock(&sbi->orphan_inode_mutex); > if (sbi->n_orphans >= max_orphans) > err = -ENOSPC; > + else > + sbi->n_orphans++; > mutex_unlock(&sbi->orphan_inode_mutex); > return err; > } > > +void release_orphan_inode(struct f2fs_sb_info *sbi) > +{ > + mutex_lock(&sbi->orphan_inode_mutex); > + sbi->n_orphans--; > + mutex_unlock(&sbi->orphan_inode_mutex); > +} > + > void add_orphan_inode(struct f2fs_sb_info *sbi, nid_t ino) > { > struct list_head *head, *this; > @@ -229,8 +238,6 @@ retry: > list_add(&new->list, this->prev); > else > list_add_tail(&new->list, head); > - > - sbi->n_orphans++; > out: > mutex_unlock(&sbi->orphan_inode_mutex); > } > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c > index d1bb260..384c6da 100644 > --- a/fs/f2fs/dir.c > +++ b/fs/f2fs/dir.c > @@ -572,6 +572,8 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, > struct page *page, > > if (inode->i_nlink == 0) > add_orphan_inode(sbi, inode->i_ino); > + else > + release_orphan_inode(sbi); > } > > if (bit_pos == NR_DENTRY_IN_BLOCK) { > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index a6858c7..78777cd 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -1044,7 +1044,8 @@ void destroy_segment_manager(struct f2fs_sb_info *); > struct page *grab_meta_page(struct f2fs_sb_info *, pgoff_t); > struct page *get_meta_page(struct f2fs_sb_info *, pgoff_t); > long sync_meta_pages(struct f2fs_sb_info *, enum page_type, long); > -int check_orphan_space(struct f2fs_sb_info *); > +int acquire_orphan_inode(struct f2fs_sb_info *); > +void release_orphan_inode(struct f2fs_sb_info *); > void add_orphan_inode(struct f2fs_sb_info *, nid_t); > void remove_orphan_inode(struct f2fs_sb_info *, nid_t); > int recover_orphan_inodes(struct f2fs_sb_info *); > diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c > index 3297278..4e47518 100644 > --- a/fs/f2fs/namei.c > +++ b/fs/f2fs/namei.c > @@ -239,7 +239,7 @@ static int f2fs_unlink(struct inode *dir, struct dentry > *dentry) > if (!de) > goto fail; > > - err = check_orphan_space(sbi); > + err = acquire_orphan_inode(sbi); > if (err) { > kunmap(page); > f2fs_put_page(page, 0); > @@ -393,7 +393,7 @@ static int f2fs_rename(struct inode *old_dir, struct > dentry *old_dentry, > struct inode *old_inode = old_dentry->d_inode; > struct inode *new_inode = new_dentry->d_inode; > struct page *old_dir_page; > - struct page *old_page; > + struct page *old_page, *new_page; > struct f2fs_dir_entry *old_dir_entry = NULL; > struct f2fs_dir_entry *old_entry; > struct f2fs_dir_entry *new_entry; > @@ -415,7 +415,6 @@ static int f2fs_rename(struct inode *old_dir, struct > dentry *old_dentry, > ilock = mutex_lock_op(sbi); > > if (new_inode) { > - struct page *new_page; > > err = -ENOTEMPTY; > if (old_dir_entry && !f2fs_empty_dir(new_inode)) > @@ -427,9 +426,13 @@ static int f2fs_rename(struct inode *old_dir, struct > dentry *old_dentry, > if (!new_entry) > goto out_dir; > > + err = acquire_orphan_inode(sbi); > + if (err) > + goto put_out_dir; > + > if (update_dent_inode(old_inode, &new_dentry->d_name)) { > - f2fs_put_page(new_page, 1); > - goto out_dir; > + release_orphan_inode(sbi); > + goto put_out_dir; > } > > f2fs_set_link(new_dir, new_entry, new_page, old_inode); > @@ -438,8 +441,12 @@ static int f2fs_rename(struct inode *old_dir, struct > dentry *old_dentry, > if (old_dir_entry) > drop_nlink(new_inode); > drop_nlink(new_inode); > + > if (!new_inode->i_nlink) > add_orphan_inode(sbi, new_inode->i_ino); > + else > + release_orphan_inode(sbi); > + > update_inode_page(new_inode); > } else { > err = f2fs_add_link(new_dentry, old_inode); > @@ -472,6 +479,8 @@ static int f2fs_rename(struct inode *old_dir, struct > dentry *old_dentry, > mutex_unlock_op(sbi, ilock); > return 0; > > +put_out_dir: > + f2fs_put_page(new_page, 1); > out_dir: > if (old_dir_entry) { > kunmap(old_dir_page); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
