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;
Message-ID: <0c2337c3-2b39-c54c-27e9-dd4f0a99cf71@suse.com>
Date:   Thu, 21 Jul 2022 16:37:43 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH]btrfs: Fix fstest case btrfs/219
Content-Language: en-US
To:     "Flint.Wang" <hmsjwzb@zoho.com>, anand.jain@oracle.com
Cc:     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
References: <20220721083609.5695-1-hmsjwzb@zoho.com>
From:   Nikolay Borisov <nborisov@suse.com>
In-Reply-To: <20220721083609.5695-1-hmsjwzb@zoho.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 21.07.22 г. 11:36 ч., Flint.Wang wrote:
> Hi,
> fstest btrfs/291 failed.
> 
> [How to reproduce]
> mkdir -p /mnt/test/219.mnt
> xfs_io -f -c "truncate 256m" /mnt/test/219.img1
> mkfs.btrfs /mnt/test/219.img1
> cp /mnt/test/219.img1 /mnt/test/219.img2
> mount -o loop /mnt/test/219.img1 /mnt/test/219.mnt
> umount /mnt/test/219.mnt
> losetup -f --show /mnt/test/219.img1 dev
> mount /dev/loop0 /mnt/test/219.mnt
> umount /mnt/test/219.mnt
> mount -o loop /mnt/test/219.img2 /mnt/test/219.mnt
> 
> [Root cause]
> if (fs_devices->opened && found_transid < device->generation) {
> 	/*
> 	 * That is if the FS is _not_ mounted and if you
> 	 * are here, that means there is more than one
> 	 * disk with same uuid and devid.We keep the one
> 	 * with larger generation number or the last-in if
> 	 * generation are equal.
> 	 */
> 	mutex_unlock(&fs_devices->device_list_mutex);
> 	return ERR_PTR(-EEXIST);
> }
> 
> [Personal opinion]
> User might back up a block device to another. I think it is improper
> to forbid user from mounting it.
> 
> Signed-off-by: Flint.Wang <hmsjwzb@zoho.com>


This lacks any explanation whatsoever so it's not possible to judge 
whether the fix is correct or not.

> ---
>   fs/btrfs/volumes.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6aa6bc769569a..76af32032ac85 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -900,7 +900,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   		 * tracking a problem where systems fail mount by subvolume id
>   		 * when we reject replacement on a mounted FS.
>   		 */
> -		if (!fs_devices->opened && found_transid < device->generation) {
> +		if (fs_devices->opened && found_transid < device->generation) {
>   			/*
>   			 * That is if the FS is _not_ mounted and if you
>   			 * are here, that means there is more than one