Re: [PATCH] btrfs: do not skip re-registration for the mounted device

Anand Jain
2024-02-08T17:22:17Z

Thanks for the kernel messages with debug enabled.

I don't see the message to skip scannaing for
the mounted device. So it's not what we thought
was the issue.

pr_debug("BTRFS: skip registering single non-seed device %s\n", path);

Based on the assumption above, I have a fix below,
but I doubt its effectiveness.

https://patchwork.kernel.org/project/linux-btrfs/patch/8dd1990114aabb775d4631969f1beabeadaac5b7.1707132247.git.anand.jain@oracle.com/

-Anand

On 2/8/24 18:05, Alex Romosan wrote:
> i'm attaching my boot log with 6.8.0-rc3 no fixes and btrfs debug
> enabled (i assume this is what you wanted). update-grub doesn't work.
> there was no patch in your last message. do you want me to try the
> patch you sent on monday? confused
>
> On Thu, Feb 8, 2024 at 3:23 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>>
>> Alex,
>>
>> Please provide the kernel boot messages with debugging enabled but
>> no fixes applied. Kindly collect these messages after reproducing
>> the problem.
>>
>> We've found issues with the previous fix. Please test this
>> new patch, as it may address the bug. Keep debugging enabled
>> during testing.
>>
>>
>> Thanks ,Anand
>>
>>
>> On 2/7/24 23:48, Alex Romosan wrote:
>>> Which version of the patch are we talking about? Let me know and I’ll
>>> try it with debugging on. I tried David’s patch and it seemed to work (I
>>> just booted into that kernel and ran update-grub) but I’ll try something
>>> else…
>>>
>>> On Wed, Feb 7, 2024 at 19:08 Anand Jain <anand.jain@oracle.com
>>> <mailto:anand.jain@oracle.com>> wrote:
>>>
>>>
>>>
>>> On 2/7/24 08:08, Anand Jain wrote:
>>> >
>>> >
>>> >
>>> > On 2/5/24 18:27, David Sterba wrote:
>>> >> On Mon, Feb 05, 2024 at 07:45:05PM +0800, Anand Jain wrote:
>>> >>> We skip device registration for a single device. However, we do
>>> not do
>>> >>> that if the device is already mounted, as it might be coming in
>>> again
>>> >>> for scanning a different path.
>>> >>>
>>> >>> This patch is lightly tested; for verification if it fixes.
>>> >>>
>>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com
>>> <mailto:anand.jain@oracle.com>>
>>> >>> ---
>>> >>> I still have some unknowns about the problem. Pls test if this
>>> fixes
>>> >>> the problem.
>>>
>>> Successfully tested with fstests (-g volume) and temp-fsid test cases.
>>>
>>> Can someone verify if this patch fixes the problem? Also, when problem
>>> occurs please provide kernel messages with Btrfs debugging support
>>> option compiled in.
>>>
>>> Thanks, Anand
>>>
>>>
>>> >>>
>>> >>> fs/btrfs/volumes.c | 44
>>> ++++++++++++++++++++++++++++++++++----------
>>> >>> fs/btrfs/volumes.h | 1 -
>>> >>> 2 files changed, 34 insertions(+), 11 deletions(-)
>>> >>>
>>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> >>> index 474ab7ed65ea..192c540a650c 100644
>>> >>> --- a/fs/btrfs/volumes.c
>>> >>> +++ b/fs/btrfs/volumes.c
>>> >>> @@ -1299,6 +1299,31 @@ int btrfs_forget_devices(dev_t devt)
>>> >>> return ret;
>>> >>> }
>>> >>> +static bool btrfs_skip_registration(struct btrfs_super_block
>>> >>> *disk_super,
>>> >>> + dev_t devt, bool mount_arg_dev)
>>> >>> +{
>>> >>> + struct btrfs_fs_devices *fs_devices;
>>> >>> +
>>> >>> + list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
>>> >>> + struct btrfs_device *device;
>>> >>> +
>>> >>> + mutex_lock(&fs_devices->device_list_mutex);
>>> >>> + list_for_each_entry(device, &fs_devices->devices,
>>> dev_list) {
>>> >>> + if (device->devt == devt) {
>>> >>> + mutex_unlock(&fs_devices->device_list_mutex);
>>> >>> + return false;
>>> >>> + }
>>> >>> + }
>>> >>> + mutex_unlock(&fs_devices->device_list_mutex);
>>> >>
>>> >> This is locking and unlocking again before going to
>>> device_list_add, so
>>> >> if something changes regarding the registered device then it's
>>> not up to
>>> >> date.
>>> >>
>>> >
>>>
>>> We are in the uuid_mutex, a potentially racing thread will have to
>>> acquire this mutex to delete from the list. So there can't a race.
>>>
>>>
>>>
>>> > Right. A race might happen, but it is not an issue. At worst, there
>>> > will be a stale device in the cache, which gets removed or re-used
>>> > in the next mkfs or mount of the same device.
>>> >
>>> > However, this is a rough cut that we need to fix. I am reviewing
>>> > your approach as well. I'm fine with any fix.
>>> >
>>> >
>>> >>
>>> >>> + }
>>> >>> +
>>> >>> + if (!mount_arg_dev && btrfs_super_num_devices(disk_super)
>>> == 1 &&
>>> >>> + !(btrfs_super_flags(disk_super) &
>>> BTRFS_SUPER_FLAG_SEEDING))
>>> >>> + return true;
>>> >>
>>> >> The way I implemented it is to check the above conditions as a
>>> >> prerequisite but leave the heavy work for device_list_add that
>>> does all
>>> >> the uuid and device list locking and we are quite sure it
>>> survives all
>>> >> the races between scanning and mounts.
>>> >>
>>> >
>>> > Hm. But isn't that the bug we need to fix? That we skipped the device
>>> > scan thread that wanted to replace the device path from /dev/root to
>>> > /dev/sdx?
>>> >
>>> > And we skipped, because it was not a mount thread
>>> > (%mount_arg_dev=false), and the device is already mounted and the
>>> > devt will match?
>>> >
>>> > So my fix also checked if devt is a match, then allow it to scan
>>> > (so that the device path can be updated, such as /dev/root to
>>> /dev/sdx).
>>> >
>>> > To confirm the bug, I asked for the debug kernel messages, I don't
>>> > this we got it. Also, the existing kernel log shows no such issue.
>>> >
>>> >
>>> >>> +
>>> >>> + return false;
>>> >>> +}
>>> >>> +
>>> >>> /*
>>> >>> * Look for a btrfs signature on a device. This may be called
>>> out
>>> >>> of the mount path
>>> >>> * and we are not allowed to call set_blocksize during the scan.
>>> >>> The superblock
>>> >>> @@ -1316,6 +1341,7 @@ struct btrfs_device
>>> >>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>> >>> struct btrfs_device *device = NULL;
>>> >>> struct bdev_handle *bdev_handle;
>>> >>> u64 bytenr, bytenr_orig;
>>> >>> + dev_t devt = 0;
>>> >>> int ret;
>>> >>> lockdep_assert_held(&uuid_mutex);
>>> >>> @@ -1355,18 +1381,16 @@ struct btrfs_device
>>> >>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>> >>> goto error_bdev_put;
>>> >>> }
>>> >>> - if (!mount_arg_dev && btrfs_super_num_devices(disk_super)
>>> == 1 &&
>>> >>> - !(btrfs_super_flags(disk_super) &
>>> BTRFS_SUPER_FLAG_SEEDING)) {
>>> >>> - dev_t devt;
>>> >>> + ret = lookup_bdev(path, &devt);
>>> >>> + if (ret)
>>> >>> + btrfs_warn(NULL, "lookup bdev failed for path %s: %d",
>>> >>> + path, ret);
>>> >>> - ret = lookup_bdev(path, &devt);
>>> >>
>>> >> Do we actually need this check? It was added with the patch
>>> skipping the
>>> >> registration, so it's validating the block device but how can we
>>> pass
>>> >> something that is not a valid block device?
>>> >>
>>> >
>>> > Do you mean to check if the lookup_bdev() is successful? Hm. It
>>> should
>>> > be okay not to check, but we do that consistently in other places.
>>> >
>>> >> Besides there's a call to bdev_open_by_path() that in turn does the
>>> >> lookup_bdev so checking it here is redundant. It's not related
>>> to the
>>> >> fix itself but I deleted it in my fix.
>>> >>
>>> >
>>> > Oh no. We need %devt to be set because:
>>> >
>>> > It will match if that device is already mounted/scanned.
>>> > It will also free stale entries.
>>> >
>>> > Thx, Anand
>>> >
>>> >>> - if (ret)
>>> >>> - btrfs_warn(NULL, "lookup bdev failed for path %s: %d",
>>> >>> - path, ret);
>>> >>> - else
>>> >>> + if (btrfs_skip_registration(disk_super, devt,
>>> mount_arg_dev)) {
>>> >>> + pr_debug("BTRFS: skip registering single non-seed
>>> device %s\n",
>>> >>> + path);
>>> >>> + if (devt)
>>> >>> btrfs_free_stale_devices(devt, NULL);
>>> >>> -
>>> >>> - pr_debug("BTRFS: skip registering single non-seed device
>>> >>> %s\n", path);
>>> >>> device = NULL;
>>> >>> goto free_disk_super;
>>> >>> }
>>>