Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 31 Mar 2023 19:45:33 +0200
From:   David Sterba <dsterba@suse.cz>
To:     xiaoshoukui <xiaoshoukui@gmail.com>
Cc:     dsterba@suse.cz, clm@fb.com, josef@toxicpanda.com,
        dsterba@suse.com, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org, xiaoshoukui@ruijie.com.cn
Subject: Re: [PATCH] btrfs: ioctl: fix inaccurate determination of
 exclusive_operation
Message-ID: <20230331174533.GZ10580@twin.jikos.cz>
Reply-To: dsterba@suse.cz
References: <20230328094335.107562-1-xiaoshoukui@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230328094335.107562-1-xiaoshoukui@gmail.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Tue, Mar 28, 2023 at 05:43:35AM -0400, xiaoshoukui wrote:
> > Have you found some bug with the above or is there other combination of
> > the exclusive operations that should not work? The changes to the state
> > values are the same, besides the wrong locking.
> 
> Yes, there is a racy bewteen btrfs_exclop_balance and btrfs_exclop_finish
> in btrfs_ioctl_add_dev, when cocurrently adding multiple devices to the
> same mnt point. That will cause the assertion in btrfs_exclop_balance to fail.
> 
> > void btrfs_exclop_balance(struct btrfs_fs_info *fs_info,
> > 			  enum btrfs_exclusive_operation op)
> > {
> > 	switch (op) {
> > 	case BTRFS_EXCLOP_BALANCE_PAUSED:
> > 		spin_lock(&fs_info->super_lock);
> > 		ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE ||
> > 		       fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD);
> 
> when btrfs_exclop_finish function was executed before the ASSERT, the
> fs_info->exclusive_operation will change to BTRFS_EXCLOP_NONE. So this
> assert will failed.
>
> Please review whether we should patch the assert to add BTRFS_EXCLOP_NONE condtion.
> I'll post a patch if needed. thx.

Yeah I think the assertion should also check for NONE status. The paused
balance makes the state tracking harder but in user-started (manual or
scripted) commands it's typically not racing.

btrfs_exclop_start_try_lock does not allow to do the change from
none -> op mandating an explicit btrfs_exclop_start first but the
assertions do not care about that.