linux-kernel - Re: [PATCH -next] md: wake up mdmon after setting badblocks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5f306d9e-56f7-5e3b-625e-e740f528e130@huaweicloud.com>
Date: Mon, 5 Aug 2024 19:08:19 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Mariusz Tkaczyk <mariusz.tkaczyk@...ux.intel.com>,
 Yu Kuai <yukuai1@...weicloud.com>
Cc: song@...nel.org, linux-raid@...r.kernel.org,
 linux-kernel@...r.kernel.org, yi.zhang@...wei.com, yangerkun@...wei.com,
 "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH -next] md: wake up mdmon after setting badblocks

Hi,

在 2024/08/05 17:40, Mariusz Tkaczyk 写道:
> On Thu,  1 Aug 2024 20:51:48 +0800
> Yu Kuai <yukuai1@...weicloud.com> wrote:
> 
>> From: Yu Kuai <yukuai3@...wei.com>
>>
>> For external super_block, mdmon rely on "mddev->sysfs_state" to update
>> super_block. However, rdev_set_badblocks() will set sb_flags without
>> waking up mdmon, which might cauing IO hang due to suepr_block can't be
>> updated.
>>
>> This problem is found by code review.
>>
>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
>> ---
>>   drivers/md/md.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 23cc77d51676..06d6ee8cd543 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9831,10 +9831,12 @@ int rdev_set_badblocks(struct md_rdev *rdev, sector_t
>> s, int sectors, /* Make sure they get written out promptly */
>>   		if (test_bit(ExternalBbl, &rdev->flags))
>>   			sysfs_notify_dirent_safe(rdev->sysfs_unack_badblocks);
>> -		sysfs_notify_dirent_safe(rdev->sysfs_state);
>>   		set_mask_bits(&mddev->sb_flags, 0,
>>   			      BIT(MD_SB_CHANGE_CLEAN) |
>> BIT(MD_SB_CHANGE_PENDING)); md_wakeup_thread(rdev->mddev->thread);
>> +
>> +		sysfs_notify_dirent_safe(rdev->sysfs_state);
>> +		sysfs_notify_dirent_safe(mddev->sysfs_state);
>>   		return 1;
>>   	} else
>>   		return 0;
> 
> 
> Hey Kuai,
> Later, I realized that mdmon is attached to various fds:
> 
> Here a mdmon code:
> 
> add_fd(&rfds, &maxfd, a->info.state_fd);	#mddev->sysfs_state
> add_fd(&rfds, &maxfd, a->action_fd);		#mddev->sysfs_action
> add_fd(&rfds, &maxfd, a->sync_completed_fd);	#mddev->resync_position
> 
> 
> for (mdi = a->info.devs ; mdi ; mdi = mdi->next) { #for each rdev
> 	add_fd(&rfds, &maxfd, mdi->state_fd);		#rdev->sysfs_state
> 	add_fd(&rfds, &maxfd, mdi->bb_fd);		#rdev->sysfs_badblocks
> 	add_fd(&rfds, &maxfd, mdi->ubb_fd);
> #rdev->sysfs_unack_badblocks }

Ok, so I was wrong that mdmon rely on "mddev->sysfs_state".
> 
> Notification in rdev->sysfs_state is fine. The problem was somewhere else
> (mdmon blocked on "rdev remove"). And It will be fixed by moving rdev remove to
> managemon because it is blocking action now.
> 
> https://github.com/md-raid-utilities/mdadm/pull/66
> 
> I think it is fine to move this down after set_mask_bits, but we don't
> have to repeat notification to mddev->state.

Yes, I'll send a v2 patch to move down "rdev->sysfs_state".

Thanks,
Kuai

> 
> Thanks,
> Mariusz
> 
> .
>