From patchwork Fri Jun 28 06:38:35 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "jeff.liu" X-Patchwork-Id: 2797201 Return-Path: X-Original-To: patchwork-ocfs2-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 4CA7ABF4A1 for ; Fri, 28 Jun 2013 06:39:14 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9638920126 for ; Fri, 28 Jun 2013 06:39:10 +0000 (UTC) Received: from userp1040.oracle.com (userp1040.oracle.com [156.151.31.81]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5542420123 for ; Fri, 28 Jun 2013 06:39:09 +0000 (UTC) Received: from acsinet22.oracle.com (acsinet22.oracle.com [141.146.126.238]) by userp1040.oracle.com (Sentrion-MTA-4.3.1/Sentrion-MTA-4.3.1) with ESMTP id r5S6WGcS012379 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 28 Jun 2013 06:32:17 GMT Received: from oss.oracle.com (oss-external.oracle.com [137.254.96.51]) by acsinet22.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id r5S6cYVg023253 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 28 Jun 2013 06:38:35 GMT Received: from localhost ([127.0.0.1] helo=oss.oracle.com) by oss.oracle.com with esmtp (Exim 4.63) (envelope-from ) id 1UsSKI-0007Bc-JD; Thu, 27 Jun 2013 23:38:34 -0700 Received: from ucsinet22.oracle.com ([156.151.31.94]) by oss.oracle.com with esmtp (Exim 4.63) (envelope-from ) id 1UsSKC-0007BP-Fn for ocfs2-devel@oss.oracle.com; Thu, 27 Jun 2013 23:38:28 -0700 Received: from userz7022.oracle.com (userz7022.oracle.com [156.151.31.86]) by ucsinet22.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id r5S6cRLV028641 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 28 Jun 2013 06:38:28 GMT Received: from abhmt113.oracle.com (abhmt113.oracle.com [141.146.116.65]) by userz7022.oracle.com (8.14.4+Sun/8.14.4) with ESMTP id r5S6cRlf028624; Fri, 28 Jun 2013 06:38:27 GMT Received: from [192.168.1.104] (/123.119.101.94) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Thu, 27 Jun 2013 23:38:26 -0700 Message-ID: <51CD2F6B.8010503@oracle.com> Date: Fri, 28 Jun 2013 14:38:35 +0800 From: Jeff Liu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120410 Thunderbird/11.0.1 MIME-Version: 1.0 To: Younger Liu References: <51CBAC04.6010805@huawei.com> <20130627145855.6ed028022d3a63fa97dcee13@linux-foundation.org> <51CD2494.8070300@huawei.com> In-Reply-To: <51CD2494.8070300@huawei.com> Cc: Ocfs2-Devel Subject: Re: [Ocfs2-devel] [PATCH] ocfs2: fix readonly issue in ocfs2_unlink() X-BeenThere: ocfs2-devel@oss.oracle.com X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ocfs2-devel-bounces@oss.oracle.com Errors-To: ocfs2-devel-bounces@oss.oracle.com X-Source-IP: acsinet22.oracle.com [141.146.126.238] X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 06/28/2013 01:52 PM, Younger Liu wrote: > On 2013/6/28 5:58, Andrew Morton wrote: >> On Thu, 27 Jun 2013 11:05:40 +0800 Younger Liu wrote: >> >>> While deleting a file with ocfs2_unlink(), there is a bug in this >>> function. This bug will result in filesystem read-only. >>> >>> After calling ocfs2_orphan_add(), the file which will be deleted >>> is added into orphan dir. If ocfs2_delete_entry() fails, >>> the file still exists in the parent dir. >>> And this scenario introduces a conflict of metadata. >>> >>> If a file is added into orphan dir, when we put inode of the file >>> with iput(), the inode i_flags is setted (~OCFS2_VALID_FL) in >>> ocfs2_remove_inode(), and then write back to disk. >>> >>> But as previously mentioned, the file still exists in the parent dir. >>> On other nodes, the file can be still accessed. When first read the file >>> with ocfs2_read_blocks() from disk, It will check and avalidate inode >>> using ocfs2_validate_inode_block(). >>> So File system will be readonly because the inode is invalid. >>> In other words, the inode i_flags has been setted (~OCFS2_VALID_FL). >>> >>> ... >>> >>> --- a/fs/ocfs2/namei.c >>> +++ b/fs/ocfs2/namei.c >>> @@ -790,7 +790,7 @@ static int ocfs2_unlink(struct inode *dir, >>> struct dentry *dentry) >>> { >>> int status; >>> - int child_locked = 0; >>> + int child_locked = 0, is_unlinkable = 0; >> >> Please note that the surrounding code was carful to use the >> one-definition-per-line convention. That's a good convention - more >> readable, less patch rejects during code evolution, leaves room for a >> nice little comment. >> >> Also, type `bool' would have been appropraite here. >> >>> struct inode *inode = dentry->d_inode; >>> struct inode *orphan_dir = NULL; >>> struct ocfs2_super *osb = OCFS2_SB(dir->i_sb); >>> @@ -873,6 +873,7 @@ static int ocfs2_unlink(struct inode *dir, >>> mlog_errno(status); >>> goto leave; >>> } >>> + is_unlinkable = 1; >>> } >>> >>> handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb)); >>> @@ -892,15 +893,6 @@ static int ocfs2_unlink(struct inode *dir, >>> >>> fe = (struct ocfs2_dinode *) fe_bh->b_data; >>> >>> - if (inode_is_unlinkable(inode)) { >>> - status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name, >>> - &orphan_insert, orphan_dir); >>> - if (status < 0) { >>> - mlog_errno(status); >>> - goto leave; >>> - } >>> - } >>> - >>> /* delete the name from the parent dir */ >>> status = ocfs2_delete_entry(handle, dir, &lookup); >>> if (status < 0) { >>> @@ -923,6 +915,14 @@ static int ocfs2_unlink(struct inode *dir, >>> mlog_errno(status); >>> if (S_ISDIR(inode->i_mode)) >>> inc_nlink(dir); >>> + goto leave; >>> + } >>> + >>> + if (is_unlinkable) { >>> + status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name, >>> + &orphan_insert, orphan_dir); >>> + if (status < 0) >>> + mlog_errno(status); >>> } >> >> This is yet another ocfs2 function which reports the same error two >> times. ho hum. >> > Thanks for your review. > >> Please review: >> >> --- a/fs/ocfs2/namei.c~ocfs2-fix-readonly-issue-in-ocfs2_unlink-fix >> +++ a/fs/ocfs2/namei.c >> @@ -790,7 +790,8 @@ static int ocfs2_unlink(struct inode *di >> struct dentry *dentry) >> { >> int status; >> - int child_locked = 0, is_unlinkable = 0; >> + int child_locked = 0; >> + bool is_unlinkable = false; >> struct inode *inode = dentry->d_inode; >> struct inode *orphan_dir = NULL; >> struct ocfs2_super *osb = OCFS2_SB(dir->i_sb); >> @@ -873,7 +874,7 @@ static int ocfs2_unlink(struct inode *di >> mlog_errno(status); >> goto leave; >> } >> - is_unlinkable = 1; >> + is_unlinkable = true; >> } >> >> handle = ocfs2_start_trans(osb, ocfs2_unlink_credits(osb->sb)); >> @@ -919,8 +920,8 @@ static int ocfs2_unlink(struct inode *di >> } >> >> if (is_unlinkable) { >> - status = ocfs2_orphan_add(osb, handle, inode, fe_bh, orphan_name, >> - &orphan_insert, orphan_dir); >> + status = ocfs2_orphan_add(osb, handle, inode, fe_bh, >> + orphan_name, &orphan_insert, orphan_dir); >> if (status < 0) >> mlog_errno(status); >> } >> _ >> > ... > > This patch looks fine to me. I also test it, and the result is fine. > You can consider to add: > Reviewed-by: Younger Liu I'd like to have a minor change for the naming conventions of inode_is_unlinkable() to avoid any possible conflicts to the VFS exported methods in the future, i.e, s/inode_is_unlinkable/ocfs2_inode_is_unlinkable/ How about below changes in addition to above fix up? Thanks, -Jeff diff --git a/fs/ocfs2/namei.c b/fs/ocfs2/namei.c index d11cf81..6708d29 100644 --- a/fs/ocfs2/namei.c +++ b/fs/ocfs2/namei.c @@ -773,7 +773,7 @@ static int ocfs2_remote_dentry_delete(struct dentry *dentry) return ret; } -static inline int inode_is_unlinkable(struct inode *inode) +static inline int ocfs2_inode_is_unlinkable(struct inode *inode) { if (S_ISDIR(inode->i_mode)) { if (inode->i_nlink == 2) @@ -866,7 +866,7 @@ static int ocfs2_unlink(struct inode *dir, goto leave; } - if (inode_is_unlinkable(inode)) { + if (ocfs2_inode_is_unlinkable(inode)) { status = ocfs2_prepare_orphan_dir(osb, &orphan_dir, OCFS2_I(inode)->ip_blkno, orphan_name, &orphan_insert);