Hi Jaegeuk,

> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaeg...@kernel.org]
> Sent: Saturday, July 11, 2015 8:17 AM
> To: Chao Yu; Chao Yu
> Cc: linux-ker...@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net;
> linux-ker...@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH] f2fs: fix to release inode page in 
> get_new_data_page
> 
> Hi Chao,
> 
> On Thu, Jul 09, 2015 at 06:20:08PM +0800, Chao Yu wrote:
> > 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;
>               ---------------

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,

> Thanks,
> 
> >
> > Signed-off-by: Chao Yu <chao2...@samsung.com>
> > ---
> >  fs/f2fs/data.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 08dfdc6..ea8898b 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -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);
> > --
> > 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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------
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
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Reply via email to