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 <jaegeuk.kim@xxxxxxxxxxx>
> ---
> 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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
