Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <284ccc08-8de7-9188-19d8-20f4eda56cb4@dorminy.me>
Date:   Thu, 24 Feb 2022 11:10:59 -0500
MIME-Version: 1.0
Subject: Re: [PATCH v4] btrfs: add fs state details to error messages.
Content-Language: en-US
To:     dsterba@suse.cz, Chris Mason <clm@fb.com>,
        Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@fb.com
References: <a059920460fe13f773fd9a2e870ceb9a8e3a105a.1645644489.git.sweettea-kernel@dorminy.me>
 <20220224132210.GS12643@twin.jikos.cz>
From:   Sweet Tea Dorminy <sweettea-kernel@dorminy.me>
In-Reply-To: <20220224132210.GS12643@twin.jikos.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


> Added to misc-next with some minor updates, thanks.
>
Awesome, thank you. I realized this morning that it might be technically 
slightly racy actually and would propose something like the following

static void btrfs_state_to_string(const struct btrfs_fs_info *info, char *buf)
{
	unsigned int bit;
+	unsigned long fs_state = READ_ONCE(info->fs_state);
	unsigned int states_printed = 0;
	char *curr = buf;

	memcpy(curr, STATE_STRING_PREFACE, sizeof(STATE_STRING_PREFACE));
	curr += sizeof(STATE_STRING_PREFACE) - 1;

-	for_each_set_bit(bit, &info->fs_state, sizeof(info->fs_state)) {
+	for_each_set_bit(bit, fs_state, sizeof(fs_state)) {


All the other interactions with info->fs_state are test/set/clear_bit, which treat the argument as volatile and are therefore safe to do from multiple threads. Without the READ_ONCE (reading it as a volatile), the compiler or cpu could turn the reads of info->fs_state in for_each_set_bit() into writes of random stuff into info->fs_state, potentially clearing the state bits or filling them with garbage.

Even if this is right, it'd be rare, but it would be exceeding weird for a message to be logged listing an error and then future messages be logged without any such state, or with a random collection of garbage states.

I can send another patch if this explanation seems reasonable and you'd like it separate; or maybe this is too unlikely to worry about. Thanks again for the review!

Sweet Tea