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

[Anand Jain <anand.jain@oracle.com>]

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;
>>>       >>>       }
>>>