From patchwork Mon Mar 31 21:02:16 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Morton X-Patchwork-Id: 3917851 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 C2282BF540 for ; Mon, 31 Mar 2014 21:03:29 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 88D8A203AF for ; Mon, 31 Mar 2014 21:03:28 +0000 (UTC) Received: from aserp1040.oracle.com (aserp1040.oracle.com [141.146.126.69]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EA099203AE for ; Mon, 31 Mar 2014 21:03:26 +0000 (UTC) Received: from acsinet22.oracle.com (acsinet22.oracle.com [141.146.126.238]) by aserp1040.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id s2VL2ePQ016449 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 31 Mar 2014 21:02:41 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 s2VL2Zmv001460 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 31 Mar 2014 21:02:36 GMT Received: from localhost ([127.0.0.1] helo=oss.oracle.com) by oss.oracle.com with esmtp (Exim 4.63) (envelope-from ) id 1WUjLn-0002kP-JP; Mon, 31 Mar 2014 14:02:35 -0700 Received: from ucsinet22.oracle.com ([156.151.31.94]) by oss.oracle.com with esmtp (Exim 4.63) (envelope-from ) id 1WUjLX-0002jg-Dm for ocfs2-devel@oss.oracle.com; Mon, 31 Mar 2014 14:02:19 -0700 Received: from userp1030.oracle.com (userp1030.oracle.com [156.151.31.80]) by ucsinet22.oracle.com (8.14.5+Sun/8.14.5) with ESMTP id s2VL2IQr025397 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Mon, 31 Mar 2014 21:02:19 GMT Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) by userp1030.oracle.com (Sentrion-MTA-4.3.2/Sentrion-MTA-4.3.2) with ESMTP id s2VL2H5O024975 for ; Mon, 31 Mar 2014 21:02:18 GMT Received: from akpm3.mtv.corp.google.com (unknown [216.239.45.95]) by mail.linuxfoundation.org (Postfix) with ESMTPSA id 9229A885; Mon, 31 Mar 2014 21:02:17 +0000 (UTC) Date: Mon, 31 Mar 2014 14:02:16 -0700 From: Andrew Morton To: Younger Liu Message-Id: <20140331140216.78da0e9b2dbf7c794e9350e0@linux-foundation.org> In-Reply-To: <5339689C.1060807@gmail.com> References: <5339689C.1060807@gmail.com> X-Mailer: Sylpheed 3.2.0beta5 (GTK+ 2.24.10; x86_64-pc-linux-gnu) Mime-Version: 1.0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=2 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=7.0.1-1402240000 definitions=main-1403310110 X-Proofpoint-ZeroHour: threat=unknown, refid=1B2M2Y8AsgTpgAmY7PhCfg X-Proofpoint-Reputation: rule=pass Cc: Mark Fasheh , ocfs2-devel@oss.oracle.com Subject: Re: [Ocfs2-devel] [PATCH] Ocfs2: Rollback alloc_dinode counts when ocfs2_block_group_set_bits() failed 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=-4.8 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 Mon, 31 Mar 2014 21:07:40 +0800 Younger Liu wrote: > After updating alloc_dinode counts in ocfs2_alloc_dinode_update_counts(), > if ocfs2_alloc_dinode_update_bitmap() failed, there is a rare case that some > space may be lost. > So, we rollback alloc_dinode counts when ocfs2_block_group_set_bits() failed. This patch wildly conflicts with your earlier patch ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously.patch. below. What should be done? From: Younger Liu Subject: ocfs2: alloc_dinode counts and group bitmap should be update simultaneously Updating alloc_dinode counts in ocfs2_alloc_dinode_update_counts() and setting group bitmap in ocfs2_alloc_dinode_update_bitmap() have to be done simultaneously. Two cases are as follow: (1) If ocfs2_alloc_dinode_update_counts() fails, there is no need to set group bitmap. This case has been considered. (2) If ocfs2_alloc_dinode_update_bitmap() fails, alloc_dinode counts should be rolled back. Otherwise, some clusters would never be used. This case is not considered. So, we combine two functions, and ensure simultaneity. Signed-off-by: Younger Liu Cc: Joel Becker Cc: Mark Fasheh Signed-off-by: Andrew Morton --- fs/ocfs2/move_extents.c | 11 -- fs/ocfs2/ocfs2_trace.h | 2 fs/ocfs2/suballoc.c | 155 +++++++++++++++----------------------- fs/ocfs2/suballoc.h | 21 +---- 4 files changed, 77 insertions(+), 112 deletions(-) diff -puN fs/ocfs2/move_extents.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously fs/ocfs2/move_extents.c --- a/fs/ocfs2/move_extents.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously +++ a/fs/ocfs2/move_extents.c @@ -681,18 +681,15 @@ static int ocfs2_move_extent(struct ocfs } gd = (struct ocfs2_group_desc *)gd_bh->b_data; - ret = ocfs2_alloc_dinode_update_counts(gb_inode, handle, gb_bh, len, - le16_to_cpu(gd->bg_chain)); + ret = ocfs2_alloc_dinode_update_bitmap(handle, + gb_inode, gb_bh, gd, gb_bh, + le16_to_cpu(gd->bg_chain), + goal_bit, len); if (ret) { mlog_errno(ret); goto out_commit; } - ret = ocfs2_block_group_set_bits(handle, gb_inode, gd, gd_bh, - goal_bit, len); - if (ret) - mlog_errno(ret); - /* * Here we should write the new page out first if we are * in write-back mode. diff -puN fs/ocfs2/ocfs2_trace.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously fs/ocfs2/ocfs2_trace.h --- a/fs/ocfs2/ocfs2_trace.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously +++ a/fs/ocfs2/ocfs2_trace.h @@ -788,7 +788,7 @@ DEFINE_OCFS2_UINT_UINT_UINT_EVENT(ocfs2_ DEFINE_OCFS2_ULL_EVENT(ocfs2_reserve_new_inode_new_group); -DEFINE_OCFS2_UINT_UINT_EVENT(ocfs2_block_group_set_bits); +DEFINE_OCFS2_UINT_UINT_EVENT(ocfs2_alloc_dinode_update_bitmap); TRACE_EVENT(ocfs2_relink_block_group, TP_PROTO(unsigned long long i_blkno, unsigned int chain, diff -puN fs/ocfs2/suballoc.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously fs/ocfs2/suballoc.c --- a/fs/ocfs2/suballoc.c~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously +++ a/fs/ocfs2/suballoc.c @@ -1337,54 +1337,6 @@ static int ocfs2_block_group_find_clear_ return status; } -int ocfs2_block_group_set_bits(handle_t *handle, - struct inode *alloc_inode, - struct ocfs2_group_desc *bg, - struct buffer_head *group_bh, - unsigned int bit_off, - unsigned int num_bits) -{ - int status; - void *bitmap = bg->bg_bitmap; - int journal_type = OCFS2_JOURNAL_ACCESS_WRITE; - - /* All callers get the descriptor via - * ocfs2_read_group_descriptor(). Any corruption is a code bug. */ - BUG_ON(!OCFS2_IS_VALID_GROUP_DESC(bg)); - BUG_ON(le16_to_cpu(bg->bg_free_bits_count) < num_bits); - - trace_ocfs2_block_group_set_bits(bit_off, num_bits); - - if (ocfs2_is_cluster_bitmap(alloc_inode)) - journal_type = OCFS2_JOURNAL_ACCESS_UNDO; - - status = ocfs2_journal_access_gd(handle, - INODE_CACHE(alloc_inode), - group_bh, - journal_type); - if (status < 0) { - mlog_errno(status); - goto bail; - } - - le16_add_cpu(&bg->bg_free_bits_count, -num_bits); - if (le16_to_cpu(bg->bg_free_bits_count) > le16_to_cpu(bg->bg_bits)) { - ocfs2_error(alloc_inode->i_sb, "Group descriptor # %llu has bit" - " count %u but claims %u are freed. num_bits %d", - (unsigned long long)le64_to_cpu(bg->bg_blkno), - le16_to_cpu(bg->bg_bits), - le16_to_cpu(bg->bg_free_bits_count), num_bits); - return -EROFS; - } - while(num_bits--) - ocfs2_set_bit(bit_off++, bitmap); - - ocfs2_journal_dirty(handle, group_bh); - -bail: - return status; -} - /* find the one with the most empty bits */ static inline u16 ocfs2_find_victim_chain(struct ocfs2_chain_list *cl) { @@ -1580,31 +1532,78 @@ static int ocfs2_block_group_search(stru return ret; } -int ocfs2_alloc_dinode_update_counts(struct inode *inode, - handle_t *handle, - struct buffer_head *di_bh, - u32 num_bits, - u16 chain) +int ocfs2_alloc_dinode_update_bitmap(handle_t *handle, + struct inode *alloc_inode, + struct buffer_head *di_bh, + struct ocfs2_group_desc *bg, + struct buffer_head *group_bh, + u16 chain, u32 bit_off, u32 num_bits) { int ret; u32 tmp_used; struct ocfs2_dinode *di = (struct ocfs2_dinode *) di_bh->b_data; struct ocfs2_chain_list *cl = (struct ocfs2_chain_list *) &di->id2.i_chain; + void *bitmap = bg->bg_bitmap; + int journal_type = OCFS2_JOURNAL_ACCESS_WRITE; - ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh, - OCFS2_JOURNAL_ACCESS_WRITE); + /* + * All callers get the descriptor via + * ocfs2_read_group_descriptor(). Any corruption is a code bug. + */ + BUG_ON(!OCFS2_IS_VALID_GROUP_DESC(bg)); + BUG_ON(le16_to_cpu(bg->bg_free_bits_count) < num_bits); + + trace_ocfs2_alloc_dinode_update_bitmap(bit_off, num_bits); + + ret = ocfs2_journal_access_di(handle, + INODE_CACHE(alloc_inode), di_bh, journal_type); if (ret < 0) { mlog_errno(ret); goto out; } + if (ocfs2_is_cluster_bitmap(alloc_inode)) + journal_type = OCFS2_JOURNAL_ACCESS_UNDO; + + ret = ocfs2_journal_access_gd(handle, + INODE_CACHE(alloc_inode), group_bh, journal_type); + if (ret < 0) { + mlog_errno(ret); + goto out; + } + + /* update alloc_dinode counts */ tmp_used = le32_to_cpu(di->id1.bitmap1.i_used); di->id1.bitmap1.i_used = cpu_to_le32(num_bits + tmp_used); le32_add_cpu(&cl->cl_recs[chain].c_free, -num_bits); + + /* update bg counts and bitmap*/ + le16_add_cpu(&bg->bg_free_bits_count, -num_bits); + if (le16_to_cpu(bg->bg_free_bits_count) > le16_to_cpu(bg->bg_bits)) { + ocfs2_error(alloc_inode->i_sb, "Group descriptor # %llu has bit" + " count %u but claims %u are freed. num_bits %d", + (unsigned long long)le64_to_cpu(bg->bg_blkno), + le16_to_cpu(bg->bg_bits), + le16_to_cpu(bg->bg_free_bits_count), num_bits); + ret = -EROFS; + goto out_rollback; + } + while (num_bits--) + ocfs2_set_bit(bit_off++, bitmap); + ocfs2_journal_dirty(handle, di_bh); + ocfs2_journal_dirty(handle, group_bh); out: return ret; + +out_rollback: + le16_add_cpu(&bg->bg_free_bits_count, num_bits); + + di->id1.bitmap1.i_used = cpu_to_le32(tmp_used - num_bits); + le32_add_cpu(&cl->cl_recs[chain].c_free, num_bits); + + return ret; } static int ocfs2_bg_discontig_fix_by_rec(struct ocfs2_suballoc_result *res, @@ -1697,19 +1696,15 @@ static int ocfs2_search_one_group(struct if (ac->ac_find_loc_only) goto out_loc_only; - ret = ocfs2_alloc_dinode_update_counts(alloc_inode, handle, ac->ac_bh, - res->sr_bits, - le16_to_cpu(gd->bg_chain)); + ret = ocfs2_alloc_dinode_update_bitmap(handle, + alloc_inode, ac->ac_bh, gd, group_bh, + le16_to_cpu(gd->bg_chain), + res->sr_bit_offset, res->sr_bits); if (ret < 0) { mlog_errno(ret); goto out; } - ret = ocfs2_block_group_set_bits(handle, alloc_inode, gd, group_bh, - res->sr_bit_offset, res->sr_bits); - if (ret < 0) - mlog_errno(ret); - out_loc_only: *bits_left = le16_to_cpu(gd->bg_free_bits_count); @@ -1823,20 +1818,9 @@ static int ocfs2_search_chain(struct ocf if (ac->ac_find_loc_only) goto out_loc_only; - status = ocfs2_alloc_dinode_update_counts(alloc_inode, handle, - ac->ac_bh, res->sr_bits, - chain); - if (status) { - mlog_errno(status); - goto bail; - } - - status = ocfs2_block_group_set_bits(handle, - alloc_inode, - bg, - group_bh, - res->sr_bit_offset, - res->sr_bits); + status = ocfs2_alloc_dinode_update_bitmap(handle, + alloc_inode, ac->ac_bh, bg, group_bh, + chain, res->sr_bit_offset, res->sr_bits); if (status < 0) { mlog_errno(status); goto bail; @@ -2134,20 +2118,9 @@ int ocfs2_claim_new_inode_at_loc(handle_ bg = (struct ocfs2_group_desc *) bg_bh->b_data; chain = le16_to_cpu(bg->bg_chain); - ret = ocfs2_alloc_dinode_update_counts(ac->ac_inode, handle, - ac->ac_bh, res->sr_bits, - chain); - if (ret) { - mlog_errno(ret); - goto out; - } - - ret = ocfs2_block_group_set_bits(handle, - ac->ac_inode, - bg, - bg_bh, - res->sr_bit_offset, - res->sr_bits); + ret = ocfs2_alloc_dinode_update_bitmap(handle, + ac->ac_inode, ac->ac_bh, bg, bg_bh, + chain, res->sr_bit_offset, res->sr_bits); if (ret < 0) { mlog_errno(ret); goto out; diff -puN fs/ocfs2/suballoc.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously fs/ocfs2/suballoc.h --- a/fs/ocfs2/suballoc.h~ocfs2-alloc_dinode-counts-and-group-bitmap-should-be-update-simultaneously +++ a/fs/ocfs2/suballoc.h @@ -85,19 +85,14 @@ int ocfs2_reserve_new_inode(struct ocfs2 int ocfs2_reserve_clusters(struct ocfs2_super *osb, u32 bits_wanted, struct ocfs2_alloc_context **ac); - -int ocfs2_alloc_dinode_update_counts(struct inode *inode, - handle_t *handle, - struct buffer_head *di_bh, - u32 num_bits, - u16 chain); -int ocfs2_block_group_set_bits(handle_t *handle, - struct inode *alloc_inode, - struct ocfs2_group_desc *bg, - struct buffer_head *group_bh, - unsigned int bit_off, - unsigned int num_bits); - +int ocfs2_alloc_dinode_update_bitmap(handle_t *handle, + struct inode *alloc_inode, + struct buffer_head *di_bh, + struct ocfs2_group_desc *bg, + struct buffer_head *group_bh, + u16 chain, + u32 bit_off, + u32 num_bits); int ocfs2_claim_metadata(handle_t *handle, struct ocfs2_alloc_context *ac, u32 bits_wanted,