Message ID | 1530175695-301-1-git-send-email-nborisov@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Nikolay, Thank you for the patch! Yet something to improve: [auto build test ERROR on v4.18-rc2] [also build test ERROR on next-20180628] [cannot apply to btrfs/next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Revert-btrfs-fix-a-possible-umount-deadlock/20180629-044154 config: i386-randconfig-a0-201825 (attached as .config) compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): fs//btrfs/volumes.c: In function 'btrfs_close_one_device': >> fs//btrfs/volumes.c:1041:25: error: 'free_device' undeclared (first use in this function) call_rcu(&device->rcu, free_device); ^ fs//btrfs/volumes.c:1041:25: note: each undeclared identifier is reported only once for each function it appears in vim +/free_device +1041 fs//btrfs/volumes.c 1006 1007 static void btrfs_close_one_device(struct btrfs_device *device) 1008 { 1009 struct btrfs_fs_devices *fs_devices = device->fs_devices; 1010 struct btrfs_device *new_device; 1011 struct rcu_string *name; 1012 1013 if (device->bdev) 1014 fs_devices->open_devices--; 1015 1016 if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) && 1017 device->devid != BTRFS_DEV_REPLACE_DEVID) { 1018 list_del_init(&device->dev_alloc_list); 1019 fs_devices->rw_devices--; 1020 } 1021 1022 if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state)) 1023 fs_devices->missing_devices--; 1024 1025 btrfs_close_bdev(device); 1026 1027 new_device = btrfs_alloc_device(NULL, &device->devid, 1028 device->uuid); 1029 BUG_ON(IS_ERR(new_device)); /* -ENOMEM */ 1030 1031 /* Safe because we are under uuid_mutex */ 1032 if (device->name) { 1033 name = rcu_string_strdup(device->name->str, GFP_NOFS); 1034 BUG_ON(!name); /* -ENOMEM */ 1035 rcu_assign_pointer(new_device->name, name); 1036 } 1037 1038 list_replace_rcu(&device->dev_list, &new_device->dev_list); 1039 new_device->fs_devices = device->fs_devices; 1040 > 1041 call_rcu(&device->rcu, free_device); 1042 } 1043 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Nikolay, Thank you for the patch! Yet something to improve: [auto build test ERROR on v4.18-rc2] [also build test ERROR on next-20180628] [cannot apply to btrfs/next] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Nikolay-Borisov/Revert-btrfs-fix-a-possible-umount-deadlock/20180629-044154 config: x86_64-randconfig-x015-201825 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): fs/btrfs/volumes.c: In function 'btrfs_close_one_device': >> fs/btrfs/volumes.c:1041:25: error: 'free_device' undeclared (first use in this function); did you mean 'new_device'? call_rcu(&device->rcu, free_device); ^~~~~~~~~~~ new_device fs/btrfs/volumes.c:1041:25: note: each undeclared identifier is reported only once for each function it appears in vim +1041 fs/btrfs/volumes.c 1006 1007 static void btrfs_close_one_device(struct btrfs_device *device) 1008 { 1009 struct btrfs_fs_devices *fs_devices = device->fs_devices; 1010 struct btrfs_device *new_device; 1011 struct rcu_string *name; 1012 1013 if (device->bdev) 1014 fs_devices->open_devices--; 1015 1016 if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) && 1017 device->devid != BTRFS_DEV_REPLACE_DEVID) { 1018 list_del_init(&device->dev_alloc_list); 1019 fs_devices->rw_devices--; 1020 } 1021 1022 if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state)) 1023 fs_devices->missing_devices--; 1024 1025 btrfs_close_bdev(device); 1026 1027 new_device = btrfs_alloc_device(NULL, &device->devid, 1028 device->uuid); 1029 BUG_ON(IS_ERR(new_device)); /* -ENOMEM */ 1030 1031 /* Safe because we are under uuid_mutex */ 1032 if (device->name) { 1033 name = rcu_string_strdup(device->name->str, GFP_NOFS); 1034 BUG_ON(!name); /* -ENOMEM */ 1035 rcu_assign_pointer(new_device->name, name); 1036 } 1037 1038 list_replace_rcu(&device->dev_list, &new_device->dev_list); 1039 new_device->fs_devices = device->fs_devices; 1040 > 1041 call_rcu(&device->rcu, free_device); 1042 } 1043 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 5bd6f3a40f9c..70b0ed2ba4df 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1004,7 +1004,7 @@ static void btrfs_close_bdev(struct btrfs_device *device) blkdev_put(device->bdev, device->mode); } -static void btrfs_prepare_close_one_device(struct btrfs_device *device) +static void btrfs_close_one_device(struct btrfs_device *device) { struct btrfs_fs_devices *fs_devices = device->fs_devices; struct btrfs_device *new_device; @@ -1022,6 +1022,8 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device) if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state)) fs_devices->missing_devices--; + btrfs_close_bdev(device); + new_device = btrfs_alloc_device(NULL, &device->devid, device->uuid); BUG_ON(IS_ERR(new_device)); /* -ENOMEM */ @@ -1035,39 +1037,23 @@ static void btrfs_prepare_close_one_device(struct btrfs_device *device) list_replace_rcu(&device->dev_list, &new_device->dev_list); new_device->fs_devices = device->fs_devices; + + call_rcu(&device->rcu, free_device); } static int close_fs_devices(struct btrfs_fs_devices *fs_devices) { struct btrfs_device *device, *tmp; - struct list_head pending_put; - - INIT_LIST_HEAD(&pending_put); if (--fs_devices->opened > 0) return 0; mutex_lock(&fs_devices->device_list_mutex); list_for_each_entry_safe(device, tmp, &fs_devices->devices, dev_list) { - btrfs_prepare_close_one_device(device); - list_add(&device->dev_list, &pending_put); + btrfs_close_one_device(device); } mutex_unlock(&fs_devices->device_list_mutex); - /* - * btrfs_show_devname() is using the device_list_mutex, - * sometimes call to blkdev_put() leads vfs calling - * into this func. So do put outside of device_list_mutex, - * as of now. - */ - while (!list_empty(&pending_put)) { - device = list_first_entry(&pending_put, - struct btrfs_device, dev_list); - list_del(&device->dev_list); - btrfs_close_bdev(device); - call_rcu(&device->rcu, free_device_rcu); - } - WARN_ON(fs_devices->open_devices); WARN_ON(fs_devices->rw_devices); fs_devices->opened = 0;
Since commit 88c14590cdd6 ("btrfs: use RCU in btrfs_show_devname for device list traversal") btrfs_show_devname no longer takes device_list_mutex. As such the deadlock that 0ccd05285e7f ("btrfs: fix a possible umount deadlock") aimed to fix no longer exists. So remove the extra code this commit added. No functional changes. This reverts commit 0ccd05285e7f5a8e297e1d6dfc41e7c65757d6fa. Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- Anand, I would like to use the opportunity to ask you about the peculiar sequence needed to close btrfs devices. Why do we first replace the closed device with a copy in btrfs_close_one_device, then dispose of the copied devices in btrfs_close_devices IFF we had fs_devices->seed not being NULL? fs/btrfs/volumes.c | 26 ++++++-------------------- 1 file changed, 6 insertions(+), 20 deletions(-)