* [PATCH 0/5] Convert seed devices to proper list API @ 2020-07-15 10:48 Nikolay Borisov 2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov ` (6 more replies) 0 siblings, 7 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov This series converts the existing seed devices pointer in btrfs_fs_devices to proper list api. First 4 patches are cleanups preparing the code for the switch. Patch 5 has more information about the required transformations, it might seem lengthy but he logic everywhere is pretty much the same so shouldn't be that cumbersome to review. This series survived xfstest runs and even caught 2 bugs while developing it. Nikolay Borisov (5): btrfs: Factor out reada loop in __reada_start_machine btrfs: Factor out loop logic from btrfs_free_extra_devids btrfs: Make close_fs_devices return void btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices btrfs: Switch seed device to list api fs/btrfs/disk-io.c | 39 +++++------ fs/btrfs/reada.c | 26 ++++--- fs/btrfs/sysfs.c | 4 -- fs/btrfs/volumes.c | 166 ++++++++++++++++++++------------------------- fs/btrfs/volumes.h | 6 +- 5 files changed, 112 insertions(+), 129 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine 2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov @ 2020-07-15 10:48 ` Nikolay Borisov 2020-08-18 15:02 ` Josef Bacik 2020-08-29 15:06 ` Anand Jain 2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov ` (5 subsequent siblings) 6 siblings, 2 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov This is in preparation for moving fs_devices to proper lists. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/reada.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c index 243a2e44526e..aa9d24ed56d7 100644 --- a/fs/btrfs/reada.c +++ b/fs/btrfs/reada.c @@ -767,15 +767,14 @@ static void reada_start_machine_worker(struct btrfs_work *work) kfree(rmw); } -static void __reada_start_machine(struct btrfs_fs_info *fs_info) + +/* Try to start up to 10k READA requests for a group of devices. */ +static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices) { - struct btrfs_device *device; - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; u64 enqueued; u64 total = 0; - int i; + struct btrfs_device *device; -again: do { enqueued = 0; mutex_lock(&fs_devices->device_list_mutex); @@ -787,6 +786,18 @@ static void __reada_start_machine(struct btrfs_fs_info *fs_info) mutex_unlock(&fs_devices->device_list_mutex); total += enqueued; } while (enqueued && total < 10000); + + return total; +} + +static void __reada_start_machine(struct btrfs_fs_info *fs_info) +{ + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + int i; + u64 enqueued = 0; + +again: + enqueued += __reada_start_for_fsdevs(fs_devices); if (fs_devices->seed) { fs_devices = fs_devices->seed; goto again; -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine 2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov @ 2020-08-18 15:02 ` Josef Bacik 2020-08-29 15:06 ` Anand Jain 1 sibling, 0 replies; 31+ messages in thread From: Josef Bacik @ 2020-08-18 15:02 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 7/15/20 6:48 AM, Nikolay Borisov wrote: > This is in preparation for moving fs_devices to proper lists. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine 2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov 2020-08-18 15:02 ` Josef Bacik @ 2020-08-29 15:06 ` Anand Jain 2020-08-31 12:24 ` David Sterba 1 sibling, 1 reply; 31+ messages in thread From: Anand Jain @ 2020-08-29 15:06 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 15/7/20 6:48 pm, Nikolay Borisov wrote: > This is in preparation for moving fs_devices to proper lists. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/reada.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c > index 243a2e44526e..aa9d24ed56d7 100644 > --- a/fs/btrfs/reada.c > +++ b/fs/btrfs/reada.c > @@ -767,15 +767,14 @@ static void reada_start_machine_worker(struct btrfs_work *work) > kfree(rmw); > } > > -static void __reada_start_machine(struct btrfs_fs_info *fs_info) > + > +/* Try to start up to 10k READA requests for a group of devices. */ > +static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices) In my experience David doesn't prefer __ prefix for helper functions. > { > - struct btrfs_device *device; > - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; > u64 enqueued; > u64 total = 0; > - int i; > + struct btrfs_device *device; > > -again: > do { > enqueued = 0; > mutex_lock(&fs_devices->device_list_mutex); > @@ -787,6 +786,18 @@ static void __reada_start_machine(struct btrfs_fs_info *fs_info) > mutex_unlock(&fs_devices->device_list_mutex); > total += enqueued; > } while (enqueued && total < 10000); > + > + return total; > +} > + > +static void __reada_start_machine(struct btrfs_fs_info *fs_info) > +{ > + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; > + int i; > + u64 enqueued = 0; > + > +again: > + enqueued += __reada_start_for_fsdevs(fs_devices); > if (fs_devices->seed) { > fs_devices = fs_devices->seed; > goto again; > There wasn't any need to create another helper function here IMO. Anyway changes looks good. Reviewed-by: Anand Jain <anand.jain@oracle.com> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine 2020-08-29 15:06 ` Anand Jain @ 2020-08-31 12:24 ` David Sterba 0 siblings, 0 replies; 31+ messages in thread From: David Sterba @ 2020-08-31 12:24 UTC (permalink / raw) To: Anand Jain; +Cc: Nikolay Borisov, linux-btrfs On Sat, Aug 29, 2020 at 11:06:32PM +0800, Anand Jain wrote: > On 15/7/20 6:48 pm, Nikolay Borisov wrote: > > This is in preparation for moving fs_devices to proper lists. > > > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > > --- > > fs/btrfs/reada.c | 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c > > index 243a2e44526e..aa9d24ed56d7 100644 > > --- a/fs/btrfs/reada.c > > +++ b/fs/btrfs/reada.c > > @@ -767,15 +767,14 @@ static void reada_start_machine_worker(struct btrfs_work *work) > > kfree(rmw); > > } > > > > -static void __reada_start_machine(struct btrfs_fs_info *fs_info) > > + > > +/* Try to start up to 10k READA requests for a group of devices. */ > > +static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices) > > In my experience David doesn't prefer __ prefix for helper functions. Right, I've updated the patch to drop the __ as there's no other function without the underscores (unlike __reada_start_machine/reada_start_machine) ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids 2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov 2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov @ 2020-07-15 10:48 ` Nikolay Borisov 2020-07-15 12:32 ` kernel test robot ` (2 more replies) 2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov ` (4 subsequent siblings) 6 siblings, 3 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov This prepares the code to switching seeds devices to a proper list. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/volumes.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index ce01e44f8134..db29fc4fbe89 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1024,28 +1024,24 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig) return ERR_PTR(ret); } -/* - * After we have read the system tree and know devids belonging to - * this filesystem, remove the device which does not belong there. - */ -void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) + + +void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step, + struct btrfs_device **latest_dev) { struct btrfs_device *device, *next; - struct btrfs_device *latest_dev = NULL; - mutex_lock(&uuid_mutex); -again: /* This is the initialized path, it is safe to release the devices. */ list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) { if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, - &device->dev_state)) { + &device->dev_state)) { if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, - &device->dev_state) && + &device->dev_state) && !test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state) && - (!latest_dev || - device->generation > latest_dev->generation)) { - latest_dev = device; + (!*latest_dev || + device->generation > (*latest_dev)->generation)) { + *latest_dev = device; } continue; } @@ -1083,6 +1079,18 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) btrfs_free_device(device); } +} +/* + * After we have read the system tree and know devids belonging to + * this filesystem, remove the device which does not belong there. + */ +void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) +{ + struct btrfs_device *latest_dev = NULL; + + mutex_lock(&uuid_mutex); +again: + __btrfs_free_extra_devids(fs_devices, step, &latest_dev); if (fs_devices->seed) { fs_devices = fs_devices->seed; goto again; -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids 2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov @ 2020-07-15 12:32 ` kernel test robot 2020-07-15 12:39 ` Nikolay Borisov 2020-07-16 7:17 ` [PATCH v2] " Nikolay Borisov 2020-08-18 15:03 ` [PATCH 2/5] " Josef Bacik 2 siblings, 1 reply; 31+ messages in thread From: kernel test robot @ 2020-07-15 12:32 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs; +Cc: kbuild-all, Nikolay Borisov [-- Attachment #1: Type: text/plain, Size: 3949 bytes --] Hi Nikolay, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on kdave/for-next] [also build test WARNING on v5.8-rc5 next-20200715] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Convert-seed-devices-to-proper-list-API/20200715-185102 base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next config: x86_64-rhel-7.6-kselftests (attached as .config) compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from fs/btrfs/volumes.c:17: fs/btrfs/ctree.h:2271:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers] 2271 | size_t __const btrfs_get_num_csums(void); | ^~~~~~~ In file included from fs/btrfs/volumes.c:28: fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers] 16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set); | ^~~~~ >> fs/btrfs/volumes.c:1029:6: warning: no previous prototype for '__btrfs_free_extra_devids' [-Wmissing-prototypes] 1029 | void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step, | ^~~~~~~~~~~~~~~~~~~~~~~~~ vim +/__btrfs_free_extra_devids +1029 fs/btrfs/volumes.c 1026 1027 1028 > 1029 void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step, 1030 struct btrfs_device **latest_dev) 1031 { 1032 struct btrfs_device *device, *next; 1033 1034 /* This is the initialized path, it is safe to release the devices. */ 1035 list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) { 1036 if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, 1037 &device->dev_state)) { 1038 if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, 1039 &device->dev_state) && 1040 !test_bit(BTRFS_DEV_STATE_MISSING, 1041 &device->dev_state) && 1042 (!*latest_dev || 1043 device->generation > (*latest_dev)->generation)) { 1044 *latest_dev = device; 1045 } 1046 continue; 1047 } 1048 1049 if (device->devid == BTRFS_DEV_REPLACE_DEVID) { 1050 /* 1051 * In the first step, keep the device which has 1052 * the correct fsid and the devid that is used 1053 * for the dev_replace procedure. 1054 * In the second step, the dev_replace state is 1055 * read from the device tree and it is known 1056 * whether the procedure is really active or 1057 * not, which means whether this device is 1058 * used or whether it should be removed. 1059 */ 1060 if (step == 0 || test_bit(BTRFS_DEV_STATE_REPLACE_TGT, 1061 &device->dev_state)) { 1062 continue; 1063 } 1064 } 1065 if (device->bdev) { 1066 blkdev_put(device->bdev, device->mode); 1067 device->bdev = NULL; 1068 fs_devices->open_devices--; 1069 } 1070 if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state)) { 1071 list_del_init(&device->dev_alloc_list); 1072 clear_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state); 1073 if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, 1074 &device->dev_state)) 1075 fs_devices->rw_devices--; 1076 } 1077 list_del_init(&device->dev_list); 1078 fs_devices->num_devices--; 1079 btrfs_free_device(device); 1080 } 1081 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 50003 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids 2020-07-15 12:32 ` kernel test robot @ 2020-07-15 12:39 ` Nikolay Borisov 0 siblings, 0 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-15 12:39 UTC (permalink / raw) To: linux-btrfs On 15.07.20 г. 15:32 ч., kernel test robot wrote: > Hi Nikolay, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on kdave/for-next] > [also build test WARNING on v5.8-rc5 next-20200715] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Convert-seed-devices-to-proper-list-API/20200715-185102 > base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next > config: x86_64-rhel-7.6-kselftests (attached as .config) > compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 > reproduce (this is a W=1 build): > # save the attached .config to linux build tree > make W=1 ARCH=x86_64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > In file included from fs/btrfs/volumes.c:17: > fs/btrfs/ctree.h:2271:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers] > 2271 | size_t __const btrfs_get_num_csums(void); > | ^~~~~~~ > In file included from fs/btrfs/volumes.c:28: > fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers] > 16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set); > | ^~~~~ >>> fs/btrfs/volumes.c:1029:6: warning: no previous prototype for '__btrfs_free_extra_devids' [-Wmissing-prototypes] > 1029 | void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step, > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > > vim +/__btrfs_free_extra_devids +1029 fs/btrfs/volumes.c > > 1026 > 1027 > 1028 >> 1029 void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step This function is missing a static modifier. It shouldn't be a reason to block review. ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2] btrfs: Factor out loop logic from btrfs_free_extra_devids 2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov 2020-07-15 12:32 ` kernel test robot @ 2020-07-16 7:17 ` Nikolay Borisov 2020-08-29 15:13 ` Anand Jain 2020-08-18 15:03 ` [PATCH 2/5] " Josef Bacik 2 siblings, 1 reply; 31+ messages in thread From: Nikolay Borisov @ 2020-07-16 7:17 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov This prepares the code to switching seeds devices to a proper list. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- V2: * Added missing static modifier to the factored out function. Reported by kernel test robot fs/btrfs/volumes.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index ce01e44f8134..76a68edb3127 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1024,28 +1024,24 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig) return ERR_PTR(ret); } -/* - * After we have read the system tree and know devids belonging to - * this filesystem, remove the device which does not belong there. - */ -void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) + + +static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, + int step, struct btrfs_device **latest_dev) { struct btrfs_device *device, *next; - struct btrfs_device *latest_dev = NULL; - mutex_lock(&uuid_mutex); -again: /* This is the initialized path, it is safe to release the devices. */ list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) { if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, - &device->dev_state)) { + &device->dev_state)) { if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, - &device->dev_state) && + &device->dev_state) && !test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state) && - (!latest_dev || - device->generation > latest_dev->generation)) { - latest_dev = device; + (!*latest_dev || + device->generation > (*latest_dev)->generation)) { + *latest_dev = device; } continue; } @@ -1083,6 +1079,18 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) btrfs_free_device(device); } +} +/* + * After we have read the system tree and know devids belonging to + * this filesystem, remove the device which does not belong there. + */ +void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) +{ + struct btrfs_device *latest_dev = NULL; + + mutex_lock(&uuid_mutex); +again: + __btrfs_free_extra_devids(fs_devices, step, &latest_dev); if (fs_devices->seed) { fs_devices = fs_devices->seed; goto again; -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2] btrfs: Factor out loop logic from btrfs_free_extra_devids 2020-07-16 7:17 ` [PATCH v2] " Nikolay Borisov @ 2020-08-29 15:13 ` Anand Jain 0 siblings, 0 replies; 31+ messages in thread From: Anand Jain @ 2020-08-29 15:13 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 16/7/20 3:17 pm, Nikolay Borisov wrote: > This prepares the code to switching seeds devices to a proper list. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > > V2: > * Added missing static modifier to the factored out function. Reported by > kernel test robot > > fs/btrfs/volumes.c | 34 +++++++++++++++++++++------------- > 1 file changed, 21 insertions(+), 13 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index ce01e44f8134..76a68edb3127 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1024,28 +1024,24 @@ static struct btrfs_fs_devices *clone_fs_devices(struct btrfs_fs_devices *orig) > return ERR_PTR(ret); > } > > -/* > - * After we have read the system tree and know devids belonging to > - * this filesystem, remove the device which does not belong there. > - */ > -void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) > + > + > +static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, > + int step, struct btrfs_device **latest_dev) David doesn't prefer __ prefix for helpers. > { > struct btrfs_device *device, *next; > - struct btrfs_device *latest_dev = NULL; > > - mutex_lock(&uuid_mutex); > -again: > /* This is the initialized path, it is safe to release the devices. */ > list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) { > if (test_bit(BTRFS_DEV_STATE_IN_FS_METADATA, > - &device->dev_state)) { > + &device->dev_state)) { > if (!test_bit(BTRFS_DEV_STATE_REPLACE_TGT, > - &device->dev_state) && > + &device->dev_state) && > !test_bit(BTRFS_DEV_STATE_MISSING, > &device->dev_state) && > - (!latest_dev || > - device->generation > latest_dev->generation)) { > - latest_dev = device; > + (!*latest_dev || > + device->generation > (*latest_dev)->generation)) { > + *latest_dev = device; > } > continue; > } > @@ -1083,6 +1079,18 @@ void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) > btrfs_free_device(device); > } > > +} > +/* > + * After we have read the system tree and know devids belonging to > + * this filesystem, remove the device which does not belong there. > + */ > +void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) > +{ > + struct btrfs_device *latest_dev = NULL; > + > + mutex_lock(&uuid_mutex); > +again: > + __btrfs_free_extra_devids(fs_devices, step, &latest_dev); > if (fs_devices->seed) { > fs_devices = fs_devices->seed; > goto again; > -- > 2.17.1 > Looks good. Reviewed-by: Anand Jain <anand.jain@oracle.com> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids 2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov 2020-07-15 12:32 ` kernel test robot 2020-07-16 7:17 ` [PATCH v2] " Nikolay Borisov @ 2020-08-18 15:03 ` Josef Bacik 2 siblings, 0 replies; 31+ messages in thread From: Josef Bacik @ 2020-08-18 15:03 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 7/15/20 6:48 AM, Nikolay Borisov wrote: > This prepares the code to switching seeds devices to a proper list. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/5] btrfs: Make close_fs_devices return void 2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov 2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov 2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov @ 2020-07-15 10:48 ` Nikolay Borisov 2020-08-18 15:05 ` Josef Bacik 2020-08-29 15:14 ` Anand Jain 2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov ` (3 subsequent siblings) 6 siblings, 2 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov The return value of this function conveys absolutely no information. All callers already check the state of fs_devices->opened to decide how to proceed. So conver the function to returning void. While at it make btrfs_close_devices also return void. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/volumes.c | 12 ++++-------- fs/btrfs/volumes.h | 2 +- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index db29fc4fbe89..6de021c78277 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1146,12 +1146,12 @@ static void btrfs_close_one_device(struct btrfs_device *device) ASSERT(atomic_read(&device->reada_in_flight) == 0); } -static int close_fs_devices(struct btrfs_fs_devices *fs_devices) +static void close_fs_devices(struct btrfs_fs_devices *fs_devices) { struct btrfs_device *device, *tmp; if (--fs_devices->opened > 0) - return 0; + return; mutex_lock(&fs_devices->device_list_mutex); list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) { @@ -1163,17 +1163,14 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices) WARN_ON(fs_devices->rw_devices); fs_devices->opened = 0; fs_devices->seeding = false; - - return 0; } -int btrfs_close_devices(struct btrfs_fs_devices *fs_devices) +void btrfs_close_devices(struct btrfs_fs_devices *fs_devices) { struct btrfs_fs_devices *seed_devices = NULL; - int ret; mutex_lock(&uuid_mutex); - ret = close_fs_devices(fs_devices); + close_fs_devices(fs_devices); if (!fs_devices->opened) { seed_devices = fs_devices->seed; fs_devices->seed = NULL; @@ -1186,7 +1183,6 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices) close_fs_devices(fs_devices); free_fs_devices(fs_devices); } - return ret; } static int open_fs_devices(struct btrfs_fs_devices *fs_devices, diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 5eea93916fbf..76e5470e19a8 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -435,7 +435,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices, struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags, void *holder); int btrfs_forget_devices(const char *path); -int btrfs_close_devices(struct btrfs_fs_devices *fs_devices); +void btrfs_close_devices(struct btrfs_fs_devices *fs_devices); void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step); void btrfs_assign_next_active_device(struct btrfs_device *device, struct btrfs_device *this_dev); -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] btrfs: Make close_fs_devices return void 2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov @ 2020-08-18 15:05 ` Josef Bacik 2020-08-29 15:14 ` Anand Jain 1 sibling, 0 replies; 31+ messages in thread From: Josef Bacik @ 2020-08-18 15:05 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 7/15/20 6:48 AM, Nikolay Borisov wrote: > The return value of this function conveys absolutely no information. > All callers already check the state of fs_devices->opened to decide > how to proceed. So conver the function to returning void. While at it > make btrfs_close_devices also return void. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] btrfs: Make close_fs_devices return void 2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov 2020-08-18 15:05 ` Josef Bacik @ 2020-08-29 15:14 ` Anand Jain 1 sibling, 0 replies; 31+ messages in thread From: Anand Jain @ 2020-08-29 15:14 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 15/7/20 6:48 pm, Nikolay Borisov wrote: > The return value of this function conveys absolutely no information. > All callers already check the state of fs_devices->opened to decide > how to proceed. So conver the function to returning void. While at it > make btrfs_close_devices also return void. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/volumes.c | 12 ++++-------- > fs/btrfs/volumes.h | 2 +- > 2 files changed, 5 insertions(+), 9 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index db29fc4fbe89..6de021c78277 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1146,12 +1146,12 @@ static void btrfs_close_one_device(struct btrfs_device *device) > ASSERT(atomic_read(&device->reada_in_flight) == 0); > } > > -static int close_fs_devices(struct btrfs_fs_devices *fs_devices) > +static void close_fs_devices(struct btrfs_fs_devices *fs_devices) > { > struct btrfs_device *device, *tmp; > > if (--fs_devices->opened > 0) > - return 0; > + return; > > mutex_lock(&fs_devices->device_list_mutex); > list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) { > @@ -1163,17 +1163,14 @@ static int close_fs_devices(struct btrfs_fs_devices *fs_devices) > WARN_ON(fs_devices->rw_devices); > fs_devices->opened = 0; > fs_devices->seeding = false; > - > - return 0; > } > > -int btrfs_close_devices(struct btrfs_fs_devices *fs_devices) > +void btrfs_close_devices(struct btrfs_fs_devices *fs_devices) > { > struct btrfs_fs_devices *seed_devices = NULL; > - int ret; > > mutex_lock(&uuid_mutex); > - ret = close_fs_devices(fs_devices); > + close_fs_devices(fs_devices); > if (!fs_devices->opened) { > seed_devices = fs_devices->seed; > fs_devices->seed = NULL; > @@ -1186,7 +1183,6 @@ int btrfs_close_devices(struct btrfs_fs_devices *fs_devices) > close_fs_devices(fs_devices); > free_fs_devices(fs_devices); > } > - return ret; > } > > static int open_fs_devices(struct btrfs_fs_devices *fs_devices, > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index 5eea93916fbf..76e5470e19a8 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -435,7 +435,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices, > struct btrfs_device *btrfs_scan_one_device(const char *path, > fmode_t flags, void *holder); > int btrfs_forget_devices(const char *path); > -int btrfs_close_devices(struct btrfs_fs_devices *fs_devices); > +void btrfs_close_devices(struct btrfs_fs_devices *fs_devices); > void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step); > void btrfs_assign_next_active_device(struct btrfs_device *device, > struct btrfs_device *this_dev); > Reviewed-by: Anand Jain <anand.jain@oracle.com> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices 2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov ` (2 preceding siblings ...) 2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov @ 2020-07-15 10:48 ` Nikolay Borisov 2020-08-18 15:08 ` Josef Bacik 2020-08-26 10:50 ` Anand Jain 2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov ` (2 subsequent siblings) 6 siblings, 2 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov It makes no sense to have sysfs-related routines be responsible for properly initialising the fs_info pointer of struct btrfs_fs_device. Instead this can be streamlined by making it the responsibility of btrfs_init_devices_late to initialize it. That function already initializes fs_info of every individual device in btrfs_fs_devices. As far as clearing it is concerned it makes sense to move it to close_fs_devices. That function is only called when struct btrfs_fs_devices is no longer in use - either for holding seeds or main devices for a mounted filesystem. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/sysfs.c | 4 ---- fs/btrfs/volumes.c | 22 ++-------------------- fs/btrfs/volumes.h | 2 -- 3 files changed, 2 insertions(+), 26 deletions(-) diff --git a/fs/btrfs/sysfs.c b/fs/btrfs/sysfs.c index 38c0b95e0e7f..24f6bbd655e3 100644 --- a/fs/btrfs/sysfs.c +++ b/fs/btrfs/sysfs.c @@ -939,8 +939,6 @@ void btrfs_sysfs_remove_mounted(struct btrfs_fs_info *fs_info) { struct kobject *fsid_kobj = &fs_info->fs_devices->fsid_kobj; - btrfs_reset_fs_info_ptr(fs_info); - sysfs_remove_link(fsid_kobj, "bdi"); if (fs_info->space_info_kobj) { @@ -1397,8 +1395,6 @@ int btrfs_sysfs_add_mounted(struct btrfs_fs_info *fs_info) struct btrfs_fs_devices *fs_devs = fs_info->fs_devices; struct kobject *fsid_kobj = &fs_devs->fsid_kobj; - btrfs_set_fs_info_ptr(fs_info); - error = btrfs_sysfs_add_devices_dir(fs_devs, NULL); if (error) return error; diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 6de021c78277..a0cdd027e99c 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1163,6 +1163,7 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices) WARN_ON(fs_devices->rw_devices); fs_devices->opened = 0; fs_devices->seeding = false; + fs_devices->fs_info = NULL; } void btrfs_close_devices(struct btrfs_fs_devices *fs_devices) @@ -7149,6 +7150,7 @@ void btrfs_init_devices_late(struct btrfs_fs_info *fs_info) device->fs_info = fs_info; mutex_unlock(&fs_devices->device_list_mutex); + fs_devices->fs_info = fs_info; fs_devices = fs_devices->seed; } } @@ -7447,24 +7449,6 @@ void btrfs_commit_device_sizes(struct btrfs_transaction *trans) mutex_unlock(&trans->fs_info->chunk_mutex); } -void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info) -{ - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; - while (fs_devices) { - fs_devices->fs_info = fs_info; - fs_devices = fs_devices->seed; - } -} - -void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info) -{ - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; - while (fs_devices) { - fs_devices->fs_info = NULL; - fs_devices = fs_devices->seed; - } -} - /* * Multiplicity factor for simple profiles: DUP, RAID1-like and RAID10. */ @@ -7475,8 +7459,6 @@ int btrfs_bg_type_to_factor(u64 flags) return btrfs_raid_array[index].ncopies; } - - static int verify_one_dev_extent(struct btrfs_fs_info *fs_info, u64 chunk_offset, u64 devid, u64 physical_offset, u64 physical_len) diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 76e5470e19a8..fc283fdbcece 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -569,8 +569,6 @@ static inline enum btrfs_raid_types btrfs_bg_flags_to_raid_index(u64 flags) void btrfs_commit_device_sizes(struct btrfs_transaction *trans); struct list_head * __attribute_const__ btrfs_get_fs_uuids(void); -void btrfs_set_fs_info_ptr(struct btrfs_fs_info *fs_info); -void btrfs_reset_fs_info_ptr(struct btrfs_fs_info *fs_info); bool btrfs_check_rw_degradable(struct btrfs_fs_info *fs_info, struct btrfs_device *failing_dev); -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices 2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov @ 2020-08-18 15:08 ` Josef Bacik 2020-08-26 10:50 ` Anand Jain 1 sibling, 0 replies; 31+ messages in thread From: Josef Bacik @ 2020-08-18 15:08 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 7/15/20 6:48 AM, Nikolay Borisov wrote: > It makes no sense to have sysfs-related routines be responsible for > properly initialising the fs_info pointer of struct btrfs_fs_device. > Instead this can be streamlined by making it the responsibility of > btrfs_init_devices_late to initialize it. That function already > initializes fs_info of every individual device in btrfs_fs_devices. > > As far as clearing it is concerned it makes sense to move it to > close_fs_devices. That function is only called when struct > btrfs_fs_devices is no longer in use - either for holding seeds or main > devices for a mounted filesystem. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> I was sort of worried we inadvertently depended on some other locking from where btrfs_sysfs_add_mounted() was called, but we don't Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices 2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov 2020-08-18 15:08 ` Josef Bacik @ 2020-08-26 10:50 ` Anand Jain 1 sibling, 0 replies; 31+ messages in thread From: Anand Jain @ 2020-08-26 10:50 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 15/7/20 6:48 pm, Nikolay Borisov wrote: > It makes no sense to have sysfs-related routines be responsible for > properly initialising the fs_info pointer of struct btrfs_fs_device. > Instead this can be streamlined by making it the responsibility of > btrfs_init_devices_late to initialize it. That function already > initializes fs_info of every individual device in btrfs_fs_devices. > > As far as clearing it is concerned it makes sense to move it to > close_fs_devices. That function is only called when struct > btrfs_fs_devices is no longer in use - either for holding seeds or main > devices for a mounted filesystem. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: Anand Jain <anand.jain@oracle.com> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 5/5] btrfs: Switch seed device to list api 2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov ` (3 preceding siblings ...) 2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov @ 2020-07-15 10:48 ` Nikolay Borisov 2020-07-15 13:14 ` kernel test robot ` (3 more replies) 2020-07-22 14:26 ` [PATCH 0/5] Convert seed devices to proper list API David Sterba 2020-08-17 19:19 ` Nikolay Borisov 6 siblings, 4 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-15 10:48 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov While this patch touches a bunch of files the conversion is straighforward. Instead of using the implicit linked list anchored at btrfs_fs_devices::seed the code is switched to using list_for_each_entry. Previous patches in the series already factored out code that processed both main and seed devices so in those cases the factored out functions are called on the main fs_devices and then on every seed dev inside list_for_each_entry. Using list api also allows to simplify deletion from the seed dev list performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by substituting a while() loop with a simple list_del_init. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- fs/btrfs/disk-io.c | 39 ++++++++--------- fs/btrfs/reada.c | 9 ++-- fs/btrfs/volumes.c | 102 ++++++++++++++++++++++----------------------- fs/btrfs/volumes.h | 2 +- 4 files changed, 72 insertions(+), 80 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 2fb0b43bc1c5..8767d47f6e29 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -545,33 +545,30 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page) static int check_tree_block_fsid(struct extent_buffer *eb) { struct btrfs_fs_info *fs_info = eb->fs_info; - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs; u8 fsid[BTRFS_FSID_SIZE]; - int ret = 1; + u8 *metadata_uuid; read_extent_buffer(eb, fsid, offsetof(struct btrfs_header, fsid), BTRFS_FSID_SIZE); - while (fs_devices) { - u8 *metadata_uuid; + /* + * Checking the incompat flag is only valid for the current + * fs. For seed devices it's forbidden to have their uuid + * changed so reading ->fsid in this case is fine + */ + if (btrfs_fs_incompat(fs_info, METADATA_UUID)) + metadata_uuid = fs_devices->metadata_uuid; + else + metadata_uuid = fs_devices->fsid; - /* - * Checking the incompat flag is only valid for the current - * fs. For seed devices it's forbidden to have their uuid - * changed so reading ->fsid in this case is fine - */ - if (fs_devices == fs_info->fs_devices && - btrfs_fs_incompat(fs_info, METADATA_UUID)) - metadata_uuid = fs_devices->metadata_uuid; - else - metadata_uuid = fs_devices->fsid; + if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE)) + return 0; - if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE)) { - ret = 0; - break; - } - fs_devices = fs_devices->seed; - } - return ret; + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) + if (!memcmp(fsid, seed_devs->fsid, BTRFS_FSID_SIZE)) + return 0; + + return 1; } static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c index aa9d24ed56d7..ac5b07ded0fe 100644 --- a/fs/btrfs/reada.c +++ b/fs/btrfs/reada.c @@ -792,16 +792,13 @@ static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices) static void __reada_start_machine(struct btrfs_fs_info *fs_info) { - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs; int i; u64 enqueued = 0; -again: enqueued += __reada_start_for_fsdevs(fs_devices); - if (fs_devices->seed) { - fs_devices = fs_devices->seed; - goto again; - } + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) + enqueued += __reada_start_for_fsdevs(seed_devs); if (enqueued == 0) return; diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index a0cdd027e99c..7fce7a220a76 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -350,6 +350,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid, INIT_LIST_HEAD(&fs_devs->devices); INIT_LIST_HEAD(&fs_devs->alloc_list); INIT_LIST_HEAD(&fs_devs->fs_list); + INIT_LIST_HEAD(&fs_devs->seed_list); if (fsid) memcpy(fs_devs->fsid, fsid, BTRFS_FSID_SIZE); @@ -1087,14 +1088,13 @@ void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step, void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) { struct btrfs_device *latest_dev = NULL; + struct btrfs_fs_devices *seed_dev; mutex_lock(&uuid_mutex); -again: __btrfs_free_extra_devids(fs_devices, step, &latest_dev); - if (fs_devices->seed) { - fs_devices = fs_devices->seed; - goto again; - } + + list_for_each_entry(seed_dev, &fs_devices->seed_list, seed_list) + __btrfs_free_extra_devids(seed_dev, step, &latest_dev); fs_devices->latest_bdev = latest_dev->bdev; @@ -1168,20 +1168,18 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices) void btrfs_close_devices(struct btrfs_fs_devices *fs_devices) { - struct btrfs_fs_devices *seed_devices = NULL; + LIST_HEAD(list); + struct btrfs_fs_devices *tmp; mutex_lock(&uuid_mutex); close_fs_devices(fs_devices); - if (!fs_devices->opened) { - seed_devices = fs_devices->seed; - fs_devices->seed = NULL; - } + if (!fs_devices->opened) + list_splice_init(&fs_devices->seed_list, &list); mutex_unlock(&uuid_mutex); - while (seed_devices) { - fs_devices = seed_devices; - seed_devices = fs_devices->seed; + list_for_each_entry_safe(fs_devices, tmp, &list, seed_list) { close_fs_devices(fs_devices); + list_del(&fs_devices->seed_list); free_fs_devices(fs_devices); } } @@ -2154,14 +2152,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, btrfs_free_device(device); if (cur_devices->open_devices == 0) { - while (fs_devices) { - if (fs_devices->seed == cur_devices) { - fs_devices->seed = cur_devices->seed; - break; - } - fs_devices = fs_devices->seed; - } - cur_devices->seed = NULL; + list_del_init(&cur_devices->seed_list); close_fs_devices(cur_devices); free_fs_devices(cur_devices); } @@ -2236,14 +2227,7 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_device *srcdev) ASSERT(fs_devices->seeding); tmp_fs_devices = fs_info->fs_devices; - while (tmp_fs_devices) { - if (tmp_fs_devices->seed == fs_devices) { - tmp_fs_devices->seed = fs_devices->seed; - break; - } - tmp_fs_devices = tmp_fs_devices->seed; - } - fs_devices->seed = NULL; + list_del_init(&fs_devices->seed_list); close_fs_devices(fs_devices); free_fs_devices(fs_devices); } @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) fs_devices->open_devices = 0; fs_devices->missing_devices = 0; fs_devices->rotating = false; - fs_devices->seed = seed_devices; + list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list); generate_random_uuid(fs_devices->fsid); memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE); @@ -6430,11 +6414,23 @@ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices, bool seed) { struct btrfs_device *device; + struct btrfs_fs_devices *seed_devs; - while (fs_devices) { + if (!fsid || + !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) { + list_for_each_entry(device, &fs_devices->devices, + dev_list) { + if (device->devid == devid && + (!uuid || memcmp(device->uuid, uuid, + BTRFS_UUID_SIZE) == 0)) + return device; + } + } + + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) { if (!fsid || - !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) { - list_for_each_entry(device, &fs_devices->devices, + !memcmp(seed_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE)) { + list_for_each_entry(device, &seed_devs->devices, dev_list) { if (device->devid == devid && (!uuid || memcmp(device->uuid, uuid, @@ -6442,11 +6438,8 @@ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices, return device; } } - if (seed) - fs_devices = fs_devices->seed; - else - return NULL; } + return NULL; } @@ -6688,13 +6681,10 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info, lockdep_assert_held(&uuid_mutex); ASSERT(fsid); - fs_devices = fs_info->fs_devices->seed; - while (fs_devices) { + list_for_each_entry(fs_devices, &fs_info->fs_devices->seed_list, seed_list) if (!memcmp(fs_devices->fsid, fsid, BTRFS_FSID_SIZE)) return fs_devices; - fs_devices = fs_devices->seed; - } fs_devices = find_fsid(fsid, NULL); if (!fs_devices) { @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info, goto out; } - fs_devices->seed = fs_info->fs_devices->seed; - fs_info->fs_devices->seed = fs_devices; + ASSERT(list_empty(&fs_devices->seed_list)); + list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list); out: return fs_devices; } @@ -7141,17 +7131,23 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info) void btrfs_init_devices_late(struct btrfs_fs_info *fs_info) { - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs; struct btrfs_device *device; - while (fs_devices) { - mutex_lock(&fs_devices->device_list_mutex); - list_for_each_entry(device, &fs_devices->devices, dev_list) + fs_devices->fs_info = fs_info; + + mutex_lock(&fs_devices->device_list_mutex); + list_for_each_entry(device, &fs_devices->devices, dev_list) + device->fs_info = fs_info; + mutex_unlock(&fs_devices->device_list_mutex); + + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) { + mutex_lock(&seed_devs->device_list_mutex); + list_for_each_entry(device, &seed_devs->devices, dev_list) device->fs_info = fs_info; - mutex_unlock(&fs_devices->device_list_mutex); + mutex_unlock(&seed_devs->device_list_mutex); - fs_devices->fs_info = fs_info; - fs_devices = fs_devices->seed; + seed_devs->fs_info = fs_info; } } @@ -7527,8 +7523,10 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info, /* It's possible this device is a dummy for seed device */ if (dev->disk_total_bytes == 0) { - dev = btrfs_find_device(fs_info->fs_devices->seed, devid, NULL, - NULL, false); + struct btrfs_fs_devices *devs; + devs = list_first_entry(&fs_info->fs_devices->seed_list, + struct btrfs_fs_devices, seed_list); + dev = btrfs_find_device(devs, devid, NULL, NULL, false); if (!dev) { btrfs_err(fs_info, "failed to find seed devid %llu", devid); diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index fc283fdbcece..709f4aacbd3f 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -246,7 +246,7 @@ struct btrfs_fs_devices { */ struct list_head alloc_list; - struct btrfs_fs_devices *seed; + struct list_head seed_list; bool seeding; int opened; -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] btrfs: Switch seed device to list api 2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov @ 2020-07-15 13:14 ` kernel test robot 2020-07-16 7:25 ` [PATCH v2] " Nikolay Borisov ` (2 subsequent siblings) 3 siblings, 0 replies; 31+ messages in thread From: kernel test robot @ 2020-07-15 13:14 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs; +Cc: kbuild-all, Nikolay Borisov [-- Attachment #1: Type: text/plain, Size: 5808 bytes --] Hi Nikolay, Thank you for the patch! Yet something to improve: [auto build test ERROR on kdave/for-next] [also build test ERROR on next-20200715] [cannot apply to v5.8-rc5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Convert-seed-devices-to-proper-list-API/20200715-185102 base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next config: x86_64-rhel-7.6-kselftests (attached as .config) compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All error/warnings (new ones prefixed by >>): In file included from fs/btrfs/volumes.c:17: fs/btrfs/ctree.h:2271:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers] 2271 | size_t __const btrfs_get_num_csums(void); | ^~~~~~~ In file included from fs/btrfs/volumes.c:28: fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers] 16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set); | ^~~~~ fs/btrfs/volumes.c:1030:6: warning: no previous prototype for '__btrfs_free_extra_devids' [-Wmissing-prototypes] 1030 | void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step, | ^~~~~~~~~~~~~~~~~~~~~~~~~ fs/btrfs/volumes.c: In function 'btrfs_rm_dev_replace_free_srcdev': >> fs/btrfs/volumes.c:2219:28: warning: variable 'tmp_fs_devices' set but not used [-Wunused-but-set-variable] 2219 | struct btrfs_fs_devices *tmp_fs_devices; | ^~~~~~~~~~~~~~ -- In file included from fs/btrfs/delayed-inode.h:17, from fs/btrfs/super.c:30: fs/btrfs/ctree.h:2271:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers] 2271 | size_t __const btrfs_get_num_csums(void); | ^~~~~~~ In file included from fs/btrfs/super.c:46: fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers] 16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set); | ^~~~~ fs/btrfs/super.c: In function 'btrfs_show_devname': >> fs/btrfs/super.c:2404:28: error: 'struct btrfs_fs_devices' has no member named 'seed' 2404 | cur_devices = cur_devices->seed; | ^~ vim +2404 fs/btrfs/super.c 9e7cc91a6d18a4 Wang Xiaoguang 2016-08-01 2377 9c5085c147989d Josef Bacik 2012-06-05 2378 static int btrfs_show_devname(struct seq_file *m, struct dentry *root) 9c5085c147989d Josef Bacik 2012-06-05 2379 { 9c5085c147989d Josef Bacik 2012-06-05 2380 struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); 9c5085c147989d Josef Bacik 2012-06-05 2381 struct btrfs_fs_devices *cur_devices; 9c5085c147989d Josef Bacik 2012-06-05 2382 struct btrfs_device *dev, *first_dev = NULL; 9c5085c147989d Josef Bacik 2012-06-05 2383 struct list_head *head; 9c5085c147989d Josef Bacik 2012-06-05 2384 88c14590cdd6f3 David Sterba 2018-03-16 2385 /* 88c14590cdd6f3 David Sterba 2018-03-16 2386 * Lightweight locking of the devices. We should not need 88c14590cdd6f3 David Sterba 2018-03-16 2387 * device_list_mutex here as we only read the device data and the list 88c14590cdd6f3 David Sterba 2018-03-16 2388 * is protected by RCU. Even if a device is deleted during the list 88c14590cdd6f3 David Sterba 2018-03-16 2389 * traversals, we'll get valid data, the freeing callback will wait at 52042d8e82ff50 Andrea Gelmini 2018-11-28 2390 * least until the rcu_read_unlock. 88c14590cdd6f3 David Sterba 2018-03-16 2391 */ 88c14590cdd6f3 David Sterba 2018-03-16 2392 rcu_read_lock(); 9c5085c147989d Josef Bacik 2012-06-05 2393 cur_devices = fs_info->fs_devices; 9c5085c147989d Josef Bacik 2012-06-05 2394 while (cur_devices) { 9c5085c147989d Josef Bacik 2012-06-05 2395 head = &cur_devices->devices; 88c14590cdd6f3 David Sterba 2018-03-16 2396 list_for_each_entry_rcu(dev, head, dev_list) { e6e674bd4d54fe Anand Jain 2017-12-04 2397 if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) aa9ddcd4b55571 Josef Bacik 2012-08-02 2398 continue; 0aeb8a6e67cdde Anand Jain 2014-06-30 2399 if (!dev->name) 0aeb8a6e67cdde Anand Jain 2014-06-30 2400 continue; 9c5085c147989d Josef Bacik 2012-06-05 2401 if (!first_dev || dev->devid < first_dev->devid) 9c5085c147989d Josef Bacik 2012-06-05 2402 first_dev = dev; 9c5085c147989d Josef Bacik 2012-06-05 2403 } 9c5085c147989d Josef Bacik 2012-06-05 @2404 cur_devices = cur_devices->seed; 9c5085c147989d Josef Bacik 2012-06-05 2405 } 9c5085c147989d Josef Bacik 2012-06-05 2406 672d599041c862 Misono Tomohiro 2018-08-02 2407 if (first_dev) 672d599041c862 Misono Tomohiro 2018-08-02 2408 seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); 672d599041c862 Misono Tomohiro 2018-08-02 2409 else 9c5085c147989d Josef Bacik 2012-06-05 2410 WARN_ON(1); 88c14590cdd6f3 David Sterba 2018-03-16 2411 rcu_read_unlock(); 9c5085c147989d Josef Bacik 2012-06-05 2412 return 0; 9c5085c147989d Josef Bacik 2012-06-05 2413 } 9c5085c147989d Josef Bacik 2012-06-05 2414 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 50003 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2] btrfs: Switch seed device to list api 2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov 2020-07-15 13:14 ` kernel test robot @ 2020-07-16 7:25 ` Nikolay Borisov 2020-08-18 15:19 ` Josef Bacik 2020-08-30 14:39 ` Anand Jain 2020-07-24 7:36 ` [PATCH 5/5] " Nikolay Borisov 2020-09-02 15:58 ` Anand Jain 3 siblings, 2 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-16 7:25 UTC (permalink / raw) To: linux-btrfs; +Cc: Nikolay Borisov While this patch touches a bunch of files the conversion is straighforward. Instead of using the implicit linked list anchored at btrfs_fs_devices::seed the code is switched to using list_for_each_entry. Previous patches in the series already factored out code that processed both main and seed devices so in those cases the factored out functions are called on the main fs_devices and then on every seed dev inside list_for_each_entry. Using list api also allows to simplify deletion from the seed dev list performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by substituting a while() loop with a simple list_del_init. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- V2: * Removed tmp_fs_devices from btrfs_rm_dev_replace_free_srcdev. Reported by kernel test robot fs/btrfs/disk-io.c | 39 ++++++++--------- fs/btrfs/reada.c | 9 ++-- fs/btrfs/volumes.c | 105 +++++++++++++++++++++------------------------ fs/btrfs/volumes.h | 2 +- 4 files changed, 72 insertions(+), 83 deletions(-) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 2fb0b43bc1c5..8767d47f6e29 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -545,33 +545,30 @@ static int csum_dirty_buffer(struct btrfs_fs_info *fs_info, struct page *page) static int check_tree_block_fsid(struct extent_buffer *eb) { struct btrfs_fs_info *fs_info = eb->fs_info; - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs; u8 fsid[BTRFS_FSID_SIZE]; - int ret = 1; + u8 *metadata_uuid; read_extent_buffer(eb, fsid, offsetof(struct btrfs_header, fsid), BTRFS_FSID_SIZE); - while (fs_devices) { - u8 *metadata_uuid; + /* + * Checking the incompat flag is only valid for the current + * fs. For seed devices it's forbidden to have their uuid + * changed so reading ->fsid in this case is fine + */ + if (btrfs_fs_incompat(fs_info, METADATA_UUID)) + metadata_uuid = fs_devices->metadata_uuid; + else + metadata_uuid = fs_devices->fsid; - /* - * Checking the incompat flag is only valid for the current - * fs. For seed devices it's forbidden to have their uuid - * changed so reading ->fsid in this case is fine - */ - if (fs_devices == fs_info->fs_devices && - btrfs_fs_incompat(fs_info, METADATA_UUID)) - metadata_uuid = fs_devices->metadata_uuid; - else - metadata_uuid = fs_devices->fsid; + if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE)) + return 0; - if (!memcmp(fsid, metadata_uuid, BTRFS_FSID_SIZE)) { - ret = 0; - break; - } - fs_devices = fs_devices->seed; - } - return ret; + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) + if (!memcmp(fsid, seed_devs->fsid, BTRFS_FSID_SIZE)) + return 0; + + return 1; } static int btree_readpage_end_io_hook(struct btrfs_io_bio *io_bio, diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c index aa9d24ed56d7..ac5b07ded0fe 100644 --- a/fs/btrfs/reada.c +++ b/fs/btrfs/reada.c @@ -792,16 +792,13 @@ static int __reada_start_for_fsdevs(struct btrfs_fs_devices *fs_devices) static void __reada_start_machine(struct btrfs_fs_info *fs_info) { - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs; int i; u64 enqueued = 0; -again: enqueued += __reada_start_for_fsdevs(fs_devices); - if (fs_devices->seed) { - fs_devices = fs_devices->seed; - goto again; - } + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) + enqueued += __reada_start_for_fsdevs(seed_devs); if (enqueued == 0) return; diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 70e58d724384..b05dbb448ee0 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -350,6 +350,7 @@ static struct btrfs_fs_devices *alloc_fs_devices(const u8 *fsid, INIT_LIST_HEAD(&fs_devs->devices); INIT_LIST_HEAD(&fs_devs->alloc_list); INIT_LIST_HEAD(&fs_devs->fs_list); + INIT_LIST_HEAD(&fs_devs->seed_list); if (fsid) memcpy(fs_devs->fsid, fsid, BTRFS_FSID_SIZE); @@ -1087,14 +1088,13 @@ static void __btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices, int step) { struct btrfs_device *latest_dev = NULL; + struct btrfs_fs_devices *seed_dev; mutex_lock(&uuid_mutex); -again: __btrfs_free_extra_devids(fs_devices, step, &latest_dev); - if (fs_devices->seed) { - fs_devices = fs_devices->seed; - goto again; - } + + list_for_each_entry(seed_dev, &fs_devices->seed_list, seed_list) + __btrfs_free_extra_devids(seed_dev, step, &latest_dev); fs_devices->latest_bdev = latest_dev->bdev; @@ -1168,20 +1168,18 @@ static void close_fs_devices(struct btrfs_fs_devices *fs_devices) void btrfs_close_devices(struct btrfs_fs_devices *fs_devices) { - struct btrfs_fs_devices *seed_devices = NULL; + LIST_HEAD(list); + struct btrfs_fs_devices *tmp; mutex_lock(&uuid_mutex); close_fs_devices(fs_devices); - if (!fs_devices->opened) { - seed_devices = fs_devices->seed; - fs_devices->seed = NULL; - } + if (!fs_devices->opened) + list_splice_init(&fs_devices->seed_list, &list); mutex_unlock(&uuid_mutex); - while (seed_devices) { - fs_devices = seed_devices; - seed_devices = fs_devices->seed; + list_for_each_entry_safe(fs_devices, tmp, &list, seed_list) { close_fs_devices(fs_devices); + list_del(&fs_devices->seed_list); free_fs_devices(fs_devices); } } @@ -2154,14 +2152,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path, btrfs_free_device(device); if (cur_devices->open_devices == 0) { - while (fs_devices) { - if (fs_devices->seed == cur_devices) { - fs_devices->seed = cur_devices->seed; - break; - } - fs_devices = fs_devices->seed; - } - cur_devices->seed = NULL; + list_del_init(&cur_devices->seed_list); close_fs_devices(cur_devices); free_fs_devices(cur_devices); } @@ -2225,8 +2216,6 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_device *srcdev) /* if this is no devs we rather delete the fs_devices */ if (!fs_devices->num_devices) { - struct btrfs_fs_devices *tmp_fs_devices; - /* * On a mounted FS, num_devices can't be zero unless it's a * seed. In case of a seed device being replaced, the replace @@ -2235,15 +2224,7 @@ void btrfs_rm_dev_replace_free_srcdev(struct btrfs_device *srcdev) */ ASSERT(fs_devices->seeding); - tmp_fs_devices = fs_info->fs_devices; - while (tmp_fs_devices) { - if (tmp_fs_devices->seed == fs_devices) { - tmp_fs_devices->seed = fs_devices->seed; - break; - } - tmp_fs_devices = tmp_fs_devices->seed; - } - fs_devices->seed = NULL; + list_del_init(&fs_devices->seed_list); close_fs_devices(fs_devices); free_fs_devices(fs_devices); } @@ -2397,7 +2378,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) fs_devices->open_devices = 0; fs_devices->missing_devices = 0; fs_devices->rotating = false; - fs_devices->seed = seed_devices; + list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list); generate_random_uuid(fs_devices->fsid); memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE); @@ -6430,11 +6411,23 @@ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices, bool seed) { struct btrfs_device *device; + struct btrfs_fs_devices *seed_devs; + + if (!fsid || + !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) { + list_for_each_entry(device, &fs_devices->devices, + dev_list) { + if (device->devid == devid && + (!uuid || memcmp(device->uuid, uuid, + BTRFS_UUID_SIZE) == 0)) + return device; + } + } - while (fs_devices) { + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) { if (!fsid || - !memcmp(fs_devices->metadata_uuid, fsid, BTRFS_FSID_SIZE)) { - list_for_each_entry(device, &fs_devices->devices, + !memcmp(seed_devs->metadata_uuid, fsid, BTRFS_FSID_SIZE)) { + list_for_each_entry(device, &seed_devs->devices, dev_list) { if (device->devid == devid && (!uuid || memcmp(device->uuid, uuid, @@ -6442,11 +6435,8 @@ struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices, return device; } } - if (seed) - fs_devices = fs_devices->seed; - else - return NULL; } + return NULL; } @@ -6688,13 +6678,10 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info, lockdep_assert_held(&uuid_mutex); ASSERT(fsid); - fs_devices = fs_info->fs_devices->seed; - while (fs_devices) { + list_for_each_entry(fs_devices, &fs_info->fs_devices->seed_list, seed_list) if (!memcmp(fs_devices->fsid, fsid, BTRFS_FSID_SIZE)) return fs_devices; - fs_devices = fs_devices->seed; - } fs_devices = find_fsid(fsid, NULL); if (!fs_devices) { @@ -6728,8 +6715,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info, goto out; } - fs_devices->seed = fs_info->fs_devices->seed; - fs_info->fs_devices->seed = fs_devices; + ASSERT(list_empty(&fs_devices->seed_list)); + list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list); out: return fs_devices; } @@ -7141,17 +7128,23 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info) void btrfs_init_devices_late(struct btrfs_fs_info *fs_info) { - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs; struct btrfs_device *device; - while (fs_devices) { - mutex_lock(&fs_devices->device_list_mutex); - list_for_each_entry(device, &fs_devices->devices, dev_list) + fs_devices->fs_info = fs_info; + + mutex_lock(&fs_devices->device_list_mutex); + list_for_each_entry(device, &fs_devices->devices, dev_list) + device->fs_info = fs_info; + mutex_unlock(&fs_devices->device_list_mutex); + + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) { + mutex_lock(&seed_devs->device_list_mutex); + list_for_each_entry(device, &seed_devs->devices, dev_list) device->fs_info = fs_info; - mutex_unlock(&fs_devices->device_list_mutex); + mutex_unlock(&seed_devs->device_list_mutex); - fs_devices->fs_info = fs_info; - fs_devices = fs_devices->seed; + seed_devs->fs_info = fs_info; } } @@ -7527,8 +7520,10 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info, /* It's possible this device is a dummy for seed device */ if (dev->disk_total_bytes == 0) { - dev = btrfs_find_device(fs_info->fs_devices->seed, devid, NULL, - NULL, false); + struct btrfs_fs_devices *devs; + devs = list_first_entry(&fs_info->fs_devices->seed_list, + struct btrfs_fs_devices, seed_list); + dev = btrfs_find_device(devs, devid, NULL, NULL, false); if (!dev) { btrfs_err(fs_info, "failed to find seed devid %llu", devid); diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index fc283fdbcece..709f4aacbd3f 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -246,7 +246,7 @@ struct btrfs_fs_devices { */ struct list_head alloc_list; - struct btrfs_fs_devices *seed; + struct list_head seed_list; bool seeding; int opened; -- 2.17.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2] btrfs: Switch seed device to list api 2020-07-16 7:25 ` [PATCH v2] " Nikolay Borisov @ 2020-08-18 15:19 ` Josef Bacik 2020-08-30 14:39 ` Anand Jain 1 sibling, 0 replies; 31+ messages in thread From: Josef Bacik @ 2020-08-18 15:19 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 7/16/20 3:25 AM, Nikolay Borisov wrote: > While this patch touches a bunch of files the conversion is > straighforward. Instead of using the implicit linked list anchored at > btrfs_fs_devices::seed the code is switched to using > list_for_each_entry. Previous patches in the series already factored out > code that processed both main and seed devices so in those cases > the factored out functions are called on the main fs_devices and then > on every seed dev inside list_for_each_entry. > > Using list api also allows to simplify deletion from the seed dev list > performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by > substituting a while() loop with a simple list_del_init. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2] btrfs: Switch seed device to list api 2020-07-16 7:25 ` [PATCH v2] " Nikolay Borisov 2020-08-18 15:19 ` Josef Bacik @ 2020-08-30 14:39 ` Anand Jain 1 sibling, 0 replies; 31+ messages in thread From: Anand Jain @ 2020-08-30 14:39 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 16/7/20 3:25 pm, Nikolay Borisov wrote: > While this patch touches a bunch of files the conversion is > straighforward. Instead of using the implicit linked list anchored at > btrfs_fs_devices::seed the code is switched to using > list_for_each_entry. Previous patches in the series already factored out > code that processed both main and seed devices so in those cases > the factored out functions are called on the main fs_devices and then > on every seed dev inside list_for_each_entry. > > Using list api also allows to simplify deletion from the seed dev list > performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by > substituting a while() loop with a simple list_del_init. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> > --- > > V2: > * Removed tmp_fs_devices from btrfs_rm_dev_replace_free_srcdev. Reported by > kernel test robot > Reviewed-by: Anand Jain <anand.jain@oracle.com> Thanks, Anand ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] btrfs: Switch seed device to list api 2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov 2020-07-15 13:14 ` kernel test robot 2020-07-16 7:25 ` [PATCH v2] " Nikolay Borisov @ 2020-07-24 7:36 ` Nikolay Borisov 2020-09-02 15:58 ` Anand Jain 3 siblings, 0 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-07-24 7:36 UTC (permalink / raw) To: linux-btrfs On 15.07.20 г. 13:48 ч., Nikolay Borisov wrote: > While this patch touches a bunch of files the conversion is > straighforward. Instead of using the implicit linked list anchored at > btrfs_fs_devices::seed the code is switched to using > list_for_each_entry. Previous patches in the series already factored out > code that processed both main and seed devices so in those cases > the factored out functions are called on the main fs_devices and then > on every seed dev inside list_for_each_entry. > > Using list api also allows to simplify deletion from the seed dev list > performed in btrfs_rm_device and btrfs_rm_dev_replace_free_srcdev by > substituting a while() loop with a simple list_del_init. > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> <snip> > @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info, > goto out; > } > > - fs_devices->seed = fs_info->fs_devices->seed; > - fs_info->fs_devices->seed = fs_devices; > + ASSERT(list_empty(&fs_devices->seed_list)); That assert is useless, since fs_devices at this point are a clone from clone_fs_devices hence its seed_list cannot be non-empty. Consider this when removing merging it. > + list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list); > out: > return fs_devices; > } > @@ -7141,17 +7131,23 @@ int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info) > > void btrfs_init_devices_late(struct btrfs_fs_info *fs_info) > { > - struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; > + struct btrfs_fs_devices *fs_devices = fs_info->fs_devices, *seed_devs; > struct btrfs_device *device; > > - while (fs_devices) { > - mutex_lock(&fs_devices->device_list_mutex); > - list_for_each_entry(device, &fs_devices->devices, dev_list) > + fs_devices->fs_info = fs_info; > + > + mutex_lock(&fs_devices->device_list_mutex); > + list_for_each_entry(device, &fs_devices->devices, dev_list) > + device->fs_info = fs_info; > + mutex_unlock(&fs_devices->device_list_mutex); > + > + list_for_each_entry(seed_devs, &fs_devices->seed_list, seed_list) { > + mutex_lock(&seed_devs->device_list_mutex); > + list_for_each_entry(device, &seed_devs->devices, dev_list) > device->fs_info = fs_info; > - mutex_unlock(&fs_devices->device_list_mutex); > + mutex_unlock(&seed_devs->device_list_mutex); > > - fs_devices->fs_info = fs_info; > - fs_devices = fs_devices->seed; > + seed_devs->fs_info = fs_info; > } > } > > @@ -7527,8 +7523,10 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info, > > /* It's possible this device is a dummy for seed device */ > if (dev->disk_total_bytes == 0) { > - dev = btrfs_find_device(fs_info->fs_devices->seed, devid, NULL, > - NULL, false); > + struct btrfs_fs_devices *devs; > + devs = list_first_entry(&fs_info->fs_devices->seed_list, > + struct btrfs_fs_devices, seed_list); > + dev = btrfs_find_device(devs, devid, NULL, NULL, false); > if (!dev) { > btrfs_err(fs_info, "failed to find seed devid %llu", > devid); > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index fc283fdbcece..709f4aacbd3f 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -246,7 +246,7 @@ struct btrfs_fs_devices { > */ > struct list_head alloc_list; > > - struct btrfs_fs_devices *seed; > + struct list_head seed_list; > bool seeding; > > int opened; > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] btrfs: Switch seed device to list api 2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov ` (2 preceding siblings ...) 2020-07-24 7:36 ` [PATCH 5/5] " Nikolay Borisov @ 2020-09-02 15:58 ` Anand Jain 2020-09-03 9:03 ` Nikolay Borisov 3 siblings, 1 reply; 31+ messages in thread From: Anand Jain @ 2020-09-02 15:58 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs The seed of the current sprout should rather be at the head instead of at the bottom. > @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct btrfs_fs_info *fs_info) > fs_devices->open_devices = 0; > fs_devices->missing_devices = 0; > fs_devices->rotating = false; > - fs_devices->seed = seed_devices; > + list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list); It should be list_add_head. > > generate_random_uuid(fs_devices->fsid); > memcpy(fs_devices->metadata_uuid, fs_devices->fsid, BTRFS_FSID_SIZE); > @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices *open_seed_devices(struct btrfs_fs_info *fs_info, > goto out; > } > > - fs_devices->seed = fs_info->fs_devices->seed; > - fs_info->fs_devices->seed = fs_devices; > + ASSERT(list_empty(&fs_devices->seed_list)); > + list_add_tail(&fs_devices->seed_list, &fs_info->fs_devices->seed_list); It should be list_add_head. > out: > return fs_devices; > } Thanks, Anand ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] btrfs: Switch seed device to list api 2020-09-02 15:58 ` Anand Jain @ 2020-09-03 9:03 ` Nikolay Borisov 2020-09-03 9:33 ` Anand Jain 0 siblings, 1 reply; 31+ messages in thread From: Nikolay Borisov @ 2020-09-03 9:03 UTC (permalink / raw) To: Anand Jain, linux-btrfs On 2.09.20 г. 18:58 ч., Anand Jain wrote: > > > The seed of the current sprout should rather be at the head instead of > at the bottom. > > >> @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct >> btrfs_fs_info *fs_info) >> fs_devices->open_devices = 0; >> fs_devices->missing_devices = 0; >> fs_devices->rotating = false; >> - fs_devices->seed = seed_devices; >> + list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list); > > It should be list_add_head. Generally yes, but in this case I don't think it makes any functional differences so even adding at the tail is fine. > >> generate_random_uuid(fs_devices->fsid); >> memcpy(fs_devices->metadata_uuid, fs_devices->fsid, >> BTRFS_FSID_SIZE); > > > > >> @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices >> *open_seed_devices(struct btrfs_fs_info *fs_info, >> goto out; >> } >> - fs_devices->seed = fs_info->fs_devices->seed; >> - fs_info->fs_devices->seed = fs_devices; >> + ASSERT(list_empty(&fs_devices->seed_list)); >> + list_add_tail(&fs_devices->seed_list, >> &fs_info->fs_devices->seed_list); > > It should be list_add_head. > >> out: >> return fs_devices; >> } > > > Thanks, Anand > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] btrfs: Switch seed device to list api 2020-09-03 9:03 ` Nikolay Borisov @ 2020-09-03 9:33 ` Anand Jain 2020-09-10 16:28 ` David Sterba 0 siblings, 1 reply; 31+ messages in thread From: Anand Jain @ 2020-09-03 9:33 UTC (permalink / raw) To: Nikolay Borisov, linux-btrfs On 3/9/20 5:03 pm, Nikolay Borisov wrote: > > > On 2.09.20 г. 18:58 ч., Anand Jain wrote: >> >> >> The seed of the current sprout should rather be at the head instead of >> at the bottom. >> >> >>> @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct >>> btrfs_fs_info *fs_info) >>> fs_devices->open_devices = 0; >>> fs_devices->missing_devices = 0; >>> fs_devices->rotating = false; >>> - fs_devices->seed = seed_devices; >>> + list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list); >> >> It should be list_add_head. > > Generally yes, but in this case I don't think it makes any functional > differences so even adding at the tail is fine. > Hm No. Adding to the head matches to the order of dependency. As it was in the while loop. Thanks, Anand >> >>> generate_random_uuid(fs_devices->fsid); >>> memcpy(fs_devices->metadata_uuid, fs_devices->fsid, >>> BTRFS_FSID_SIZE); >> >> >> >> >>> @@ -6728,8 +6718,8 @@ static struct btrfs_fs_devices >>> *open_seed_devices(struct btrfs_fs_info *fs_info, >>> goto out; >>> } >>> - fs_devices->seed = fs_info->fs_devices->seed; >>> - fs_info->fs_devices->seed = fs_devices; >>> + ASSERT(list_empty(&fs_devices->seed_list)); >>> + list_add_tail(&fs_devices->seed_list, >>> &fs_info->fs_devices->seed_list); >> >> It should be list_add_head. >> >>> out: >>> return fs_devices; >>> } >> >> >> Thanks, Anand >> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] btrfs: Switch seed device to list api 2020-09-03 9:33 ` Anand Jain @ 2020-09-10 16:28 ` David Sterba 0 siblings, 0 replies; 31+ messages in thread From: David Sterba @ 2020-09-10 16:28 UTC (permalink / raw) To: Anand Jain; +Cc: Nikolay Borisov, linux-btrfs On Thu, Sep 03, 2020 at 05:33:26PM +0800, Anand Jain wrote: > > > On 3/9/20 5:03 pm, Nikolay Borisov wrote: > > > > > > On 2.09.20 г. 18:58 ч., Anand Jain wrote: > >> > >> > >> The seed of the current sprout should rather be at the head instead of > >> at the bottom. > >> > >> > >>> @@ -2397,7 +2381,7 @@ static int btrfs_prepare_sprout(struct > >>> btrfs_fs_info *fs_info) > >>> fs_devices->open_devices = 0; > >>> fs_devices->missing_devices = 0; > >>> fs_devices->rotating = false; > >>> - fs_devices->seed = seed_devices; > >>> + list_add_tail(&seed_devices->seed_list, &fs_devices->seed_list); > >> > >> It should be list_add_head. > > > > Generally yes, but in this case I don't think it makes any functional > > differences so even adding at the tail is fine. > > > > Hm No. Adding to the head matches to the order of dependency. As it > was in the while loop. Following the same order sounds like a better idea to me. If there's really no differece and we want to add the entry to the tail, then it would be another patch with proper reasoning. I'll update it to do list_head. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/5] Convert seed devices to proper list API 2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov ` (4 preceding siblings ...) 2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov @ 2020-07-22 14:26 ` David Sterba 2020-07-23 8:02 ` Nikolay Borisov 2020-08-17 19:19 ` Nikolay Borisov 6 siblings, 1 reply; 31+ messages in thread From: David Sterba @ 2020-07-22 14:26 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs On Wed, Jul 15, 2020 at 01:48:45PM +0300, Nikolay Borisov wrote: > This series converts the existing seed devices pointer in btrfs_fs_devices to > proper list api. First 4 patches are cleanups preparing the code for the switch. > Patch 5 has more information about the required transformations, it might seem > lengthy but he logic everywhere is pretty much the same so shouldn't be that > cumbersome to review. The first 3 patches are clear, I start to have doubts in 4 and 5 was a stop for me, but I'm not saying it's wrong. Just that I always thought the seed devices + the sprout are close to a tree structure but you're switching that to a linear list. I'll add the first three patches now, but would appreciate some help with 4 and 5. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/5] Convert seed devices to proper list API 2020-07-22 14:26 ` [PATCH 0/5] Convert seed devices to proper list API David Sterba @ 2020-07-23 8:02 ` Nikolay Borisov 2020-08-21 14:33 ` David Sterba 0 siblings, 1 reply; 31+ messages in thread From: Nikolay Borisov @ 2020-07-23 8:02 UTC (permalink / raw) To: dsterba, linux-btrfs On 22.07.20 г. 17:26 ч., David Sterba wrote: > On Wed, Jul 15, 2020 at 01:48:45PM +0300, Nikolay Borisov wrote: >> This series converts the existing seed devices pointer in btrfs_fs_devices to >> proper list api. First 4 patches are cleanups preparing the code for the switch. > >> Patch 5 has more information about the required transformations, it might seem >> lengthy but he logic everywhere is pretty much the same so shouldn't be that >> cumbersome to review. > > The first 3 patches are clear, I start to have doubts in 4 and 5 was a > stop for me, but I'm not saying it's wrong. Just that I always thought > the seed devices + the sprout are close to a tree structure but you're > switching that to a linear list. > > I'll add the first three patches now, but would appreciate some help > with 4 and 5. > Ok, let's take it one at a time, patch 4: Originally fs_info for fs_devices is set by btrfs_set_fs_info_ptr called by btrfs_sysfs_add_mounted. THe latter is called in open_ctree before block_groups are read i.e on before the fs is exposed. My patch instead changes this so that fs_info is set in btrfs_init_devices_late, called from btrfs_read_roots, called by init_tree_roots. The last function is called before btrfs_sysfs_add_mounted. This means as far as setting fs_devices is concerned its fs_info is being set earlier. And this is correct. Now, let's think about clearing it, the old code cleared it in : btrfs_sysfs_remove_mounted which is called from: 1. fail_sysfs label in open_ctree 2. close_ctree 3. failure in btrfs_sysfs_add_mounted which does goto fail_fsdev_sysfs So how are those 3 situation handled by the new code : 1 and 3 - the last function called in open_ctree is btrfs_close_devices in case of errors and both failure labels are before it, so we are guaranteed that on failure in open_ctree fs_devices will have it's fs_info being NULL. 2. Again, last function called in close_ctree is btrfs_close_devices so we are guaranteed on unmount fs_devices will have fs_info being NULL. Thus the patch doesn't introduce any functional changes but IMO makes the code more coherent. Regarding patch 5 - I don't know what made you think there is a tree-like structure involved. Simply looking at the old way seeds are iterated: while (seed_devices) { fs_devices = seed_devices; seed_devices = fs_devices->seed; close_fs_devices(fs_devices); free_fs_devices(fs_devices); } There's no conditional deciding if we should go left/right of the tree. Or let's take for example deleting from a list of seed devices: tmp_fs_devices = fs_info->fs_devices; while (tmp_fs_devices) { if (tmp_fs_devices->seed == fs_devices) { tmp_fs_devices->seed = fs_devices->seed; break; } tmp_fs_devices = tmp_fs_devices->seed; } Here a simple linear search is performed and when a member of the seed list matches our fs_devices it simply pointed 1 past our fs_devices When the if inside the loop matches we get the following situation: [tmp_fs_devices]->[fs_devices]->[fs_devices->seeds] Then we perform [tmp_fs_devices] -> [fs_devices->seed] So a simple deletion, just the fact you were confused shows the old code is rather wonky. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/5] Convert seed devices to proper list API 2020-07-23 8:02 ` Nikolay Borisov @ 2020-08-21 14:33 ` David Sterba 0 siblings, 0 replies; 31+ messages in thread From: David Sterba @ 2020-08-21 14:33 UTC (permalink / raw) To: Nikolay Borisov; +Cc: dsterba, linux-btrfs On Thu, Jul 23, 2020 at 11:02:14AM +0300, Nikolay Borisov wrote: > Regarding patch 5 - I don't know what made you think there is a > tree-like structure involved. Simply looking at the old way seeds are > iterated: > > while (seed_devices) { > fs_devices = seed_devices; > seed_devices = fs_devices->seed; > close_fs_devices(fs_devices); > free_fs_devices(fs_devices); > } > > There's no conditional deciding if we should go left/right of the tree. A tree structure that has only one direction arrows, from leaves to the root. Where the root is the seed fs and the leaves are the sprouts. fs1 ---> seed ^ ^ fs2 -----| | | fs3 -------| The while loop in each fs goes by the pointers up to the seeding device and always removes it's references. I'm not sure about a deeper tree structure here, so the fs3 -> fs2 would be possible. > Or let's take for example deleting from a list of seed devices: > > tmp_fs_devices = fs_info->fs_devices; > while (tmp_fs_devices) { > if (tmp_fs_devices->seed == fs_devices) { > tmp_fs_devices->seed = fs_devices->seed; > break; > } > tmp_fs_devices = tmp_fs_devices->seed; > } > > Here a simple linear search is performed and when a member of the seed > list matches our fs_devices it simply pointed 1 past our fs_devices > > When the if inside the loop matches we get the following situation: > > [tmp_fs_devices]->[fs_devices]->[fs_devices->seeds] > > Then we perform [tmp_fs_devices] -> [fs_devices->seed] > > So a simple deletion, just the fact you were confused shows the old code > is rather wonky. Well, I still am after reading the above, the missing part is about the device cloning and that each fs has it's own copy. The tree structure implies sharing and would need reference counting, but this does not seem to be so. I'll add the series to for-next so we have a base for the other seeding patches but I haven't reviewed it to the point where I'm convinced it's correct. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/5] Convert seed devices to proper list API 2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov ` (5 preceding siblings ...) 2020-07-22 14:26 ` [PATCH 0/5] Convert seed devices to proper list API David Sterba @ 2020-08-17 19:19 ` Nikolay Borisov 6 siblings, 0 replies; 31+ messages in thread From: Nikolay Borisov @ 2020-08-17 19:19 UTC (permalink / raw) To: linux-btrfs On 15.07.20 г. 13:48 ч., Nikolay Borisov wrote: > This series converts the existing seed devices pointer in btrfs_fs_devices to > proper list api. First 4 patches are cleanups preparing the code for the switch. > Patch 5 has more information about the required transformations, it might seem > lengthy but he logic everywhere is pretty much the same so shouldn't be that > cumbersome to review. > > This series survived xfstest runs and even caught 2 bugs while developing it. > > Nikolay Borisov (5): > btrfs: Factor out reada loop in __reada_start_machine > btrfs: Factor out loop logic from btrfs_free_extra_devids > btrfs: Make close_fs_devices return void > btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices > btrfs: Switch seed device to list api > > fs/btrfs/disk-io.c | 39 +++++------ > fs/btrfs/reada.c | 26 ++++--- > fs/btrfs/sysfs.c | 4 -- > fs/btrfs/volumes.c | 166 ++++++++++++++++++++------------------------- > fs/btrfs/volumes.h | 6 +- > 5 files changed, 112 insertions(+), 129 deletions(-) > > -- > 2.17.1 > Gentle ping ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-09-10 16:44 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-15 10:48 [PATCH 0/5] Convert seed devices to proper list API Nikolay Borisov 2020-07-15 10:48 ` [PATCH 1/5] btrfs: Factor out reada loop in __reada_start_machine Nikolay Borisov 2020-08-18 15:02 ` Josef Bacik 2020-08-29 15:06 ` Anand Jain 2020-08-31 12:24 ` David Sterba 2020-07-15 10:48 ` [PATCH 2/5] btrfs: Factor out loop logic from btrfs_free_extra_devids Nikolay Borisov 2020-07-15 12:32 ` kernel test robot 2020-07-15 12:39 ` Nikolay Borisov 2020-07-16 7:17 ` [PATCH v2] " Nikolay Borisov 2020-08-29 15:13 ` Anand Jain 2020-08-18 15:03 ` [PATCH 2/5] " Josef Bacik 2020-07-15 10:48 ` [PATCH 3/5] btrfs: Make close_fs_devices return void Nikolay Borisov 2020-08-18 15:05 ` Josef Bacik 2020-08-29 15:14 ` Anand Jain 2020-07-15 10:48 ` [PATCH 4/5] btrfs: Simplify setting/clearing fs_info to btrfs_fs_devices Nikolay Borisov 2020-08-18 15:08 ` Josef Bacik 2020-08-26 10:50 ` Anand Jain 2020-07-15 10:48 ` [PATCH 5/5] btrfs: Switch seed device to list api Nikolay Borisov 2020-07-15 13:14 ` kernel test robot 2020-07-16 7:25 ` [PATCH v2] " Nikolay Borisov 2020-08-18 15:19 ` Josef Bacik 2020-08-30 14:39 ` Anand Jain 2020-07-24 7:36 ` [PATCH 5/5] " Nikolay Borisov 2020-09-02 15:58 ` Anand Jain 2020-09-03 9:03 ` Nikolay Borisov 2020-09-03 9:33 ` Anand Jain 2020-09-10 16:28 ` David Sterba 2020-07-22 14:26 ` [PATCH 0/5] Convert seed devices to proper list API David Sterba 2020-07-23 8:02 ` Nikolay Borisov 2020-08-21 14:33 ` David Sterba 2020-08-17 19:19 ` Nikolay Borisov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).