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: <da885cc8-1f60-d586-b8c7-f3f51f451ada@oracle.com>
Date:   Wed, 3 Aug 2022 14:19:13 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH]btrfs: Fix fstest case btrfs/219
To:     dsterba@suse.cz, hmsjwzb <hmsjwzb@zoho.com>,
        Nikolay Borisov <nborisov@suse.com>, 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>
 <0c2337c3-2b39-c54c-27e9-dd4f0a99cf71@suse.com>
 <1aaff0ac-436e-7782-081c-3549ff3d8c8f@zoho.com>
 <20220726183824.GL13489@twin.jikos.cz>
From:   Anand Jain <anand.jain@oracle.com>
In-Reply-To: <20220726183824.GL13489@twin.jikos.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OVlPRlI0SDlxYTJUd0JjYjMwZXpOK0lRSFJUbHNydUtIdEdwV281UFYrZ2Rq?=
 =?utf-8?B?dGNpQ3RrYzRJUVY4MHVyRFcrV2lWMU43VGc5dzd4UGNiM0NZOExlQXRHSDVQ?=
 =?utf-8?B?a0Vic3o3anF3bmlHVEdIS3JxUXRNWE9IM3ZMRHVwVldPSFpNcFBSUUN4amh3?=
 =?utf-8?B?V2g1SEgrUVZxMWMzeWhVcE1DVHVMOWc2cHNUWG1UZC9hZFV6a2NjR2xZdWt1?=
 =?utf-8?B?eXhwdWdGWUZUMDN4ZlY1dC9tOFpnamdBR2lUdFhFbVk2NFhTVXk1RExCSmc4?=
 =?utf-8?B?SlZNbnBWaTF1QXRJcElCR2k2Y2o2NmNLR0tCWllwYmtvZDBWc21TQnd2bm5O?=
 =?utf-8?B?cEQzVkllS0dwbFFnS2N5T20wMlN3ZGJNaXAybVpiNVg3b2hCcklxNHBsck5P?=
 =?utf-8?B?aENQMENaTmhKYWNPWEUvcXVodVZVVzd5ZkJLV1FiR3dHbzRaYzBKbllrMWRm?=
 =?utf-8?B?YlM1a0JraUtENzluakpiZjZudlNULzllWXp5cFlYb1pNOXN0bFc3aElFZEJJ?=
 =?utf-8?B?ZTR0SEVaMUlRQ0hvc1VmdWl4eHlFamZRNXFLMEFjcTRqY2RldXBIMXdNd29W?=
 =?utf-8?B?bUpkdTJGL2tXSlgzaU02cFZyMkNqL3gwNllmRGJENUhya0xzOFF0ZVFuOVpx?=
 =?utf-8?B?NXlPU2FuL2RPc0Eydm5lU1M2YjJETnlTL0FRRmJQN0JZMk1xL1dwdWprNHZR?=
 =?utf-8?B?Vm90SEs1SUNiT3RoaDJkQW0vU2ovQjRiRU5MQkczKzhmS3FXdlRRNW0vR0ty?=
 =?utf-8?B?dXBlRUpDYlRoTTlXT0xURXliY0xLNnRpU0lEcWVwUFVldm8vbkpEWXIwVmpT?=
 =?utf-8?B?Y2FpbHA1RTk2Tkl0TmJTeGlPZGxuU2lSd2VZTU9pTVhOVWVDR0ZzR2htS3dv?=
 =?utf-8?B?b2lTeXJiSVNPWVpkd1h2YzZrMWJtOElVMGZRa3EyUXFaa2pXYkFFTG1RODlU?=
 =?utf-8?B?WTZPUVdSM1ZPWnpSK0cxNnZEdzR3MVJvbWZuSzNidzdRK0s3Z1dWODZCMUpu?=
 =?utf-8?B?a0hwOU1icXEwZ2RIdHU1d3NiS1h4WE5xK0RrSVFmU1pwd0YyQWZwMWYrUndI?=
 =?utf-8?B?WTBDbkZvamNNdndSb3NtMVZZQmVaT0hPUW5jU3hnTWVvdVE4V0dSS1NsSjNP?=
 =?utf-8?B?UHZXVU9pdHByTXBkcVBCeHVwK3dnR3g3NW0ycG1KSlQxa0c4VlBpUTNvWGl1?=
 =?utf-8?B?N0twd3BucWgrTjlxQjFyb0lIZHdvWkZaYjJ6bi95T1pqWENvbWZ1RDlCa0hj?=
 =?utf-8?B?dVRYdGgrN240Nkp4N1NXRWpPMXFwaktHb2ZoMDZhcERtcW02Z1lZT3pFcUpN?=
 =?utf-8?B?YVRPTDQ1ZTYxdnphdXVyeW9VTlNDRDhvMGNwdTdLaDhZMmdrN2JBMUJ5UXpP?=
 =?utf-8?B?cXNnRlNNbXp0U1lSc3h0WGZXNFF4YytISnJBVW1OMndsZkcyc1dScG0yZk9z?=
 =?utf-8?B?aElFZmVOTndleXg4a3NhaGhwcXNhVndkTnVtNEZBYlZRNmY4NVNGWDNERWhq?=
 =?utf-8?B?QzdBVXlTUlhHM1lHb2JlaDNGeG8zRy9DbVJOSlRpUXNqaHNabjB5MGM1blk4?=
 =?utf-8?B?Wk0xR0pzM1VFUWxXY3FvZVdlMGQ3NTJUZHE0UlNDQXI1YVEzRUNKNDdrTzZu?=
 =?utf-8?B?MnRSa0JkWURoeW9qWVpUQUZLTFh0Z3hWcWM2SHozQ3RMdTBzMS80UkQyb0hI?=
 =?utf-8?B?Z0R1cGVEanFUMndDZmtmZ1dLSHJyRmFnbEV1Tk1HOWVScDJoVXNDRFBrK0c4?=
 =?utf-8?B?TnBnZk5UVU80MHdiYSsyTlYxR1hwS2hsMjhZcU5pcTRrdUxZbjBsS0h0VVRp?=
 =?utf-8?B?aG1xdEwzSzA3M0VSeDZpR3R2WGxnbTVhMlRTcXRUaHJUZXAzc0o3WmJVV2tI?=
 =?utf-8?B?RjBTMmwvbnFibVlFWEtFQ3JCWDlsSGNVemZoQi9Ca1FjbWVXL0dWckNnblUx?=
 =?utf-8?B?czRKY1JqRE05Q1FzcGhjRFdxS2dOb0FDdnFTREd4Ujd1emZveFY3Ky9oK1JJ?=
 =?utf-8?B?MWk5c1FET0VZRVoxZFhPNFM1UG0zR2RoR3hDb0I5QWVjd3Fvdm4yTG9CcGNP?=
 =?utf-8?B?aGxKUm12enNWanZKek0zaGFBUk9SdktiU2tVR1dGNFRaNUY3QWlxUzNSQjBw?=
 =?utf-8?Q?HhiGzBhOfi5YyNrNF4iJ10S3M?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 87fadadd-83f3-4f5c-89df-08da75181a31
X-MS-Exchange-CrossTenant-AuthSource: PH0PR10MB5706.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Aug 2022 06:19:20.9204
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: yUZmJ3P3Pb42NMkVpKOy5wrN4jDXXMR+zHn9qhcwCO3q3h9U14pekDZs3WRc6j67oWc4h7xEE7L9rP7uNJCE7w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4733
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-08-03_03,2022-08-02_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxscore=0 suspectscore=0
 adultscore=0 bulkscore=0 malwarescore=0 mlxlogscore=999 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2208030030
X-Proofpoint-GUID: Dfc2cWE1THp1u2pC78ySQuOtCC1ap9l0
X-Proofpoint-ORIG-GUID: Dfc2cWE1THp1u2pC78ySQuOtCC1ap9l0
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 27/07/2022 02:38, David Sterba wrote:
> On Fri, Jul 22, 2022 at 01:34:11AM -0400, hmsjwzb wrote:
>> On 7/21/22 09:37, Nikolay Borisov wrote:
>>> 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
>>
>> Hi Nikolay,
>>
>> It seems the failure of btrfs/219 needs some explanation.
>>
>> Here is the thing.
>>          1. A storage device A with btrfs filesystem is running on a host.
>>          2. For example, we backup the device A to an exactly some device B.
>>          3. The device A continue to run for a while so the device->generation is getting bigger.
>>          4. Then you umount the device A and try to mount device B.
>>          5. Kernel find that device A has the same UUID as device B and has bigger device->generation.
>>             So the mount request of device B will be rejected.
> 
> That's on purpose, devices are matched by UUIDs and making block copies
> of the same filesystem is known "don't do that" and discouraged.
> 
> If you must store the block copies then you can change the UUID by
> btrfstune, there are two ways (fast metadata_uuid, and slow rewriting
> all metadata uuids in all blocks).
> 
>>
>>              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);
>>              }
>>
>> I think it is improper to reject that request. Because device A is not in open state.
> 
> But this would prevent mounting A. There should really be some way to
> distiguish the filesystems, the block device is not a stable identifier,
> the UUID is. Imagine having 10 copies of the same filesystem identified
> by the same UUID and device UUID, but with different generations and
> data. That's asking for problems.
> 
> There's not much the filesystem driver can do than to avoid using old
> devices and giving preference to the highest generation device. All
> devices with btrfs signature are registered in memory and this is the
> primary source when mounting the devices, not the block device itself.


David,

  The unintegrated patch [1] also used the same use case.

   [1]
     [PATCH v2][RESEND] btrfs: allow single disk devices to mount with 
older generations

  IMO device-copy and mount (without changing the UUID) can be allowed
  for a single device btrfs volume only. We even have a fstest case
  btrfs/219, which tests single device duplicate UUIDs.

  Please integrate [1].

-Anand