| Message ID | b9108a14773af7a899226f59a9dbd0953d20abe5.1600940809.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix verify_one_dev_extent and btrfs_find_device | expand |
On 24.09.20 г. 13:11 ч., Anand Jain wrote: > commit 343694eee8d8 (btrfs: switch seed device to list api), missed to > check if the last arg (seed) is true in the function btrfs_find_device(). > This arg tells whether to traverse through the seed device list or not. > > This means after the above commit the arg is always true, and the parent > function which set this arg to false aren't effective. > > So we don't worry about the parent functions which set the last arg to > true, instead there is only one parent with calling btrfs_find_device > with the last arg false in device_list_add(). > > But in fact, even the device_list_add() has no purpose that it has to set > the last arg to false. Because the fs_devices always points to the > device's fs_devices. So with the devid+uuid matching, it shall find the > btrfs_device and returns. So naturally, it won't traverse through the > seed fs_devices (if) present. > > So this patch makes it official that we don't need the last arg in the > function btrfs_find_device() and it shall always traverse through the > seed device list. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> IMO the changelog could be vastly simplified by stating that following commit 343694eee8d8 the seed argument is no longer used and can simply be removed. In any case Reviewed-by: Nikolay Borisov <nborisov@suse.com>
On 24.09.20 г. 13:21 ч., Nikolay Borisov wrote: > > > On 24.09.20 г. 13:11 ч., Anand Jain wrote: >> commit 343694eee8d8 (btrfs: switch seed device to list api), missed to >> check if the last arg (seed) is true in the function btrfs_find_device(). >> This arg tells whether to traverse through the seed device list or not. >> >> This means after the above commit the arg is always true, and the parent >> function which set this arg to false aren't effective. >> >> So we don't worry about the parent functions which set the last arg to >> true, instead there is only one parent with calling btrfs_find_device >> with the last arg false in device_list_add(). >> >> But in fact, even the device_list_add() has no purpose that it has to set >> the last arg to false. Because the fs_devices always points to the >> device's fs_devices. So with the devid+uuid matching, it shall find the >> btrfs_device and returns. So naturally, it won't traverse through the >> seed fs_devices (if) present. >> >> So this patch makes it official that we don't need the last arg in the >> function btrfs_find_device() and it shall always traverse through the >> seed device list. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > IMO the changelog could be vastly simplified by stating that following > commit 343694eee8d8 the seed argument is no longer used and can simply > be removed. > > In any case > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > > Actually I went back and it seems that I accidentally removed the portion of btrfs_find_device() which was predicated on seed bool parameter being true or false. So the correct thing to do is really to send a patch which adds it back and consider it a fix (i.e adding a Fixes: tag as well). OTOH - does it really make any difference? SO what if btrfs_find_device returns a device from fs_devices->seed_list in device_list_add (which is called only from device scan)?
On 25/9/20 4:22 pm, Nikolay Borisov wrote: > > > On 24.09.20 г. 13:21 ч., Nikolay Borisov wrote: >> >> >> On 24.09.20 г. 13:11 ч., Anand Jain wrote: >>> commit 343694eee8d8 (btrfs: switch seed device to list api), missed to >>> check if the last arg (seed) is true in the function btrfs_find_device(). >>> This arg tells whether to traverse through the seed device list or not. >>> >>> This means after the above commit the arg is always true, and the parent >>> function which set this arg to false aren't effective. >>> >>> So we don't worry about the parent functions which set the last arg to >>> true, instead there is only one parent with calling btrfs_find_device >>> with the last arg false in device_list_add(). >>> >>> But in fact, even the device_list_add() has no purpose that it has to set >>> the last arg to false. Because the fs_devices always points to the >>> device's fs_devices. So with the devid+uuid matching, it shall find the >>> btrfs_device and returns. So naturally, it won't traverse through the >>> seed fs_devices (if) present. >>> >>> So this patch makes it official that we don't need the last arg in the >>> function btrfs_find_device() and it shall always traverse through the >>> seed device list. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> >> IMO the changelog could be vastly simplified by stating that following >> commit 343694eee8d8 the seed argument is no longer used and can simply >> be removed. >> >> In any case >> >> Reviewed-by: Nikolay Borisov <nborisov@suse.com> >> >> > > > Actually I went back and it seems that I accidentally removed the > portion of btrfs_find_device() which was predicated on seed bool > parameter being true or false. So the correct thing to do is really to > send a patch which adds it back and consider it a fix (i.e adding a > Fixes: tag as well). > > > OTOH - does it really make any difference? SO what if btrfs_find_device > returns a device from fs_devices->seed_list in device_list_add (which > is called only from device scan)? > There isn't bug as such. device scan is through the btrfs_control interface. So it depends on the device fsid to get fs_devices and it could never use seed_list with a genuine (non crafted) disk image. Thanks, Anand
On 9/24/20 6:11 AM, Anand Jain wrote: > commit 343694eee8d8 (btrfs: switch seed device to list api), missed to > check if the last arg (seed) is true in the function btrfs_find_device(). > This arg tells whether to traverse through the seed device list or not. > > This means after the above commit the arg is always true, and the parent > function which set this arg to false aren't effective. > > So we don't worry about the parent functions which set the last arg to > true, instead there is only one parent with calling btrfs_find_device > with the last arg false in device_list_add(). > > But in fact, even the device_list_add() has no purpose that it has to set > the last arg to false. Because the fs_devices always points to the > device's fs_devices. So with the devid+uuid matching, it shall find the > btrfs_device and returns. So naturally, it won't traverse through the > seed fs_devices (if) present. > > So this patch makes it official that we don't need the last arg in the > function btrfs_find_device() and it shall always traverse through the > seed device list. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c index 119721eeecf6..c58a99677cf9 100644 --- a/fs/btrfs/dev-replace.c +++ b/fs/btrfs/dev-replace.c @@ -149,10 +149,10 @@ int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info) case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED: case BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED: dev_replace->srcdev = btrfs_find_device(fs_info->fs_devices, - src_devid, NULL, NULL, true); + src_devid, NULL, NULL); dev_replace->tgtdev = btrfs_find_device(fs_info->fs_devices, BTRFS_DEV_REPLACE_DEVID, - NULL, NULL, true); + NULL, NULL); /* * allow 'btrfs dev replace_cancel' if src/tgt device is * missing diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index ab408a23ba32..a3550b0fa9b0 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1680,7 +1680,7 @@ static noinline int btrfs_ioctl_resize(struct file *file, btrfs_info(fs_info, "resizing devid %llu", devid); } - device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true); + device = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL); if (!device) { btrfs_info(fs_info, "resizer unable to find device %llu", devid); @@ -3323,7 +3323,7 @@ static long btrfs_ioctl_dev_info(struct btrfs_fs_info *fs_info, rcu_read_lock(); dev = btrfs_find_device(fs_info->fs_devices, di_args->devid, s_uuid, - NULL, true); + NULL); if (!dev) { ret = -ENODEV; diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index cf63f1e27a27..a0b5fb331791 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -3855,7 +3855,7 @@ int btrfs_scrub_dev(struct btrfs_fs_info *fs_info, u64 devid, u64 start, goto out_free_ctx; mutex_lock(&fs_info->fs_devices->device_list_mutex); - dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true); + dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL); if (!dev || (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state) && !is_dev_replace)) { mutex_unlock(&fs_info->fs_devices->device_list_mutex); @@ -4031,7 +4031,7 @@ int btrfs_scrub_progress(struct btrfs_fs_info *fs_info, u64 devid, struct scrub_ctx *sctx = NULL; mutex_lock(&fs_info->fs_devices->device_list_mutex); - dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true); + dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL); if (dev) sctx = dev->scrub_ctx; if (sctx) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 9be40eece8ed..29995f23aa75 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -822,7 +822,7 @@ static noinline struct btrfs_device *device_list_add(const char *path, } else { mutex_lock(&fs_devices->device_list_mutex); device = btrfs_find_device(fs_devices, devid, - disk_super->dev_item.uuid, NULL, false); + disk_super->dev_item.uuid, NULL); /* * If this disk has been pulled into an fs devices created by @@ -2296,10 +2296,10 @@ static struct btrfs_device *btrfs_find_device_by_path( dev_uuid = disk_super->dev_item.uuid; if (btrfs_fs_incompat(fs_info, METADATA_UUID)) device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid, - disk_super->metadata_uuid, true); + disk_super->metadata_uuid); else device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid, - disk_super->fsid, true); + disk_super->fsid); btrfs_release_disk_super(disk_super); if (!device) @@ -2319,7 +2319,7 @@ struct btrfs_device *btrfs_find_device_by_devspec( if (devid) { device = btrfs_find_device(fs_info->fs_devices, devid, NULL, - NULL, true); + NULL); if (!device) return ERR_PTR(-ENOENT); return device; @@ -2468,7 +2468,7 @@ static int btrfs_finish_sprout(struct btrfs_trans_handle *trans) read_extent_buffer(leaf, fs_uuid, btrfs_device_fsid(dev_item), BTRFS_FSID_SIZE); device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid, - fs_uuid, true); + fs_uuid); BUG_ON(!device); /* Logic error */ if (device->fs_devices->seeding) { @@ -6450,8 +6450,7 @@ blk_status_t btrfs_map_bio(struct btrfs_fs_info *fs_info, struct bio *bio, * If @seed is true, traverse through the seed devices. */ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices, - u64 devid, u8 *uuid, u8 *fsid, - bool seed) + u64 devid, u8 *uuid, u8 *fsid) { struct btrfs_device *device; struct btrfs_fs_devices *seed_devs; @@ -6658,7 +6657,7 @@ static int read_one_chunk(struct btrfs_key *key, struct extent_buffer *leaf, btrfs_stripe_dev_uuid_nr(chunk, i), BTRFS_UUID_SIZE); map->stripes[i].dev = btrfs_find_device(fs_info->fs_devices, - devid, uuid, NULL, true); + devid, uuid, NULL); if (!map->stripes[i].dev && !btrfs_test_opt(fs_info, DEGRADED)) { free_extent_map(em); @@ -6797,7 +6796,7 @@ static int read_one_dev(struct extent_buffer *leaf, } device = btrfs_find_device(fs_info->fs_devices, devid, dev_uuid, - fs_uuid, true); + fs_uuid); if (!device) { if (!btrfs_test_opt(fs_info, DEGRADED)) { btrfs_report_missing_device(fs_info, devid, @@ -7439,8 +7438,7 @@ int btrfs_get_dev_stats(struct btrfs_fs_info *fs_info, int i; mutex_lock(&fs_devices->device_list_mutex); - dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL, - true); + dev = btrfs_find_device(fs_info->fs_devices, stats->devid, NULL, NULL); mutex_unlock(&fs_devices->device_list_mutex); if (!dev) { @@ -7571,7 +7569,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info, } /* Make sure no dev extent is beyond device bondary */ - dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL, true); + dev = btrfs_find_device(fs_info->fs_devices, devid, NULL, NULL); if (!dev) { btrfs_err(fs_info, "failed to find devid %llu", devid); ret = -EUCLEAN; diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 48bdca01e237..ebb7df93e63f 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -453,7 +453,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len); int btrfs_grow_device(struct btrfs_trans_handle *trans, struct btrfs_device *device, u64 new_size); struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices, - u64 devid, u8 *uuid, u8 *fsid, bool seed); + u64 devid, u8 *uuid, u8 *fsid); int btrfs_shrink_device(struct btrfs_device *device, u64 new_size); int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *path); int btrfs_balance(struct btrfs_fs_info *fs_info,
commit 343694eee8d8 (btrfs: switch seed device to list api), missed to check if the last arg (seed) is true in the function btrfs_find_device(). This arg tells whether to traverse through the seed device list or not. This means after the above commit the arg is always true, and the parent function which set this arg to false aren't effective. So we don't worry about the parent functions which set the last arg to true, instead there is only one parent with calling btrfs_find_device with the last arg false in device_list_add(). But in fact, even the device_list_add() has no purpose that it has to set the last arg to false. Because the fs_devices always points to the device's fs_devices. So with the devid+uuid matching, it shall find the btrfs_device and returns. So naturally, it won't traverse through the seed fs_devices (if) present. So this patch makes it official that we don't need the last arg in the function btrfs_find_device() and it shall always traverse through the seed device list. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/dev-replace.c | 4 ++-- fs/btrfs/ioctl.c | 4 ++-- fs/btrfs/scrub.c | 4 ++-- fs/btrfs/volumes.c | 22 ++++++++++------------ fs/btrfs/volumes.h | 2 +- 5 files changed, 17 insertions(+), 19 deletions(-)