| Message ID | 20220608162756.144600-5-logang@deltatee.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bug fixes for mdadm tests | expand |
On Wed, Jun 8, 2022 at 9:28 AM Logan Gunthorpe <logang@deltatee.com> wrote: > > The raid5-cache code relies on there being no IO in flight when > log_exit() is called. There are two places where this is not > guaranteed so add mddev_suspend() and mddev_resume() calls to these > sites. > > The site in raid5_remove_disk() has a comment saying that it is > called in raid5d and thus cannot wait for pending writes; however that > does not appear to be correct anymore (if it ever was) as > raid5_remove_disk() is called from hot_remove_disk() which only > appears to be called in the md_ioctl(). Thus, the comment is removed, > as well as the racy check and replaced with calls to suspend/resume. > > The site in raid5_change_consistency_policy() is in the error path, > and another similar call site already has suspend/resume calls just > below it; so it should be equally safe to make that change here. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > --- > drivers/md/raid5.c | 18 ++++++------------ > 1 file changed, 6 insertions(+), 12 deletions(-) > > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > index 5d09256d7f81..3ad37dd4c5cd 100644 > --- a/drivers/md/raid5.c > +++ b/drivers/md/raid5.c > @@ -7938,18 +7938,9 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev) > > print_raid5_conf(conf); > if (test_bit(Journal, &rdev->flags) && conf->log) { > - /* > - * we can't wait pending write here, as this is called in > - * raid5d, wait will deadlock. > - * neilb: there is no locking about new writes here, > - * so this cannot be safe. > - */ > - if (atomic_read(&conf->active_stripes) || > - atomic_read(&conf->r5c_cached_full_stripes) || > - atomic_read(&conf->r5c_cached_partial_stripes)) { > - return -EBUSY; > - } > + mddev_suspend(mddev); Unfortunately, the comment about deadlock is still true, and we cannot call mddev_suspend here. To trigger it: # create raid5 with journal: mdadm --create /dev/md0 -l 5 -n 3 /dev/nvme[0-2]n1 --write-journal /dev/nvme3n1 # start some writes fio ... # fail the journal mdadm --fail /dev/md0 /dev/nvme3n1 This will cause deadlock of the thread: [<0>] raid5_quiesce+0x2a8/0x5f0 [<0>] mddev_suspend+0x26b/0x530 [<0>] raid5_remove_disk+0x12e/0x6f3 [<0>] remove_and_add_spares+0x5b2/0xef0 [<0>] md_check_recovery+0xe68/0x12b0 [<0>] raid5d+0xf4/0xf30 [<0>] md_thread+0x299/0x350 [<0>] kthread+0x29d/0x340 [<0>] ret_from_fork+0x22/0x30 Thanks, Song > log_exit(conf); > + mddev_resume(mddev); > return 0; > } > if (rdev == rcu_access_pointer(p->rdev)) > @@ -8697,8 +8688,11 @@ static int raid5_change_consistency_policy(struct mddev *mddev, const char *buf) > err = log_init(conf, NULL, true); > if (!err) { > err = resize_stripes(conf, conf->pool_size); > - if (err) > + if (err) { > + mddev_suspend(mddev); > log_exit(conf); > + mddev_resume(mddev); > + } > } > } else > err = -EINVAL; > -- > 2.30.2 >
On 2022-06-08 11:59, Song Liu wrote: > On Wed, Jun 8, 2022 at 9:28 AM Logan Gunthorpe <logang@deltatee.com> wrote: >> >> The raid5-cache code relies on there being no IO in flight when >> log_exit() is called. There are two places where this is not >> guaranteed so add mddev_suspend() and mddev_resume() calls to these >> sites. >> >> The site in raid5_remove_disk() has a comment saying that it is >> called in raid5d and thus cannot wait for pending writes; however that >> does not appear to be correct anymore (if it ever was) as >> raid5_remove_disk() is called from hot_remove_disk() which only >> appears to be called in the md_ioctl(). Thus, the comment is removed, >> as well as the racy check and replaced with calls to suspend/resume. >> >> The site in raid5_change_consistency_policy() is in the error path, >> and another similar call site already has suspend/resume calls just >> below it; so it should be equally safe to make that change here. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> --- >> drivers/md/raid5.c | 18 ++++++------------ >> 1 file changed, 6 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c >> index 5d09256d7f81..3ad37dd4c5cd 100644 >> --- a/drivers/md/raid5.c >> +++ b/drivers/md/raid5.c >> @@ -7938,18 +7938,9 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev) >> >> print_raid5_conf(conf); >> if (test_bit(Journal, &rdev->flags) && conf->log) { >> - /* >> - * we can't wait pending write here, as this is called in >> - * raid5d, wait will deadlock. >> - * neilb: there is no locking about new writes here, >> - * so this cannot be safe. >> - */ >> - if (atomic_read(&conf->active_stripes) || >> - atomic_read(&conf->r5c_cached_full_stripes) || >> - atomic_read(&conf->r5c_cached_partial_stripes)) { >> - return -EBUSY; >> - } >> + mddev_suspend(mddev); > > Unfortunately, the comment about deadlock is still true, and we cannot call > mddev_suspend here. To trigger it: Ah, yes. What a tangle. I think we can just drop this patch. Now that we are removing RCU it isn't actually necessary to fix the bug I was seeing. It's still probably broken as the comment notes though. Thanks, Logan
On Wed, Jun 8, 2022 at 11:21 AM Logan Gunthorpe <logang@deltatee.com> wrote: > > > > On 2022-06-08 11:59, Song Liu wrote: > > On Wed, Jun 8, 2022 at 9:28 AM Logan Gunthorpe <logang@deltatee.com> wrote: > >> > >> The raid5-cache code relies on there being no IO in flight when > >> log_exit() is called. There are two places where this is not > >> guaranteed so add mddev_suspend() and mddev_resume() calls to these > >> sites. > >> > >> The site in raid5_remove_disk() has a comment saying that it is > >> called in raid5d and thus cannot wait for pending writes; however that > >> does not appear to be correct anymore (if it ever was) as > >> raid5_remove_disk() is called from hot_remove_disk() which only > >> appears to be called in the md_ioctl(). Thus, the comment is removed, > >> as well as the racy check and replaced with calls to suspend/resume. > >> > >> The site in raid5_change_consistency_policy() is in the error path, > >> and another similar call site already has suspend/resume calls just > >> below it; so it should be equally safe to make that change here. > >> > >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > >> Reviewed-by: Christoph Hellwig <hch@lst.de> > >> --- > >> drivers/md/raid5.c | 18 ++++++------------ > >> 1 file changed, 6 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c > >> index 5d09256d7f81..3ad37dd4c5cd 100644 > >> --- a/drivers/md/raid5.c > >> +++ b/drivers/md/raid5.c > >> @@ -7938,18 +7938,9 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev) > >> > >> print_raid5_conf(conf); > >> if (test_bit(Journal, &rdev->flags) && conf->log) { > >> - /* > >> - * we can't wait pending write here, as this is called in > >> - * raid5d, wait will deadlock. > >> - * neilb: there is no locking about new writes here, > >> - * so this cannot be safe. > >> - */ > >> - if (atomic_read(&conf->active_stripes) || > >> - atomic_read(&conf->r5c_cached_full_stripes) || > >> - atomic_read(&conf->r5c_cached_partial_stripes)) { > >> - return -EBUSY; > >> - } > >> + mddev_suspend(mddev); > > > > Unfortunately, the comment about deadlock is still true, and we cannot call > > mddev_suspend here. To trigger it: > > Ah, yes. What a tangle. I think we can just drop this patch. Now that we > are removing RCU it isn't actually necessary to fix the bug I was > seeing. It's still probably broken as the comment notes though. How about we keep the suspend/resume in raid5_change_consistency_policy()? Like the one I just pushed to md-next: https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?h=md-next&id=ac1506992459fe45a085c1f93df74d51c381887b Thanks, Song
On 2022-06-08 16:02, Song Liu wrote: > On Wed, Jun 8, 2022 at 11:21 AM Logan Gunthorpe <logang@deltatee.com> wrote: >> >> >> >> On 2022-06-08 11:59, Song Liu wrote: >>> On Wed, Jun 8, 2022 at 9:28 AM Logan Gunthorpe <logang@deltatee.com> wrote: >>>> >>>> The raid5-cache code relies on there being no IO in flight when >>>> log_exit() is called. There are two places where this is not >>>> guaranteed so add mddev_suspend() and mddev_resume() calls to these >>>> sites. >>>> >>>> The site in raid5_remove_disk() has a comment saying that it is >>>> called in raid5d and thus cannot wait for pending writes; however that >>>> does not appear to be correct anymore (if it ever was) as >>>> raid5_remove_disk() is called from hot_remove_disk() which only >>>> appears to be called in the md_ioctl(). Thus, the comment is removed, >>>> as well as the racy check and replaced with calls to suspend/resume. >>>> >>>> The site in raid5_change_consistency_policy() is in the error path, >>>> and another similar call site already has suspend/resume calls just >>>> below it; so it should be equally safe to make that change here. >>>> >>>> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >>>> Reviewed-by: Christoph Hellwig <hch@lst.de> >>>> --- >>>> drivers/md/raid5.c | 18 ++++++------------ >>>> 1 file changed, 6 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c >>>> index 5d09256d7f81..3ad37dd4c5cd 100644 >>>> --- a/drivers/md/raid5.c >>>> +++ b/drivers/md/raid5.c >>>> @@ -7938,18 +7938,9 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev) >>>> >>>> print_raid5_conf(conf); >>>> if (test_bit(Journal, &rdev->flags) && conf->log) { >>>> - /* >>>> - * we can't wait pending write here, as this is called in >>>> - * raid5d, wait will deadlock. >>>> - * neilb: there is no locking about new writes here, >>>> - * so this cannot be safe. >>>> - */ >>>> - if (atomic_read(&conf->active_stripes) || >>>> - atomic_read(&conf->r5c_cached_full_stripes) || >>>> - atomic_read(&conf->r5c_cached_partial_stripes)) { >>>> - return -EBUSY; >>>> - } >>>> + mddev_suspend(mddev); >>> >>> Unfortunately, the comment about deadlock is still true, and we cannot call >>> mddev_suspend here. To trigger it: >> >> Ah, yes. What a tangle. I think we can just drop this patch. Now that we >> are removing RCU it isn't actually necessary to fix the bug I was >> seeing. It's still probably broken as the comment notes though. > > How about we keep the suspend/resume in raid5_change_consistency_policy()? > Like the one I just pushed to md-next: > > https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git/commit/?h=md-next&id=ac1506992459fe45a085c1f93df74d51c381887b I'm good with that. Thanks. I'll do some testing on md-next shortly. Thanks, Logan
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index 5d09256d7f81..3ad37dd4c5cd 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c @@ -7938,18 +7938,9 @@ static int raid5_remove_disk(struct mddev *mddev, struct md_rdev *rdev) print_raid5_conf(conf); if (test_bit(Journal, &rdev->flags) && conf->log) { - /* - * we can't wait pending write here, as this is called in - * raid5d, wait will deadlock. - * neilb: there is no locking about new writes here, - * so this cannot be safe. - */ - if (atomic_read(&conf->active_stripes) || - atomic_read(&conf->r5c_cached_full_stripes) || - atomic_read(&conf->r5c_cached_partial_stripes)) { - return -EBUSY; - } + mddev_suspend(mddev); log_exit(conf); + mddev_resume(mddev); return 0; } if (rdev == rcu_access_pointer(p->rdev)) @@ -8697,8 +8688,11 @@ static int raid5_change_consistency_policy(struct mddev *mddev, const char *buf) err = log_init(conf, NULL, true); if (!err) { err = resize_stripes(conf, conf->pool_size); - if (err) + if (err) { + mddev_suspend(mddev); log_exit(conf); + mddev_resume(mddev); + } } } else err = -EINVAL;