From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932156AbdJRBti (ORCPT ); Tue, 17 Oct 2017 21:49:38 -0400 Received: from szxga04-in.huawei.com ([45.249.212.190]:8488 "EHLO szxga04-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762549AbdJRBtg (ORCPT ); Tue, 17 Oct 2017 21:49:36 -0400 Subject: Re: [f2fs-dev] [PATCH] f2fs: handle error case when adding xattr entry To: Jaegeuk Kim , Chao Yu CC: , , References: <20171016230642.94228-1-jaegeuk@kernel.org> <20171017164154.GA5509@jaegeuk-macbookpro.roam.corp.google.com> From: Chao Yu Message-ID: Date: Wed, 18 Oct 2017 09:47:42 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171017164154.GA5509@jaegeuk-macbookpro.roam.corp.google.com> Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.134.22.195] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020202.59E6B320.011C,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: ce48779431d5f373ec7f4db5e415f7f3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017/10/18 0:41, Jaegeuk Kim wrote: > On 10/17, Chao Yu wrote: >> On 2017/10/17 7:06, Jaegeuk Kim wrote: >>> This patch fixes recovering incomplete xattr entries remaining in inline xattr >>> and xattr block, caused by any kind of errors. >>> >>> Signed-off-by: Jaegeuk Kim >>> --- >>> fs/f2fs/xattr.c | 48 ++++++++++++++++++++++++++++-------------------- >>> 1 file changed, 28 insertions(+), 20 deletions(-) >>> >>> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c >>> index e74a4d7f744a..5a9c5e6ad714 100644 >>> --- a/fs/f2fs/xattr.c >>> +++ b/fs/f2fs/xattr.c >>> @@ -389,10 +389,11 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize, >>> { >>> struct f2fs_sb_info *sbi = F2FS_I_SB(inode); >>> size_t inline_size = inline_xattr_size(inode); >>> - void *xattr_addr; >>> + struct page *in_page = NULL; >>> + void *xattr_addr, *inline_addr; >>> struct page *xpage; >>> nid_t new_nid = 0; >>> - int err; >>> + int err = 0; >>> >>> if (hsize > inline_size && !F2FS_I(inode)->i_xattr_nid) >>> if (!alloc_nid(sbi, &new_nid)) >>> @@ -400,30 +401,30 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize, >>> >>> /* write to inline xattr */ >>> if (inline_size) { >>> - struct page *page = NULL; >>> - void *inline_addr; >>> - >>> if (ipage) { >>> inline_addr = inline_xattr_addr(inode, ipage); >>> - f2fs_wait_on_page_writeback(ipage, NODE, true); >>> - set_page_dirty(ipage); >>> } else { >>> - page = get_node_page(sbi, inode->i_ino); >>> - if (IS_ERR(page)) { >>> + in_page = get_node_page(sbi, inode->i_ino); >>> + if (IS_ERR(in_page)) { >>> alloc_nid_failed(sbi, new_nid); >>> - return PTR_ERR(page); >>> + return PTR_ERR(in_page); >>> } >>> - inline_addr = inline_xattr_addr(inode, page); >>> - f2fs_wait_on_page_writeback(page, NODE, true); >>> + inline_addr = inline_xattr_addr(inode, in_page); >>> } >>> - memcpy(inline_addr, txattr_addr, inline_size); >>> - f2fs_put_page(page, 1); >>> >>> + f2fs_wait_on_page_writeback(ipage ? ipage : in_page, >>> + NODE, true); >>> /* no need to use xattr node block */ >>> if (hsize <= inline_size) { >>> err = truncate_xattr_node(inode, ipage); >> >> truncate_xattr_node(inode, ipage ? ipage : in_page); > > No, that should be ipage. I just noted that dn.inode_page_locked in truncate_xattr_node will be set wrong, but, anyway, it looks that won't be problem because we didn't use inode_page_locked later. There is no more users of ipage in truncate_xattr_node, so no matter we passing, there will be safe for us, right? Thanks, > > Thanks, > >> Please add: >> >> Reviewed-by: Chao Yu >> >> Thanks, >> >>> alloc_nid_failed(sbi, new_nid); >>> - return err; >>> + if (err) { >>> + f2fs_put_page(in_page, 1); >>> + return err; >>> + } >>> + memcpy(inline_addr, txattr_addr, inline_size); >>> + set_page_dirty(ipage ? ipage : in_page); >>> + goto in_page_out; >>> } >>> } >>> >>> @@ -432,7 +433,7 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize, >>> xpage = get_node_page(sbi, F2FS_I(inode)->i_xattr_nid); >>> if (IS_ERR(xpage)) { >>> alloc_nid_failed(sbi, new_nid); >>> - return PTR_ERR(xpage); >>> + goto in_page_out; >>> } >>> f2fs_bug_on(sbi, new_nid); >>> f2fs_wait_on_page_writeback(xpage, NODE, true); >>> @@ -442,17 +443,24 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize, >>> xpage = new_node_page(&dn, XATTR_NODE_OFFSET); >>> if (IS_ERR(xpage)) { >>> alloc_nid_failed(sbi, new_nid); >>> - return PTR_ERR(xpage); >>> + goto in_page_out; >>> } >>> alloc_nid_done(sbi, new_nid); >>> } >>> - >>> xattr_addr = page_address(xpage); >>> + >>> + if (inline_size) >>> + memcpy(inline_addr, txattr_addr, inline_size); >>> memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE); >>> + >>> + if (inline_size) >>> + set_page_dirty(ipage ? ipage : in_page); >>> set_page_dirty(xpage); >>> - f2fs_put_page(xpage, 1); >>> >>> - return 0; >>> + f2fs_put_page(xpage, 1); >>> +in_page_out: >>> + f2fs_put_page(in_page, 1); >>> + return err; >>> } >>> >>> int f2fs_getxattr(struct inode *inode, int index, const char *name, >>> > > . >