Re: [PATCH] f2fs: fix handling orphan inodes

2013-08-01 Thread Gu Zheng 
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 
> ---
>  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(>orphan_inode_mutex);
>   if (sbi->n_orphans >= max_orphans)
>   err = -ENOSPC;
> + else
> + sbi->n_orphans++;
>   mutex_unlock(>orphan_inode_mutex);
>   return err;
>  }
>  
> +void release_orphan_inode(struct f2fs_sb_info *sbi)
> +{
> + mutex_lock(>orphan_inode_mutex);
> + sbi->n_orphans--;
> + mutex_unlock(>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(>list, this->prev);
>   else
>   list_add_tail(>list, head);
> -
> - sbi->n_orphans++;
>  out:
>   mutex_unlock(>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,
>

[PATCH] f2fs: fix handling orphan inodes

2013-08-01 Thread Jaegeuk Kim 
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.

Signed-off-by: Jaegeuk Kim 
---
 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(>orphan_inode_mutex);
if (sbi->n_orphans >= max_orphans)
err = -ENOSPC;
+   else
+   sbi->n_orphans++;
mutex_unlock(>orphan_inode_mutex);
return err;
 }
 
+void release_orphan_inode(struct f2fs_sb_info *sbi)
+{
+   mutex_lock(>orphan_inode_mutex);
+   sbi->n_orphans--;
+   mutex_unlock(>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(>list, this->prev);
else
list_add_tail(>list, head);
-
-   sbi->n_orphans++;
 out:
mutex_unlock(>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, _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

[PATCH] f2fs: fix handling orphan inodes

2013-08-01 Thread Jaegeuk Kim 
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.

Signed-off-by: Jaegeuk Kim jaegeuk@samsung.com
---
 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,

Re: [PATCH] f2fs: fix handling orphan inodes

2013-08-01 Thread Gu Zheng 
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@samsung.com
 ---
  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);