| Message ID | 8dd1990114aabb775d4631969f1beabeadaac5b7.1707132247.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: do not skip re-registration for the mounted device | expand |
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> > --- > I still have some unknowns about the problem. Pls test if this fixes > the problem. > > 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. > + } > + > + 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. > + > + 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? 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. > - 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; > }
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> >> --- >> I still have some unknowns about the problem. Pls test if this fixes >> the problem. >> >> 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. > 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; >> }
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> >>> --- >>> 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; >>> }
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; > >>> } >
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; > > >>> } > >
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;
>>> >>> }
>>>
okay, so this is the original patch from Monday. I tried the version David committed to to the btrfs tree and that fixed the problem but I'll try this one as well (with debugging now enabled) and let you know. On Thu, Feb 8, 2024 at 6:22 PM Anand Jain <anand.jain@oracle.com> wrote: > > > 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; > >>> >>> } > >>>
i'm attaching the boot log from 6.8-rc3 with your patch. update-grub works. i took a quick look at the log and i can see this (which wasn't in the unpatched kernel): BTRFS info: devid 1 device path /dev/root changed to /dev/nvme0n1p3 scanned by (udev-worker) On Thu, Feb 8, 2024 at 6:22 PM Anand Jain <anand.jain@oracle.com> wrote: > > > 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; > >>> >>> } > >>>
On 2/9/24 01:34, Alex Romosan wrote: > i'm attaching the boot log from 6.8-rc3 with your patch. update-grub > works. i took a quick look at the log and i can see this (which wasn't > in the unpatched kernel): > > BTRFS info: devid 1 device path /dev/root changed to /dev/nvme0n1p3 > scanned by (udev-worker) > That's nice. Thanks for verifying. Anand > On Thu, Feb 8, 2024 at 6:22 PM Anand Jain <anand.jain@oracle.com> wrote: >> >> >> 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; >>>>> >>> } >>>>>
QW0gRnJlaXRhZywgZGVtIDA5LjAyLjIwMjQgdW0gMTM6NDEgKzA1MzAgc2NocmllYiBBbmFuZCBK YWluOgo+IAo+IAo+IE9uIDIvOS8yNCAwMTozNCwgQWxleCBSb21vc2FuIHdyb3RlOgo+ID4gaSdt IGF0dGFjaGluZyB0aGUgYm9vdCBsb2cgZnJvbSA2LjgtcmMzIHdpdGggeW91ciBwYXRjaC4gdXBk YXRlLQo+ID4gZ3J1Ygo+ID4gd29ya3MuIGkgdG9vayBhIHF1aWNrIGxvb2sgYXQgdGhlIGxvZyBh bmQgaSBjYW4gc2VlIHRoaXMgKHdoaWNoCj4gPiB3YXNuJ3QKPiA+IGluIHRoZSB1bnBhdGNoZWQg a2VybmVsKToKPiA+IAo+ID4gQlRSRlMgaW5mbzogZGV2aWQgMSBkZXZpY2UgcGF0aCAvZGV2L3Jv b3QgY2hhbmdlZCB0byAvZGV2L252bWUwbjFwMwo+ID4gc2Nhbm5lZCBieSAodWRldi13b3JrZXIp Cj4gPiAKPiAKPiDCoCBUaGF0J3MgbmljZS4gVGhhbmtzIGZvciB2ZXJpZnlpbmcuCj4gCgpUZXN0 ZWQgaGVyZSBhcyB3ZWxsLCBmaXhlcyB0aGUgcHJvYmxlbSBmb3IgbWUgdG9vLgpUaGFua3MhCkJl cm5kCgoKPiBBbmFuZAo+IAo+IAo+ID4gT24gVGh1LCBGZWIgOCwgMjAyNCBhdCA2OjIy4oCvUE0g QW5hbmQgSmFpbiA8YW5hbmQuamFpbkBvcmFjbGUuY29tPgo+ID4gd3JvdGU6Cj4gPiA+IAo+ID4g PiAKPiA+ID4gVGhhbmtzIGZvciB0aGUga2VybmVsIG1lc3NhZ2VzIHdpdGggZGVidWcgZW5hYmxl ZC4KPiA+ID4gCj4gPiA+IEkgZG9uJ3Qgc2VlIHRoZSBtZXNzYWdlIHRvIHNraXAgc2Nhbm5haW5n IGZvcgo+ID4gPiB0aGUgbW91bnRlZCBkZXZpY2UuIFNvIGl0J3Mgbm90IHdoYXQgd2UgdGhvdWdo dAo+ID4gPiB3YXMgdGhlIGlzc3VlLgo+ID4gPiAKPiA+ID4gwqDCoMKgIHByX2RlYnVnKCJCVFJG Uzogc2tpcCByZWdpc3RlcmluZyBzaW5nbGUgbm9uLXNlZWQgZGV2aWNlCj4gPiA+ICVzXG4iLCBw YXRoKTsKPiA+ID4gCj4gPiA+IAo+ID4gPiBCYXNlZCBvbiB0aGUgYXNzdW1wdGlvbiBhYm92ZSwg SSBoYXZlIGEgZml4IGJlbG93LAo+ID4gPiBidXQgSSBkb3VidCBpdHMgZWZmZWN0aXZlbmVzcy4K PiA+ID4gCj4gPiA+IAo+ID4gPiBodHRwczovL3BhdGNod29yay5rZXJuZWwub3JnL3Byb2plY3Qv bGludXgtYnRyZnMvcGF0Y2gvOGRkMTk5MDExNGFhYmI3NzVkNDYzMTk2OWYxYmVhYmVhZGFhYzVi Ny4xNzA3MTMyMjQ3LmdpdC5hbmFuZC5qYWluQG9yYWNsZS5jb20vCj4gPiA+IAo+ID4gPiAtQW5h bmQKPiA+ID4gCj4gPiA+IAo+ID4gPiBPbiAyLzgvMjQgMTg6MDUsIEFsZXggUm9tb3NhbiB3cm90 ZToKPiA+ID4gPiBpJ20gYXR0YWNoaW5nIG15IGJvb3QgbG9nIHdpdGggNi44LjAtcmMzIG5vIGZp eGVzIGFuZCBidHJmcwo+ID4gPiA+IGRlYnVnCj4gPiA+ID4gZW5hYmxlZCAoaSBhc3N1bWUgdGhp cyBpcyB3aGF0IHlvdSB3YW50ZWQpLiB1cGRhdGUtZ3J1YiBkb2Vzbid0Cj4gPiA+ID4gd29yay4K PiA+ID4gPiB0aGVyZSB3YXMgbm8gcGF0Y2ggaW4geW91ciBsYXN0IG1lc3NhZ2UuIGRvIHlvdSB3 YW50IG1lIHRvIHRyeQo+ID4gPiA+IHRoZQo+ID4gPiA+IHBhdGNoIHlvdSBzZW50IG9uIG1vbmRh eT8gY29uZnVzZWQKPiA+ID4gPiAKPiA+ID4gPiBPbiBUaHUsIEZlYiA4LCAyMDI0IGF0IDM6MjPi gK9BTSBBbmFuZCBKYWluCj4gPiA+ID4gPGFuYW5kLmphaW5Ab3JhY2xlLmNvbT4gd3JvdGU6Cj4g PiA+ID4gPiAKPiA+ID4gPiA+IAo+ID4gPiA+ID4gQWxleCwKPiA+ID4gPiA+IAo+ID4gPiA+ID4g UGxlYXNlIHByb3ZpZGUgdGhlIGtlcm5lbCBib290IG1lc3NhZ2VzIHdpdGggZGVidWdnaW5nCj4g PiA+ID4gPiBlbmFibGVkIGJ1dAo+ID4gPiA+ID4gbm8gZml4ZXMgYXBwbGllZC4gS2luZGx5IGNv bGxlY3QgdGhlc2UgbWVzc2FnZXMgYWZ0ZXIKPiA+ID4gPiA+IHJlcHJvZHVjaW5nCj4gPiA+ID4g PiB0aGUgcHJvYmxlbS4KPiA+ID4gPiA+IAo+ID4gPiA+ID4gV2UndmUgZm91bmQgaXNzdWVzIHdp dGggdGhlIHByZXZpb3VzIGZpeC4gUGxlYXNlIHRlc3QgdGhpcwo+ID4gPiA+ID4gbmV3IHBhdGNo LCBhcyBpdCBtYXkgYWRkcmVzcyB0aGUgYnVnLiBLZWVwIGRlYnVnZ2luZyBlbmFibGVkCj4gPiA+ ID4gPiBkdXJpbmcgdGVzdGluZy4KPiA+ID4gPiA+IAo+ID4gPiA+ID4gCj4gPiA+ID4gPiBUaGFu a3MgLEFuYW5kCj4gPiA+ID4gPiAKPiA+ID4gPiA+IAo+ID4gPiA+ID4gT24gMi83LzI0IDIzOjQ4 LCBBbGV4IFJvbW9zYW4gd3JvdGU6Cj4gPiA+ID4gPiA+IFdoaWNoIHZlcnNpb24gb2YgdGhlIHBh dGNoIGFyZSB3ZSB0YWxraW5nIGFib3V0PyBMZXQgbWUKPiA+ID4gPiA+ID4ga25vdyBhbmQgSeKA mWxsCj4gPiA+ID4gPiA+IHRyeSBpdCB3aXRoIGRlYnVnZ2luZyBvbi4gSSB0cmllZCBEYXZpZOKA mXMgcGF0Y2ggYW5kIGl0Cj4gPiA+ID4gPiA+IHNlZW1lZCB0byB3b3JrIChJCj4gPiA+ID4gPiA+ IGp1c3QgYm9vdGVkIGludG8gdGhhdCBrZXJuZWwgYW5kIHJhbiB1cGRhdGUtZ3J1YikgYnV0IEni gJlsbAo+ID4gPiA+ID4gPiB0cnkgc29tZXRoaW5nCj4gPiA+ID4gPiA+IGVsc2XigKYKPiA+ID4g PiA+ID4gCj4gPiA+ID4gPiA+IE9uIFdlZCwgRmViIDcsIDIwMjQgYXQgMTk6MDggQW5hbmQgSmFp bgo+ID4gPiA+ID4gPiA8YW5hbmQuamFpbkBvcmFjbGUuY29tCj4gPiA+ID4gPiA+IDxtYWlsdG86 YW5hbmQuamFpbkBvcmFjbGUuY29tPj4gd3JvdGU6Cj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiAK PiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqAgT24gMi83LzI0IDA4OjA4LCBBbmFu ZCBKYWluIHdyb3RlOgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPgo+ID4gPiA+ID4gPiDCoMKg wqDCoMKgwqAgPgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPgo+ID4gPiA+ID4gPiDCoMKgwqDC oMKgwqAgPiBPbiAyLzUvMjQgMTg6MjcsIERhdmlkIFN0ZXJiYSB3cm90ZToKPiA+ID4gPiA+ID4g wqDCoMKgwqDCoMKgID4+IE9uIE1vbiwgRmViIDA1LCAyMDI0IGF0IDA3OjQ1OjA1UE0gKzA4MDAs IEFuYW5kCj4gPiA+ID4gPiA+IEphaW4gd3JvdGU6Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+ Pj4gV2Ugc2tpcCBkZXZpY2UgcmVnaXN0cmF0aW9uIGZvciBhIHNpbmdsZSBkZXZpY2UuCj4gPiA+ ID4gPiA+IEhvd2V2ZXIsIHdlIGRvCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqAgbm90IGRvCj4gPiA+ ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4gdGhhdCBpZiB0aGUgZGV2aWNlIGlzIGFscmVhZHkgbW91 bnRlZCwgYXMgaXQKPiA+ID4gPiA+ID4gbWlnaHQgYmUgY29taW5nIGluCj4gPiA+ID4gPiA+IMKg wqDCoMKgwqAgYWdhaW4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PiBmb3Igc2Nhbm5pbmcg YSBkaWZmZXJlbnQgcGF0aC4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+Pgo+ID4gPiA+ID4g PiDCoMKgwqDCoMKgwqAgPj4+IFRoaXMgcGF0Y2ggaXMgbGlnaHRseSB0ZXN0ZWQ7IGZvciB2ZXJp ZmljYXRpb24KPiA+ID4gPiA+ID4gaWYgaXQgZml4ZXMuCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDC oCA+Pj4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PiBTaWduZWQtb2ZmLWJ5OiBBbmFuZCBK YWluIDxhbmFuZC5qYWluQG9yYWNsZS5jb20KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoCA8bWFpbHRv OmFuYW5kLmphaW5Ab3JhY2xlLmNvbT4+Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4gLS0t Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4gSSBzdGlsbCBoYXZlIHNvbWUgdW5rbm93bnMg YWJvdXQgdGhlIHByb2JsZW0uCj4gPiA+ID4gPiA+IFBscyB0ZXN0IGlmIHRoaXMKPiA+ID4gPiA+ ID4gwqDCoMKgwqDCoCBmaXhlcwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+IHRoZSBwcm9i bGVtLgo+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoCBTdWNjZXNzZnVsbHkgdGVz dGVkIHdpdGggZnN0ZXN0cyAoLWcgdm9sdW1lKSBhbmQKPiA+ID4gPiA+ID4gdGVtcC1mc2lkIHRl c3QgY2FzZXMuCj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgIENhbiBzb21lb25l IHZlcmlmeSBpZiB0aGlzIHBhdGNoIGZpeGVzIHRoZSBwcm9ibGVtPwo+ID4gPiA+ID4gPiBBbHNv LCB3aGVuIHByb2JsZW0KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoCBvY2N1cnMgcGxlYXNlIHByb3Zp ZGUga2VybmVsIG1lc3NhZ2VzIHdpdGggQnRyZnMKPiA+ID4gPiA+ID4gZGVidWdnaW5nIHN1cHBv cnQKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoCBvcHRpb24gY29tcGlsZWQgaW4uCj4gPiA+ID4gPiA+ IAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgIFRoYW5rcywgQW5hbmQKPiA+ID4gPiA+ID4gCj4gPiA+ ID4gPiA+IAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+Cj4gPiA+ID4gPiA+IMKgwqDCoMKg wqDCoCA+Pj7CoMKgIGZzL2J0cmZzL3ZvbHVtZXMuYyB8IDQ0Cj4gPiA+ID4gPiA+IMKgwqDCoMKg wqAgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0tLS0tLS0tLS0KPiA+ID4gPiA+ ID4gwqDCoMKgwqDCoMKgID4+PsKgwqAgZnMvYnRyZnMvdm9sdW1lcy5oIHzCoCAxIC0KPiA+ID4g PiA+ID4gwqDCoMKgwqDCoMKgID4+PsKgwqAgMiBmaWxlcyBjaGFuZ2VkLCAzNCBpbnNlcnRpb25z KCspLCAxMQo+ID4gPiA+ID4gPiBkZWxldGlvbnMoLSkKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKg ID4+Pgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+IGRpZmYgLS1naXQgYS9mcy9idHJmcy92 b2x1bWVzLmMKPiA+ID4gPiA+ID4gYi9mcy9idHJmcy92b2x1bWVzLmMKPiA+ID4gPiA+ID4gwqDC oMKgwqDCoMKgID4+PiBpbmRleCA0NzRhYjdlZDY1ZWEuLjE5MmM1NDBhNjUwYyAxMDA2NDQKPiA+ ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PiAtLS0gYS9mcy9idHJmcy92b2x1bWVzLmMKPiA+ID4g PiA+ID4gwqDCoMKgwqDCoMKgID4+PiArKysgYi9mcy9idHJmcy92b2x1bWVzLmMKPiA+ID4gPiA+ ID4gwqDCoMKgwqDCoMKgID4+PiBAQCAtMTI5OSw2ICsxMjk5LDMxIEBAIGludAo+ID4gPiA+ID4g PiBidHJmc19mb3JnZXRfZGV2aWNlcyhkZXZfdCBkZXZ0KQo+ID4gPiA+ID4gPiDCoMKgwqDCoMKg wqAgPj4+wqDCoMKgwqDCoMKgIHJldHVybiByZXQ7Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+ Pj7CoMKgIH0KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PiArc3RhdGljIGJvb2wgYnRyZnNf c2tpcF9yZWdpc3RyYXRpb24oc3RydWN0Cj4gPiA+ID4gPiA+IGJ0cmZzX3N1cGVyX2Jsb2NrCj4g PiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4gKmRpc2tfc3VwZXIsCj4gPiA+ID4gPiA+IMKgwqDC oMKgwqDCoCA+Pj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIGRldl90 IGRldnQsIGJvb2wKPiA+ID4gPiA+ID4gbW91bnRfYXJnX2RldikKPiA+ID4gPiA+ID4gwqDCoMKg wqDCoMKgID4+PiArewo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICvCoMKgwqAgc3RydWN0 IGJ0cmZzX2ZzX2RldmljZXMgKmZzX2RldmljZXM7Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+ Pj4gKwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICvCoMKgwqAgbGlzdF9mb3JfZWFjaF9l bnRyeShmc19kZXZpY2VzLCAmZnNfdXVpZHMsCj4gPiA+ID4gPiA+IGZzX2xpc3QpIHsKPiA+ID4g PiA+ID4gwqDCoMKgwqDCoMKgID4+PiArwqDCoMKgwqDCoMKgwqAgc3RydWN0IGJ0cmZzX2Rldmlj ZSAqZGV2aWNlOwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICsKPiA+ID4gPiA+ID4gwqDC oMKgwqDCoMKgID4+PiArwqDCoMKgwqDCoMKgwqAgbXV0ZXhfbG9jaygmZnNfZGV2aWNlcy0KPiA+ ID4gPiA+ID4gPmRldmljZV9saXN0X211dGV4KTsKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+ PiArwqDCoMKgwqDCoMKgwqAgbGlzdF9mb3JfZWFjaF9lbnRyeShkZXZpY2UsCj4gPiA+ID4gPiA+ ICZmc19kZXZpY2VzLT5kZXZpY2VzLAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgIGRldl9saXN0KSB7 Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqAgaWYg KGRldmljZS0+ZGV2dCA9PSBkZXZ0KSB7Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4gK8Kg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBtdXRleF91bmxvY2soJmZzX2RldmljZXMtCj4g PiA+ID4gPiA+ID5kZXZpY2VfbGlzdF9tdXRleCk7Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+ Pj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCByZXR1cm4gZmFsc2U7Cj4gPiA+ID4g PiA+IMKgwqDCoMKgwqDCoCA+Pj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgwqAgfQo+ID4gPiA+ID4g PiDCoMKgwqDCoMKgwqAgPj4+ICvCoMKgwqDCoMKgwqDCoCB9Cj4gPiA+ID4gPiA+IMKgwqDCoMKg wqDCoCA+Pj4gK8KgwqDCoMKgwqDCoMKgIG11dGV4X3VubG9jaygmZnNfZGV2aWNlcy0KPiA+ID4g PiA+ID4gPmRldmljZV9saXN0X211dGV4KTsKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+Cj4g PiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+PiBUaGlzIGlzIGxvY2tpbmcgYW5kIHVubG9ja2luZyBh Z2FpbiBiZWZvcmUgZ29pbmcKPiA+ID4gPiA+ID4gdG8KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoCBk ZXZpY2VfbGlzdF9hZGQsIHNvCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+PiBpZiBzb21ldGhp bmcgY2hhbmdlcyByZWdhcmRpbmcgdGhlIHJlZ2lzdGVyZWQKPiA+ID4gPiA+ID4gZGV2aWNlIHRo ZW4gaXQncwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgIG5vdCB1cCB0bwo+ID4gPiA+ID4gPiDCoMKg wqDCoMKgwqAgPj4gZGF0ZS4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+Cj4gPiA+ID4gPiA+ IMKgwqDCoMKgwqDCoCA+Cj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgIFdlIGFy ZSBpbiB0aGUgdXVpZF9tdXRleCwgYSBwb3RlbnRpYWxseSByYWNpbmcgdGhyZWFkCj4gPiA+ID4g PiA+IHdpbGwgaGF2ZSB0bwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgIGFjcXVpcmUgdGhpcyBtdXRl eCB0byBkZWxldGUgZnJvbSB0aGUgbGlzdC4gU28gdGhlcmUKPiA+ID4gPiA+ID4gY2FuJ3QgYSBy YWNlLgo+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiDC oMKgwqDCoMKgwqAgPiBSaWdodC4gQSByYWNlIG1pZ2h0IGhhcHBlbiwgYnV0IGl0IGlzIG5vdCBh bgo+ID4gPiA+ID4gPiBpc3N1ZS4gQXQgd29yc3QsIHRoZXJlCj4gPiA+ID4gPiA+IMKgwqDCoMKg wqDCoCA+IHdpbGwgYmUgYSBzdGFsZSBkZXZpY2UgaW4gdGhlIGNhY2hlLCB3aGljaCBnZXRzCj4g PiA+ID4gPiA+IHJlbW92ZWQgb3IgcmUtdXNlZAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPiBp biB0aGUgbmV4dCBta2ZzIG9yIG1vdW50IG9mIHRoZSBzYW1lIGRldmljZS4KPiA+ID4gPiA+ID4g wqDCoMKgwqDCoMKgID4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4gSG93ZXZlciwgdGhpcyBp cyBhIHJvdWdoIGN1dCB0aGF0IHdlIG5lZWQgdG8gZml4Lgo+ID4gPiA+ID4gPiBJIGFtIHJldmll d2luZwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPiB5b3VyIGFwcHJvYWNoIGFzIHdlbGwuIEkn bSBmaW5lIHdpdGggYW55IGZpeC4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4KPiA+ID4gPiA+ ID4gwqDCoMKgwqDCoMKgID4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+Cj4gPiA+ID4gPiA+ IMKgwqDCoMKgwqDCoCA+Pj4gK8KgwqDCoCB9Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4g Kwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICvCoMKgwqAgaWYgKCFtb3VudF9hcmdfZGV2 ICYmCj4gPiA+ID4gPiA+IGJ0cmZzX3N1cGVyX251bV9kZXZpY2VzKGRpc2tfc3VwZXIpCj4gPiA+ ID4gPiA+IMKgwqDCoMKgwqAgPT0gMSAmJgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICvC oMKgwqDCoMKgwqDCoCAhKGJ0cmZzX3N1cGVyX2ZsYWdzKGRpc2tfc3VwZXIpICYKPiA+ID4gPiA+ ID4gwqDCoMKgwqDCoCBCVFJGU19TVVBFUl9GTEFHX1NFRURJTkcpKQo+ID4gPiA+ID4gPiDCoMKg wqDCoMKgwqAgPj4+ICvCoMKgwqDCoMKgwqDCoCByZXR1cm4gdHJ1ZTsKPiA+ID4gPiA+ID4gwqDC oMKgwqDCoMKgID4+Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+PiBUaGUgd2F5IEkgaW1wbGVt ZW50ZWQgaXQgaXMgdG8gY2hlY2sgdGhlIGFib3ZlCj4gPiA+ID4gPiA+IGNvbmRpdGlvbnMgYXMg YQo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4gcHJlcmVxdWlzaXRlIGJ1dCBsZWF2ZSB0aGUg aGVhdnkgd29yayBmb3IKPiA+ID4gPiA+ID4gZGV2aWNlX2xpc3RfYWRkIHRoYXQKPiA+ID4gPiA+ ID4gwqDCoMKgwqDCoCBkb2VzIGFsbAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4gdGhlIHV1 aWQgYW5kIGRldmljZSBsaXN0IGxvY2tpbmcgYW5kIHdlIGFyZSBxdWl0ZQo+ID4gPiA+ID4gPiBz dXJlIGl0Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqAgc3Vydml2ZXMgYWxsCj4gPiA+ID4gPiA+IMKg wqDCoMKgwqDCoCA+PiB0aGUgcmFjZXMgYmV0d2VlbiBzY2FubmluZyBhbmQgbW91bnRzLgo+ID4g PiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4KPiA+ID4g PiA+ID4gwqDCoMKgwqDCoMKgID4gSG0uIEJ1dCBpc24ndCB0aGF0IHRoZSBidWcgd2UgbmVlZCB0 byBmaXg/IFRoYXQgd2UKPiA+ID4gPiA+ID4gc2tpcHBlZCB0aGUgZGV2aWNlCj4gPiA+ID4gPiA+ IMKgwqDCoMKgwqDCoCA+IHNjYW4gdGhyZWFkIHRoYXQgd2FudGVkIHRvIHJlcGxhY2UgdGhlIGRl dmljZSBwYXRoCj4gPiA+ID4gPiA+IGZyb20gL2Rldi9yb290IHRvCj4gPiA+ID4gPiA+IMKgwqDC oMKgwqDCoCA+IC9kZXYvc2R4Pwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPgo+ID4gPiA+ID4g PiDCoMKgwqDCoMKgwqAgPiBBbmQgd2Ugc2tpcHBlZCwgYmVjYXVzZSBpdCB3YXMgbm90IGEgbW91 bnQgdGhyZWFkCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+ICglbW91bnRfYXJnX2Rldj1mYWxz ZSksIGFuZCB0aGUgZGV2aWNlIGlzIGFscmVhZHkKPiA+ID4gPiA+ID4gbW91bnRlZCBhbmQgdGhl Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+IGRldnQgd2lsbCBtYXRjaD8KPiA+ID4gPiA+ID4g wqDCoMKgwqDCoMKgID4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4gU28gbXkgZml4IGFsc28g Y2hlY2tlZCBpZiBkZXZ0IGlzIGEgbWF0Y2gsIHRoZW4KPiA+ID4gPiA+ID4gYWxsb3cgaXQgdG8g c2Nhbgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPiAoc28gdGhhdCB0aGUgZGV2aWNlIHBhdGgg Y2FuIGJlIHVwZGF0ZWQsIHN1Y2ggYXMKPiA+ID4gPiA+ID4gL2Rldi9yb290IHRvCj4gPiA+ID4g PiA+IMKgwqDCoMKgwqAgL2Rldi9zZHgpLgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPgo+ID4g PiA+ID4gPiDCoMKgwqDCoMKgwqAgPiBUbyBjb25maXJtIHRoZSBidWcsIEkgYXNrZWQgZm9yIHRo ZSBkZWJ1ZyBrZXJuZWwKPiA+ID4gPiA+ID4gbWVzc2FnZXMsIEkgZG9uJ3QKPiA+ID4gPiA+ID4g wqDCoMKgwqDCoMKgID4gdGhpcyB3ZSBnb3QgaXQuIEFsc28sIHRoZSBleGlzdGluZyBrZXJuZWwg bG9nCj4gPiA+ID4gPiA+IHNob3dzIG5vIHN1Y2ggaXNzdWUuCj4gPiA+ID4gPiA+IMKgwqDCoMKg wqDCoCA+Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDC oCA+Pj4gKwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICvCoMKgwqAgcmV0dXJuIGZhbHNl Owo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICt9Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDC oCA+Pj4gKwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+wqDCoCAvKgo+ID4gPiA+ID4gPiDC oMKgwqDCoMKgwqAgPj4+wqDCoMKgICogTG9vayBmb3IgYSBidHJmcyBzaWduYXR1cmUgb24gYSBk ZXZpY2UuCj4gPiA+ID4gPiA+IFRoaXMgbWF5IGJlIGNhbGxlZAo+ID4gPiA+ID4gPiDCoMKgwqDC oMKgIG91dAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+IG9mIHRoZSBtb3VudCBwYXRoCj4g PiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj7CoMKgwqAgKiBhbmQgd2UgYXJlIG5vdCBhbGxvd2Vk IHRvIGNhbGwKPiA+ID4gPiA+ID4gc2V0X2Jsb2Nrc2l6ZSBkdXJpbmcgdGhlIHNjYW4uCj4gPiA+ ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4gVGhlIHN1cGVyYmxvY2sKPiA+ID4gPiA+ID4gwqDCoMKg wqDCoMKgID4+PiBAQCAtMTMxNiw2ICsxMzQxLDcgQEAgc3RydWN0IGJ0cmZzX2RldmljZQo+ID4g PiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICpidHJmc19zY2FuX29uZV9kZXZpY2UoY29uc3QgY2hh ciAqcGF0aCwKPiA+ID4gPiA+ID4gYmxrX21vZGVfdCBmbGFncywKPiA+ID4gPiA+ID4gwqDCoMKg wqDCoMKgID4+PsKgwqDCoMKgwqDCoCBzdHJ1Y3QgYnRyZnNfZGV2aWNlICpkZXZpY2UgPSBOVUxM Owo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+wqDCoMKgwqDCoMKgIHN0cnVjdCBiZGV2X2hh bmRsZSAqYmRldl9oYW5kbGU7Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj7CoMKgwqDCoMKg wqAgdTY0IGJ5dGVuciwgYnl0ZW5yX29yaWc7Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4g K8KgwqDCoCBkZXZfdCBkZXZ0ID0gMDsKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PsKgwqDC oMKgwqDCoCBpbnQgcmV0Owo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+wqDCoMKgwqDCoMKg IGxvY2tkZXBfYXNzZXJ0X2hlbGQoJnV1aWRfbXV0ZXgpOwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKg wqAgPj4+IEBAIC0xMzU1LDE4ICsxMzgxLDE2IEBAIHN0cnVjdCBidHJmc19kZXZpY2UKPiA+ID4g PiA+ID4gwqDCoMKgwqDCoMKgID4+PiAqYnRyZnNfc2Nhbl9vbmVfZGV2aWNlKGNvbnN0IGNoYXIg KnBhdGgsCj4gPiA+ID4gPiA+IGJsa19tb2RlX3QgZmxhZ3MsCj4gPiA+ID4gPiA+IMKgwqDCoMKg wqDCoCA+Pj7CoMKgwqDCoMKgwqDCoMKgwqDCoCBnb3RvIGVycm9yX2JkZXZfcHV0Owo+ID4gPiA+ ID4gPiDCoMKgwqDCoMKgwqAgPj4+wqDCoMKgwqDCoMKgIH0KPiA+ID4gPiA+ID4gwqDCoMKgwqDC oMKgID4+PiAtwqDCoMKgIGlmICghbW91bnRfYXJnX2RldiAmJgo+ID4gPiA+ID4gPiBidHJmc19z dXBlcl9udW1fZGV2aWNlcyhkaXNrX3N1cGVyKQo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgID09IDEg JiYKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PiAtwqDCoMKgwqDCoMKgwqAgIShidHJmc19z dXBlcl9mbGFncyhkaXNrX3N1cGVyKSAmCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqAgQlRSRlNfU1VQ RVJfRkxBR19TRUVESU5HKSkgewo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+IC3CoMKgwqDC oMKgwqDCoCBkZXZfdCBkZXZ0Owo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICvCoMKgwqAg cmV0ID0gbG9va3VwX2JkZXYocGF0aCwgJmRldnQpOwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAg Pj4+ICvCoMKgwqAgaWYgKHJldCkKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PiArwqDCoMKg wqDCoMKgwqAgYnRyZnNfd2FybihOVUxMLCAibG9va3VwIGJkZXYgZmFpbGVkCj4gPiA+ID4gPiA+ IGZvciBwYXRoICVzOiAlZCIsCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj4gK8KgwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqAgcGF0aCwgcmV0KTsKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKg ID4+PiAtwqDCoMKgwqDCoMKgwqAgcmV0ID0gbG9va3VwX2JkZXYocGF0aCwgJmRldnQpOwo+ID4g PiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+IERvIHdl IGFjdHVhbGx5IG5lZWQgdGhpcyBjaGVjaz8gSXQgd2FzIGFkZGVkIHdpdGgKPiA+ID4gPiA+ID4g dGhlIHBhdGNoCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqAgc2tpcHBpbmcgdGhlCj4gPiA+ID4gPiA+ IMKgwqDCoMKgwqDCoCA+PiByZWdpc3RyYXRpb24sIHNvIGl0J3MgdmFsaWRhdGluZyB0aGUgYmxv Y2sgZGV2aWNlCj4gPiA+ID4gPiA+IGJ1dCBob3cgY2FuIHdlCj4gPiA+ID4gPiA+IMKgwqDCoMKg wqAgcGFzcwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4gc29tZXRoaW5nIHRoYXQgaXMgbm90 IGEgdmFsaWQgYmxvY2sgZGV2aWNlPwo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4KPiA+ID4g PiA+ID4gwqDCoMKgwqDCoMKgID4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4gRG8geW91IG1l YW4gdG8gY2hlY2sgaWYgdGhlIGxvb2t1cF9iZGV2KCkgaXMKPiA+ID4gPiA+ID4gc3VjY2Vzc2Z1 bD8gSG0uIEl0Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqAgc2hvdWxkCj4gPiA+ID4gPiA+IMKgwqDC oMKgwqDCoCA+IGJlIG9rYXkgbm90IHRvIGNoZWNrLCBidXQgd2UgZG8gdGhhdCBjb25zaXN0ZW50 bHkKPiA+ID4gPiA+ID4gaW4gb3RoZXIgcGxhY2VzLgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAg Pgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4gQmVzaWRlcyB0aGVyZSdzIGEgY2FsbCB0byBi ZGV2X29wZW5fYnlfcGF0aCgpCj4gPiA+ID4gPiA+IHRoYXQgaW4gdHVybiBkb2VzIHRoZQo+ID4g PiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4gbG9va3VwX2JkZXYgc28gY2hlY2tpbmcgaXQgaGVyZSBp cyByZWR1bmRhbnQuCj4gPiA+ID4gPiA+IEl0J3Mgbm90IHJlbGF0ZWQKPiA+ID4gPiA+ID4gwqDC oMKgwqDCoCB0byB0aGUKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+IGZpeCBpdHNlbGYgYnV0 IEkgZGVsZXRlZCBpdCBpbiBteSBmaXguCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pgo+ID4g PiA+ID4gPiDCoMKgwqDCoMKgwqAgPgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPiBPaCBuby4g V2UgbmVlZCAlZGV2dCB0byBiZSBzZXQgYmVjYXVzZToKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKg ID4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4gSXQgd2lsbCBtYXRjaCBpZiB0aGF0IGRldmlj ZSBpcyBhbHJlYWR5Cj4gPiA+ID4gPiA+IG1vdW50ZWQvc2Nhbm5lZC4KPiA+ID4gPiA+ID4gwqDC oMKgwqDCoMKgID4gSXQgd2lsbCBhbHNvIGZyZWUgc3RhbGUgZW50cmllcy4KPiA+ID4gPiA+ID4g wqDCoMKgwqDCoMKgID4KPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4gVGh4LCBBbmFuZAo+ID4g PiA+ID4gPiDCoMKgwqDCoMKgwqAgPgo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+IC3CoMKg wqDCoMKgwqDCoCBpZiAocmV0KQo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+IC3CoMKgwqDC oMKgwqDCoMKgwqDCoMKgIGJ0cmZzX3dhcm4oTlVMTCwgImxvb2t1cCBiZGV2Cj4gPiA+ID4gPiA+ IGZhaWxlZCBmb3IgcGF0aCAlczogJWQiLAo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+IC3C oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgcGF0aCwgcmV0KTsKPiA+ID4gPiA+ ID4gwqDCoMKgwqDCoMKgID4+PiAtwqDCoMKgwqDCoMKgwqAgZWxzZQo+ID4gPiA+ID4gPiDCoMKg wqDCoMKgwqAgPj4+ICvCoMKgwqAgaWYgKGJ0cmZzX3NraXBfcmVnaXN0cmF0aW9uKGRpc2tfc3Vw ZXIsCj4gPiA+ID4gPiA+IGRldnQsCj4gPiA+ID4gPiA+IMKgwqDCoMKgwqAgbW91bnRfYXJnX2Rl dikpIHsKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PiArwqDCoMKgwqDCoMKgwqAgcHJfZGVi dWcoIkJUUkZTOiBza2lwIHJlZ2lzdGVyaW5nCj4gPiA+ID4gPiA+IHNpbmdsZSBub24tc2VlZAo+ ID4gPiA+ID4gPiDCoMKgwqDCoMKgIGRldmljZSAlc1xuIiwKPiA+ID4gPiA+ID4gwqDCoMKgwqDC oMKgID4+PiArwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgcGF0aCk7Cj4gPiA+ID4gPiA+IMKg wqDCoMKgwqDCoCA+Pj4gK8KgwqDCoMKgwqDCoMKgIGlmIChkZXZ0KQo+ID4gPiA+ID4gPiDCoMKg wqDCoMKgwqAgPj4+wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoCBidHJmc19mcmVlX3N0YWxl X2RldmljZXMoZGV2dCwKPiA+ID4gPiA+ID4gTlVMTCk7Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDC oCA+Pj4gLQo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+IC3CoMKgwqDCoMKgwqDCoCBwcl9k ZWJ1ZygiQlRSRlM6IHNraXAgcmVnaXN0ZXJpbmcKPiA+ID4gPiA+ID4gc2luZ2xlIG5vbi1zZWVk IGRldmljZQo+ID4gPiA+ID4gPiDCoMKgwqDCoMKgwqAgPj4+ICVzXG4iLCBwYXRoKTsKPiA+ID4g PiA+ID4gwqDCoMKgwqDCoMKgID4+PsKgwqDCoMKgwqDCoMKgwqDCoMKgIGRldmljZSA9IE5VTEw7 Cj4gPiA+ID4gPiA+IMKgwqDCoMKgwqDCoCA+Pj7CoMKgwqDCoMKgwqDCoMKgwqDCoCBnb3RvIGZy ZWVfZGlza19zdXBlcjsKPiA+ID4gPiA+ID4gwqDCoMKgwqDCoMKgID4+PsKgwqDCoMKgwqDCoCB9 Cj4gPiA+ID4gPiA+IAoK
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> > --- > I still have some unknowns about the problem. Pls test if this fixes > the problem. Seems that we have that tested by some reportes, I checked it in my VM setup that it works (fstests and grub2-probe) so let's add it to for-next. Please use the changelog from my patch that describes the problem and then add description of how you fixed it. Thanks.
On 2/12/24 20:29, 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> >> --- >> I still have some unknowns about the problem. Pls test if this fixes >> the problem. It is a mystery- we didn't see "BTRFS: skip registering single non-seed device %s\n", in the kernel messages. Do you have any clue? > Seems that we have that tested by some reportes, I checked it in my VM > setup that it works (fstests and grub2-probe) so let's add it to > for-next. Please use the changelog from my patch that describes the > problem and then add description of how you fixed it. Thanks. Thxs for the verification. I'll revise, send out. Thx, Anand
On Tue, Feb 13, 2024 at 06:05:53AM +0530, Anand Jain wrote: > > > On 2/12/24 20:29, 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> > >> --- > > >> I still have some unknowns about the problem. Pls test if this fixes > >> the problem. > > It is a mystery- we didn't see > "BTRFS: skip registering single non-seed device %s\n", > in the kernel messages. > > Do you have any clue? It's printed by pr_debug(), so it may not be on by default. For my testing I changed it to printk(KERN_ERR "...") as I do for all debugging messages just to be sure, the message was printed. It may be actually useful to print that on the 'info' level as we do for the other scanning messages.
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); + } + + if (!mount_arg_dev && btrfs_super_num_devices(disk_super) == 1 && + !(btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_SEEDING)) + return true; + + 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); - 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; } diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 53f87f398da7..4ed281f0d1bd 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -792,5 +792,4 @@ bool btrfs_repair_one_zone(struct btrfs_fs_info *fs_info, u64 logical); bool btrfs_pinned_by_swapfile(struct btrfs_fs_info *fs_info, void *ptr); u8 *btrfs_sb_fsid_ptr(struct btrfs_super_block *sb); - #endif
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> --- I still have some unknowns about the problem. Pls test if this fixes the problem. fs/btrfs/volumes.c | 44 ++++++++++++++++++++++++++++++++++---------- fs/btrfs/volumes.h | 1 - 2 files changed, 34 insertions(+), 11 deletions(-)