f2fs: fix to release inode page in get_new_data_page

PATCH Superseded Chao Yu 3 participants 5 messages Jul 9, 2015 – Jul 14, 2015 F2FS file system

Share & tools

Chao Yu

author

2015-07-09 10:20 UTC #0 lore raw

PATCH 1/1 f2fs: fix to release inode page in get_new_data_page

get_new_data_page should release inode page when we encounter any error in its procedure, but there is one error path didn't cover this, fix it.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/data.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/f2fs/data.c +3 -1

index 08dfdc6..ea8898b 100644
@@ -397,8 +397,10 @@ struct page *get_new_data_page(struct inode *inode,
 	int err;
 repeat:
 	page = grab_cache_page(mapping, index);
-	if (!page)
+	if (!page) {
+		f2fs_put_page(ipage, 1);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	set_new_dnode(&dn, inode, ipage, NULL, 0);
 	err = f2fs_reserve_block(&dn, index);

Signature · 10 lines

2.4.2



------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that
you need to offload your IT needs and focus on growing your business.
Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/

Jaegeuk Kim

maintainer

2015-07-11 00:17 UTC #1 lore raw

Hi Chao,

Quoted 3 lines · Chao Yu

get_new_data_page should release inode page when we encounter any error in its procedure, but there is one error path didn't cover this, fix it.

Nice catch.
But, I think we should fix its caller:

in init_inode_metadata(),

	err = make_empty_dir();
	if (err)
		goto put_error;
		---------------

Thanks,

Quoted 24 lines

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/data.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/f2fs/data.c +3 -1

index 08dfdc6..ea8898b 100644
@@ -397,8 +397,10 @@ struct page *get_new_data_page(struct inode *inode,
 	int err;
 repeat:
 	page = grab_cache_page(mapping, index);
-	if (!page)
+	if (!page) {
+		f2fs_put_page(ipage, 1);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	set_new_dnode(&dn, inode, ipage, NULL, 0);
 	err = f2fs_reserve_block(&dn, index);

Signature · 1 line

2.4.2

------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/

Chao Yu

2015-07-11 02:03 UTC #2 lore raw

Hi Jaegeuk,

Quoted 15 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Saturday, July 11, 2015 8:17 AM
To: Chao Yu; Chao Yu
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page
Hi Chao,
Quoted 3 lines · Chao Yu
get_new_data_page should release inode page when we encounter any error in its procedure, but there is one error path didn't cover this, fix it.

Quoted 8 lines

Nice catch.
But, I think we should fix its caller:
in init_inode_metadata(),
	err = make_empty_dir();
	if (err)
		goto put_error;
		---------------

Previously, I fixed in the same way, but I got an oops when I test the patch with xfstest suit, it shows we will meet an error in this call path IIRC:

->f2fs_mkdir
->__f2fs_add_link

    ->init_inode_metadata
      ->make_empty_dir
        ->get_new_data_page
          ->f2fs_reserve_block
            ->reserve_new_block
              ->inc_valid_block_count
                return -ENOSPC;
              ->f2fs_put_dnode
                ->f2fs_put_page(ipage, 1)

      put_error:
        ->f2fs_put_page(ipage, 1);
          f2fs_bug_on(F2FS_P_SB(page), !PageLocked(page));

And I don't think we should change error handling method of f2fs_put_dnode for just fixing this issue.

How do you think?

Thanks,

Quoted 22 lines

Thanks,

Quoted 20 lines

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/data.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/f2fs/data.c +3 -1

index 08dfdc6..ea8898b 100644
@@ -397,8 +397,10 @@ struct page *get_new_data_page(struct inode *inode,
 	int err;
 repeat:
 	page = grab_cache_page(mapping, index);
-	if (!page)
+	if (!page) {
+		f2fs_put_page(ipage, 1);
 		return ERR_PTR(-ENOMEM);
+	}

Quoted 15 lines

Quoted 4 lines
fs/f2fs/data.c +0 -1
 	set_new_dnode(&dn, inode, ipage, NULL, 0);
 	err = f2fs_reserve_block(&dn, index);
--
2.4.2
------------------------------------------------------------------------------
Don't Limit Your Business. Reach for the Cloud.
GigeNET's Cloud Solutions provide you with the tools and support that you need to offload your IT needs and focus on growing your business. Configured For All Businesses. Start Your Cloud Today.
https://www.gigenetcloud.com/
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Jaegeuk Kim

maintainer

2015-07-13 23:26 UTC #3 lore raw

Hi Chao,

Quoted 46 lines · Chao Yu

Hi Jaegeuk,
Quoted 15 lines
-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Saturday, July 11, 2015 8:17 AM
To: Chao Yu; Chao Yu
Cc: [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page
Hi Chao,
Quoted 3 lines · Chao Yu
get_new_data_page should release inode page when we encounter any error in its procedure, but there is one error path didn't cover this, fix it.
Quoted 8 lines
Nice catch.
But, I think we should fix its caller:
in init_inode_metadata(),
	err = make_empty_dir();
	if (err)
		goto put_error;
		---------------
Previously, I fixed in the same way, but I got an oops when I test the patch with xfstest suit, it shows we will meet an error in this call path IIRC:
->f2fs_mkdir
->__f2fs_add_link
    ->init_inode_metadata
      ->make_empty_dir
        ->get_new_data_page
          ->f2fs_reserve_block
            ->reserve_new_block
              ->inc_valid_block_count
                return -ENOSPC;
              ->f2fs_put_dnode
                ->f2fs_put_page(ipage, 1)

      put_error:
        ->f2fs_put_page(ipage, 1);
          f2fs_bug_on(F2FS_P_SB(page), !PageLocked(page));

Quoted 4 lines

And I don't think we should change error handling method of f2fs_put_dnode for just fixing this issue.
How do you think?

Indeed. I cannot think about other clean way for now.
Instead, how about adding this description in the patch and some comments in the codes?

Thanks,

Chao Yu

author

2015-07-14 10:13 UTC #4 lore raw

Quoted 6 lines

-----Original Message-----
From: Jaegeuk Kim [mailto:[email protected]]
Sent: Tuesday, July 14, 2015 7:26 AM
To: Chao Yu
Cc: [email protected]; [email protected]
Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to release inode page in get_new_data_page

[snip]

Quoted 8 lines

Quoted 4 lines
And I don't think we should change error handling method of f2fs_put_dnode for just fixing this issue.
How do you think?
Indeed. I cannot think about other clean way for now.
Instead, how about adding this description in the patch and some comments in the codes?

OK, Please help to review the following patch. :)

Thanks,