mbox series

[-next,v2,0/6] md: remove rcu protection to access rdev from conf

Message ID 20231021102059.3198284-1-yukuai1@huaweicloud.com
Headers
Series md: remove rcu protection to access rdev from conf |

Message

Yu Kuai Oct. 21, 2023, 10:20 a.m. UTC 
  From: Yu Kuai <yukuai3@huawei.com>

The lifetime of rdev:

1. md_import_device() generate a rdev based on underlying disk;

   mddev_lock()
   rdev = kzalloc();
   rdev->bdev = blkdev_get_by_dev();
   mddev_unlock()

2. bind_rdev_to_array() add this rdev to mddev->disks;

   mddev_lock()
   kobject_add(&rdev->kobj, &mddev->kobj, ...);
   list_add_rcu(&rdev->same_set, &mddev->disks);
   mddev_unlock()

3. remove_and_add_spares() add this rdev to conf;

   mddev_lock()
   rdev_addable();
   pers->hot_add_disk();
   rcu_assign_pointer(conf->rdev, rdev);
   mddev_unlock()

4. Use this array with rdev;

5. remove_and_add_spares() remove rdev from conf;

   mddev_lock()
   // triggered by sysfs/ioctl
   rdev_removeable();
   pers->hot_remove_disk();
    rcu_assign_pointer(conf->rdev, NULL);
    synchronize_rcu();
   mddev_unlock()

   // triggered by daemon
   mddev_lock()
   rdev_removeable();
   synchronize_rcu(); -> this can't protect accessing rdev from conf
   pers->hot_remove_disk();
    rcu_assign_pointer(conf->rdev, NULL);
   mddev_unlock()

6. md_kick_rdev_from_array() remove rdev from mddev->disks;

   mddev_lock()
   list_del_rcu(&rdev->same_set);
   synchronize_rcu();
   list_add(&rdev->same_set, &mddev->deleting)
   mddev_unlock()
    export_rdev

There are two separate rcu protection for rdev, and this pathset remove
the protection of conf(step 3 and 5), because it's safe to access rdev
from conf in following cases:

 - If 'reconfig_mutex' is held, because rdev can't be added or rmoved to
 conf;
 - If there is normal IO inflight, because mddev_suspend() will wait for
 IO to be done and prevent rdev to be added or removed to conf;
 - If sync thread is running, because remove_and_add_spares() can only be
 called from daemon thread when sync thread is done, and
 'MD_RECOVERY_RUNNING' is also checked for ioctl/sysfs;
 - if any spinlock or rcu_read_lock() is held, because synchronize_rcu()
 from step 6 prevent rdev to be freed until spinlock is released or
 rcu_read_unlock();

Yu Kuai (6):
  md: remove useless debug code to print configuration
  md: remove flag RemoveSynchronized
  md/raid1: remove rcu protection to access rdev from conf
  md/raid10: remove rcu protection to access rdev from conf
  md/raid5: remove rcu protection to access rdev from conf
  md/md-multipath: remove rcu protection to access rdev from conf

 drivers/md/md-multipath.c |  29 ++---
 drivers/md/md.c           |  37 +-----
 drivers/md/raid1.c        |  94 ++++-----------
 drivers/md/raid10.c       | 248 +++++++++-----------------------------
 drivers/md/raid5-cache.c  |  11 +-
 drivers/md/raid5-ppl.c    |  16 +--
 drivers/md/raid5.c        | 225 ++++++++++------------------------
 drivers/md/raid5.h        |   4 +-
 8 files changed, 163 insertions(+), 501 deletions(-)

Comments

Song Liu Nov. 24, 2023, 8:13 a.m. UTC | #1 
On Fri, Oct 20, 2023 at 7:25 PM Yu Kuai <yukuai1@huaweicloud.com> wrote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> The lifetime of rdev:
>
> 1. md_import_device() generate a rdev based on underlying disk;
>
>    mddev_lock()
>    rdev = kzalloc();
>    rdev->bdev = blkdev_get_by_dev();
>    mddev_unlock()
>
> 2. bind_rdev_to_array() add this rdev to mddev->disks;
>
>    mddev_lock()
>    kobject_add(&rdev->kobj, &mddev->kobj, ...);
>    list_add_rcu(&rdev->same_set, &mddev->disks);
>    mddev_unlock()
>
> 3. remove_and_add_spares() add this rdev to conf;
>
>    mddev_lock()
>    rdev_addable();
>    pers->hot_add_disk();
>    rcu_assign_pointer(conf->rdev, rdev);
>    mddev_unlock()
>
> 4. Use this array with rdev;
>
> 5. remove_and_add_spares() remove rdev from conf;
>
>    mddev_lock()
>    // triggered by sysfs/ioctl
>    rdev_removeable();
>    pers->hot_remove_disk();
>     rcu_assign_pointer(conf->rdev, NULL);
>     synchronize_rcu();
>    mddev_unlock()
>
>    // triggered by daemon
>    mddev_lock()
>    rdev_removeable();
>    synchronize_rcu(); -> this can't protect accessing rdev from conf
>    pers->hot_remove_disk();
>     rcu_assign_pointer(conf->rdev, NULL);
>    mddev_unlock()
>
> 6. md_kick_rdev_from_array() remove rdev from mddev->disks;
>
>    mddev_lock()
>    list_del_rcu(&rdev->same_set);
>    synchronize_rcu();
>    list_add(&rdev->same_set, &mddev->deleting)
>    mddev_unlock()
>     export_rdev
>
> There are two separate rcu protection for rdev, and this pathset remove
> the protection of conf(step 3 and 5), because it's safe to access rdev
> from conf in following cases:
>
>  - If 'reconfig_mutex' is held, because rdev can't be added or rmoved to
>  conf;
>  - If there is normal IO inflight, because mddev_suspend() will wait for
>  IO to be done and prevent rdev to be added or removed to conf;
>  - If sync thread is running, because remove_and_add_spares() can only be
>  called from daemon thread when sync thread is done, and
>  'MD_RECOVERY_RUNNING' is also checked for ioctl/sysfs;
>  - if any spinlock or rcu_read_lock() is held, because synchronize_rcu()
>  from step 6 prevent rdev to be freed until spinlock is released or
>  rcu_read_unlock();

Thanks for the cover letter.

Song

>
> Yu Kuai (6):
>   md: remove useless debug code to print configuration
>   md: remove flag RemoveSynchronized
>   md/raid1: remove rcu protection to access rdev from conf
>   md/raid10: remove rcu protection to access rdev from conf
>   md/raid5: remove rcu protection to access rdev from conf
>   md/md-multipath: remove rcu protection to access rdev from conf
>
>  drivers/md/md-multipath.c |  29 ++---
>  drivers/md/md.c           |  37 +-----
>  drivers/md/raid1.c        |  94 ++++-----------
>  drivers/md/raid10.c       | 248 +++++++++-----------------------------
>  drivers/md/raid5-cache.c  |  11 +-
>  drivers/md/raid5-ppl.c    |  16 +--
>  drivers/md/raid5.c        | 225 ++++++++++------------------------
>  drivers/md/raid5.h        |   4 +-
>  8 files changed, 163 insertions(+), 501 deletions(-)
>
> --
> 2.39.2
>