Hi Jaegeuk,
This patch seems to have introduced a bug in the handling of sbi->n_orphans
during recovery. Recovery can cause orphans to be added to the list
without incrementing n_orphans via:
- recover_fsync_data
- find_fsync_dnodes
- recover_inode
- recover_dentry
- f2fs_delete_entry
- add_orphan_inode
Later, these orphans are removed via:
- recover_fsync_data
- recover_data
- do_recover_data
- check_index_in_prev_nodes
- truncate_hole
- evict
- f2fs_evict_inode
- remove_inode_page
- truncate_node
- remove_orphan_inode
remove_orphan_inode() will decrement sbi->n_orphans, which was initialized
to zero by init_orphan_info(). Since it is unsigned, it rolls over and
from then on the check in acquire_orphan_inode() returns ENOSPC.
The obvious fix seems to be to call acquire_orphan_inode()
from recover_dentry(), but I thought I'd run it past you first.
Russ
On Thu, Aug 1, 2013 at 4:44 AM, Gu Zheng <[email protected]> wrote:
> 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);
>
>
>
>
> ------------------------------------------------------------------------------
> Get your SQL database under version control now!
> Version control is standard for application code, but databases havent
> caught up. So what steps can you take to put your SQL databases under
> version control? Why should you start doing it? Read more to find out.
> http://pubads.g.doubleclick.net/gampad/clk?id=49501711&iu=/4140/ostg.clktrk
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. Consolidate legacy IT systems to a single system of record for IT
2. Standardize and globalize service processes across IT
3. Implement zero-touch automation to replace manual, redundant tasks
http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
