From patchwork Mon Feb 18 11:22:09 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Miao Xie X-Patchwork-Id: 2156841 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 3E8373FCFC for ; Mon, 18 Feb 2013 11:21:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752861Ab3BRLVb (ORCPT ); Mon, 18 Feb 2013 06:21:31 -0500 Received: from cn.fujitsu.com ([222.73.24.84]:2024 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751897Ab3BRLV3 (ORCPT ); Mon, 18 Feb 2013 06:21:29 -0500 X-IronPort-AV: E=Sophos;i="4.84,686,1355068800"; d="scan'208";a="6724259" Received: from unknown (HELO tang.cn.fujitsu.com) ([10.167.250.3]) by song.cn.fujitsu.com with ESMTP; 18 Feb 2013 19:19:07 +0800 Received: from fnstmail02.fnst.cn.fujitsu.com (tang.cn.fujitsu.com [127.0.0.1]) by tang.cn.fujitsu.com (8.14.3/8.13.1) with ESMTP id r1IBLNnD005775; Mon, 18 Feb 2013 19:21:23 +0800 Received: from [10.167.225.199] ([10.167.225.199]) by fnstmail02.fnst.cn.fujitsu.com (Lotus Domino Release 8.5.3) with ESMTP id 2013021819204621-215890 ; Mon, 18 Feb 2013 19:20:46 +0800 Message-ID: <51220EE1.6090607@cn.fujitsu.com> Date: Mon, 18 Feb 2013 19:22:09 +0800 From: Miao Xie Reply-To: miaox@cn.fujitsu.com User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Josef Bacik CC: linux-btrfs@vger.kernel.org Subject: Re: [PATCH] Btrfs: place ordered operations on a per transaction list References: <1360772002-8923-1-git-send-email-jbacik@fusionio.com> In-Reply-To: <1360772002-8923-1-git-send-email-jbacik@fusionio.com> X-MIMETrack: Itemize by SMTP Server on mailserver/fnst(Release 8.5.3|September 15, 2011) at 2013/02/18 19:20:46, Serialize by Router on mailserver/fnst(Release 8.5.3|September 15, 2011) at 2013/02/18 19:20:47, Serialize complete at 2013/02/18 19:20:47 Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org On wed, 13 Feb 2013 11:13:22 -0500, Josef Bacik wrote: > Miao made the ordered operations stuff run async, which introduced a > deadlock where we could get somebody (sync) racing in and committing the > transaction while a commit was already happening. The new committer would > try and flush ordered operations which would hang waiting for the commit to > finish because it is done asynchronously and no longer inherits the callers > trans handle. To fix this we need to make the ordered operations list a per > transaction list. We can get new inodes added to the ordered operation list > by truncating them and then having another process writing to them, so this > makes it so that anybody trying to add an ordered operation _must_ start a > transaction in order to add itself to the list, which will keep new inodes > from getting added to the ordered operations list after we start committing. > This should fix the deadlock and also keeps us from doing a lot more work > than we need to during commit. Thanks, Firstly, thanks to deal with the bug which was introduced by my patch. But comparing with this fix method, I prefer the following one because: - we won't worry the similar problem if we add more work during commit in the future. - it is unnecessary to get a new handle and commit it if the transaction is under the commit. Thanks Miao diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index fc03aa6..c449cb5 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -277,7 +277,8 @@ static void wait_current_trans(struct btrfs_root *root) } } -static int may_wait_transaction(struct btrfs_root *root, int type) +static int may_wait_transaction(struct btrfs_root *root, int type, + bool is_joined) { if (root->fs_info->log_root_recovering) return 0; @@ -285,6 +286,14 @@ static int may_wait_transaction(struct btrfs_root *root, int type) if (type == TRANS_USERSPACE) return 1; + /* + * If we are ATTACH, it means we just want to catch the current + * transaction and commit it. So if someone is committing the + * current transaction now, it is very glad to wait it. + */ + if (is_joined && type == TRANS_ATTACH) + return 1; + if (type == TRANS_START && !atomic_read(&root->fs_info->open_ioctl_trans)) return 1; @@ -355,7 +364,7 @@ again: if (type < TRANS_JOIN_NOLOCK) sb_start_intwrite(root->fs_info->sb); - if (may_wait_transaction(root, type)) + if (may_wait_transaction(root, type, false)) wait_current_trans(root); do { @@ -383,16 +392,26 @@ again: h->block_rsv = NULL; h->orig_rsv = NULL; h->aborted = 0; - h->qgroup_reserved = qgroup_reserved; + h->qgroup_reserved = 0; h->delayed_ref_elem.seq = 0; h->type = type; INIT_LIST_HEAD(&h->qgroup_ref_list); INIT_LIST_HEAD(&h->new_bgs); smp_mb(); - if (cur_trans->blocked && may_wait_transaction(root, type)) { - btrfs_commit_transaction(h, root); - goto again; + if (cur_trans->blocked && may_wait_transaction(root, type, true)) { + if (cur_trans->in_commit) { + btrfs_end_transaction(h, root); + wait_current_trans(root); + } else { + btrfs_commit_transaction(h, root); + } + if (unlikely(type == TRANS_ATTACH)) { + ret = -ENOENT; + goto alloc_fail; + } else { + goto again; + } } if (num_bytes) { @@ -401,6 +420,7 @@ again: h->block_rsv = &root->fs_info->trans_block_rsv; h->bytes_reserved = num_bytes; } + h->qgroup_reserved = qgroup_reserved; got_it: btrfs_record_root_in_trans(h, root);