From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cn.fujitsu.com ([222.73.24.84]:60806 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1758482Ab3EGIQ4 (ORCPT ); Tue, 7 May 2013 04:16:56 -0400 Message-ID: <5188B3BD.8090807@cn.fujitsu.com> Date: Tue, 07 May 2013 15:56:45 +0800 From: Wang Shilong MIME-Version: 1.0 To: Jan Schmidt CC: Linux Btrfs Subject: Re: [PATCH] Btrfs: save us a mutex_lock usage when doing quota rescan References: <51889BF1.1000300@cn.fujitsu.com> <5188A772.2090007@jan-o-sch.net> In-Reply-To: <5188A772.2090007@jan-o-sch.net> Content-Type: text/plain; charset=UTF-8 Sender: linux-btrfs-owner@vger.kernel.org List-ID: Hello Jan, > On Tue, May 07, 2013 at 08:15 (+0200), Wang Shilong wrote: >> If qgroup_rescan worker is in progress, we should ignore >> the extent that has not been dealt with qgroup_rescan worker,just >> let them dealt later otherwise we may get wrong qgroup accounting. >> >> However, we have checked this before find_all_roots() without spin_lock. >> When doing qgroup accounting, we don't have to check it again, because >> during this period,qgroup_rescan worker can deal with more extents and >> qgroup_rescan_extent->objectid can only go larger, so here the check >> is unnecessary. >> >> Just remove this check, so that we don't need hold qgroup_rescan_lock >> when doing qgroup accounting. > > NAK. > > After a discussion on that lock the last thing in this thread I see is ... > > On Wed, May 01, 2013 at 13:57 (+0200), Jan Schmidt wrote: >> Now I see what you mean. The second check is only required when we start >> a rescan operation after the initial check in btrfs_qgroup_account_ref. > > Please continue on that argument, your commit message doesn't explain at all why > we should be safe to remove this check. You are right! Thanks, Wang > > -Jan > >> Signed-off-by: Wang Shilong >> --- >> fs/btrfs/qgroup.c | 9 --------- >> 1 files changed, 0 insertions(+), 9 deletions(-) >> >> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c >> index d059d86..2710784 100644 >> --- a/fs/btrfs/qgroup.c >> +++ b/fs/btrfs/qgroup.c >> @@ -1445,15 +1445,7 @@ int btrfs_qgroup_account_ref(struct btrfs_trans_handle *trans, >> if (ret < 0) >> return ret; >> >> - mutex_lock(&fs_info->qgroup_rescan_lock); >> spin_lock(&fs_info->qgroup_lock); >> - if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN) { >> - if (fs_info->qgroup_rescan_progress.objectid <= node->bytenr) { >> - ret = 0; >> - goto unlock; >> - } >> - } >> - >> quota_root = fs_info->quota_root; >> if (!quota_root) >> goto unlock; >> @@ -1492,7 +1484,6 @@ int btrfs_qgroup_account_ref(struct btrfs_trans_handle *trans, >> >> unlock: >> spin_unlock(&fs_info->qgroup_lock); >> - mutex_unlock(&fs_info->qgroup_rescan_lock); >> ulist_free(roots); >> >> return ret; >> > >