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:   Tue, 4 Apr 2023 21:10:42 +0200
From:   David Sterba <dsterba@suse.cz>
To:     xiaoshoukui <xiaoshoukui@gmail.com>
Cc:     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: <20230404191042.GE19619@suse.cz>
Reply-To: dsterba@suse.cz
References: <20230403093757.120178-1-xiaoshoukui@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230403093757.120178-1-xiaoshoukui@gmail.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Mon, Apr 03, 2023 at 05:37:57AM -0400, xiaoshoukui wrote:
> > 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.
> 
> An assertion failure means that the code may not have taken careful consideration.
> After I patched the BTRFS_EXCLOP_NONE to the assertion, regression tests shows that
> another scenario I missed.
> 
> With started state == BTRFS_EXCLOP_BALANCE_PAUSED, cocurrently adding multiple devices
> to the same mount point and btrfs_exclop_balance executed finish before the latter
> thread execute assertion in btrfs_exclop_balance, exclusive_operation will changed to 
> BTRFS_EXCLOP_BALANCE_PAUSED state. 
> 
> I also added btrfs_info before ASSERT to help troubleshooting:
> > btrfs_info(fs_info, "fs_info exclusive_operation: %d",
> >        fs_info->exclusive_operation);
> > ASSERT(fs_info->exclusive_operation == BTRFS_EXCLOP_BALANCE ||
> >        fs_info->exclusive_operation == BTRFS_EXCLOP_DEV_ADD ||
> >        fs_info->exclusive_operation == BTRFS_EXCLOP_NONE);

> I think the assertion should also check for BTRFS_EXCLOP_BALANCE_PAUSED status.

Agreed and this looks like the complete set of the compatible
operations. I hope this does not enable some combination that should not
be allowed but the enabling side does the try lock and allows only the
paused + dev_add combination.

Please send a fix with the analysis you dit and add the relevant parts
of stack traces. The reproducer would be good to have in fstests, for
the changelog please describe the conditions that could trigger the
assertion, the reproducer itself is too long. Thanks.