| Message ID | 9a06b04b9003f86c3300e497b35b0ef0310c84c0.1629396187.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrf_show_devname related fixes | expand |
On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com> wrote: > latest_bdev is updated according to the changes to the device > list. > That means we could use the latest_bdev to show the device name > in > /proc/self/mounts. So this patch makes that change. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > > RFC because > 1. latest_bdev might not be the lowest devid but, we showed > the lowest devid in /proc/self/mount. > 2. The device's path is not shown now but, previously we did. > So does these break ABI? Maybe yes for 2 howabout for 1 above? > Mabybe a naive question but I have no time to dive into btrfs code recently. If a device which has highest devid was replaced, will the new device be the fs_devices->latest_bdev instead of the existing older one? Thanks. -- Su > fs/btrfs/super.c | 25 +++---------------------- > 1 file changed, 3 insertions(+), 22 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 1f9dd1a4faa3..4ad3fe174c41 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct > super_block *sb) > static int btrfs_show_devname(struct seq_file *m, struct dentry > *root) > { > struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); > - struct btrfs_device *dev, *first_dev = NULL; > + char name[BDEVNAME_SIZE]; > > - /* > - * Lightweight locking of the devices. We should not need > - * device_list_mutex here as we only read the device data and > the list > - * is protected by RCU. Even if a device is deleted during > the list > - * traversals, we'll get valid data, the freeing callback will > wait at > - * least until the rcu_read_unlock. > - */ > - rcu_read_lock(); > - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, > dev_list) { > - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) > - continue; > - if (!dev->name) > - continue; > - if (!first_dev || dev->devid < first_dev->devid) > - first_dev = dev; > - } > + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, > name), > + " \t\n\\"); > > - if (first_dev) > - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); > - else > - WARN_ON(1); > - rcu_read_unlock(); > return 0; > }
On 20/08/2021 15:31, Su Yue wrote: > > On Fri 20 Aug 2021 at 02:18, Anand Jain <anand.jain@oracle.com> wrote: > >> latest_bdev is updated according to the changes to the device list. >> That means we could use the latest_bdev to show the device name in >> /proc/self/mounts. So this patch makes that change. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> >> RFC because >> 1. latest_bdev might not be the lowest devid but, we showed >> the lowest devid in /proc/self/mount. >> 2. The device's path is not shown now but, previously we did. >> So does these break ABI? Maybe yes for 2 howabout for 1 above? In v2 I am fix the path part. By replacing latest_bdev to latest_dev which means it shall hold the pointer to btrfs_device instead of just to its bdev. > Mabybe a naive question but I have no time to dive into btrfs code > recently. If a device which has highest devid was replaced, will the > new device be the fs_devices->latest_bdev instead of the existing older > one? It is handled by btrfs_assign_next_active_device(). If the replace source device is also the latest_dev then after replace it shall point to the replace target device. So the new device path is seen in /proc/self/mounts. Which is fine and normal to expect. Similarly btrfs_assign_next_active_device() is called during remove device as well. There is a bug that we didn't update the latest_bdev when we sprout I am fixing this as well in v2. Thanks.
On Fri, Aug 20, 2021 at 02:18:14AM +0800, Anand Jain wrote: > latest_bdev is updated according to the changes to the device list. > That means we could use the latest_bdev to show the device name in > /proc/self/mounts. So this patch makes that change. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > > RFC because > 1. latest_bdev might not be the lowest devid but, we showed > the lowest devid in /proc/self/mount. > 2. The device's path is not shown now but, previously we did. > So does these break ABI? Maybe yes for 2 howabout for 1 above? The path needs to be preserved, that would break a lot of things.. > fs/btrfs/super.c | 25 +++---------------------- > 1 file changed, 3 insertions(+), 22 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 1f9dd1a4faa3..4ad3fe174c41 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb) > static int btrfs_show_devname(struct seq_file *m, struct dentry *root) > { > struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); > - struct btrfs_device *dev, *first_dev = NULL; > + char name[BDEVNAME_SIZE]; > > - /* > - * Lightweight locking of the devices. We should not need > - * device_list_mutex here as we only read the device data and the list > - * is protected by RCU. Even if a device is deleted during the list > - * traversals, we'll get valid data, the freeing callback will wait at > - * least until the rcu_read_unlock. > - */ > - rcu_read_lock(); > - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) { > - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) > - continue; > - if (!dev->name) > - continue; > - if (!first_dev || dev->devid < first_dev->devid) > - first_dev = dev; > - } > + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name), > + " \t\n\\"); No protection at all? So what if latest_bdev or latest_dev gets updated in parallel with devicre remove and there's a window where the pointer is invalid but still is accessed. The whole point of RCU section here is to prevent that. > > - if (first_dev) > - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); > - else > - WARN_ON(1); > - rcu_read_unlock(); > return 0; > } > > -- > 2.31.1
On 20/08/2021 18:57, David Sterba wrote: > On Fri, Aug 20, 2021 at 02:18:14AM +0800, Anand Jain wrote: >> latest_bdev is updated according to the changes to the device list. >> That means we could use the latest_bdev to show the device name in >> /proc/self/mounts. So this patch makes that change. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> >> RFC because >> 1. latest_bdev might not be the lowest devid but, we showed >> the lowest devid in /proc/self/mount. >> 2. The device's path is not shown now but, previously we did. >> So does these break ABI? Maybe yes for 2 howabout for 1 above? > > The path needs to be preserved, that would break a lot of things.. v2 fixed it. > >> fs/btrfs/super.c | 25 +++---------------------- >> 1 file changed, 3 insertions(+), 22 deletions(-) >> >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >> index 1f9dd1a4faa3..4ad3fe174c41 100644 >> --- a/fs/btrfs/super.c >> +++ b/fs/btrfs/super.c >> @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb) >> static int btrfs_show_devname(struct seq_file *m, struct dentry *root) >> { >> struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); >> - struct btrfs_device *dev, *first_dev = NULL; >> + char name[BDEVNAME_SIZE]; >> >> - /* >> - * Lightweight locking of the devices. We should not need >> - * device_list_mutex here as we only read the device data and the list >> - * is protected by RCU. Even if a device is deleted during the list >> - * traversals, we'll get valid data, the freeing callback will wait at >> - * least until the rcu_read_unlock. >> - */ >> - rcu_read_lock(); >> - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) { >> - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) >> - continue; >> - if (!dev->name) >> - continue; >> - if (!first_dev || dev->devid < first_dev->devid) >> - first_dev = dev; >> - } >> + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name), >> + " \t\n\\"); > > No protection at all? So what if latest_bdev or latest_dev gets updated > in parallel with devicre remove and there's a window where the pointer > is invalid but still is accessed. The whole point of RCU section here is > to prevent that. > YEsh. I noticed it. I am just on it. Will fix in v3. >> >> - if (first_dev) >> - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); >> - else >> - WARN_ON(1); >> - rcu_read_unlock(); >> return 0; >> } >> >> -- >> 2.31.1
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 1f9dd1a4faa3..4ad3fe174c41 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -2464,30 +2464,11 @@ static int btrfs_unfreeze(struct super_block *sb) static int btrfs_show_devname(struct seq_file *m, struct dentry *root) { struct btrfs_fs_info *fs_info = btrfs_sb(root->d_sb); - struct btrfs_device *dev, *first_dev = NULL; + char name[BDEVNAME_SIZE]; - /* - * Lightweight locking of the devices. We should not need - * device_list_mutex here as we only read the device data and the list - * is protected by RCU. Even if a device is deleted during the list - * traversals, we'll get valid data, the freeing callback will wait at - * least until the rcu_read_unlock. - */ - rcu_read_lock(); - list_for_each_entry_rcu(dev, &fs_info->fs_devices->devices, dev_list) { - if (test_bit(BTRFS_DEV_STATE_MISSING, &dev->dev_state)) - continue; - if (!dev->name) - continue; - if (!first_dev || dev->devid < first_dev->devid) - first_dev = dev; - } + seq_escape(m, bdevname(fs_info->fs_devices->latest_bdev, name), + " \t\n\\"); - if (first_dev) - seq_escape(m, rcu_str_deref(first_dev->name), " \t\n\\"); - else - WARN_ON(1); - rcu_read_unlock(); return 0; }
latest_bdev is updated according to the changes to the device list. That means we could use the latest_bdev to show the device name in /proc/self/mounts. So this patch makes that change. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- RFC because 1. latest_bdev might not be the lowest devid but, we showed the lowest devid in /proc/self/mount. 2. The device's path is not shown now but, previously we did. So does these break ABI? Maybe yes for 2 howabout for 1 above? fs/btrfs/super.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-)