Re: [PATCH v2] mm/swap: fix swap_info_struct race between swapoff and get_swap_pages()

[Rongwei Wang <rongwei.wang@linux.alibaba.com>]

Hi Andrew

On 4/5/23 3:26 AM, Andrew Morton wrote:
> On Tue,  4 Apr 2023 23:47:16 +0800 Rongwei Wang <rongwei.wang@linux.alibaba.com> wrote:
>
>> The si->lock must be held when deleting the si from
>> the available list.
>>
>> ...
>>
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -679,6 +679,7 @@ static void __del_from_avail_list(struct swap_info_struct *p)
>>   {
>>   	int nid;
>>   
>> +	assert_spin_locked(&p->lock);
>>   	for_each_node(nid)
>>   		plist_del(&p->avail_lists[nid], &swap_avail_heads[nid]);
>>   }
>> @@ -2434,8 +2435,8 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
>>   		spin_unlock(&swap_lock);
>>   		goto out_dput;
>>   	}
>> -	del_from_avail_list(p);
>>   	spin_lock(&p->lock);
>> +	del_from_avail_list(p);
>>   	if (p->prio < 0) {
>>   		struct swap_info_struct *si = p;
>>   		int nid;
> So we have
>
> swap_avail_lock
> swap_info_struct.lock
> swap_cluster_info.lock
>
> Is the ranking of these three clearly documented somewhere?

It seems have

swap_lock

swap_info_struct.lock

swap_avail_lock

I just summary the ranking of these three locks by reading code, not 
find any documents (maybe have).

>
>
> Did you test this with lockdep fully enabled?
>
>
> I'm thinking that Aaron's a2468cc9bfdff ("swap: choose swap device
> according to numa node") is the appropriate Fixes: target - do you
> agree?

Yes, I'm sure my latest test version has included Aaron's a2468cc9bfdff, 
and my test .config has enabled CONFIG

as below:

CONFIG_LOCK_DEBUGGING_SUPPORT=y CONFIG_PROVE_LOCKING=y 
CONFIG_DEBUG_SPINLOCK=y CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_LOCKDEP=y 
CONFIG_DEBUG_LOCKDEP=y CONFIG_DEBUG_ATOMIC_SLEEP=y

>
>
> These functions use identifier `p' for the swap_info_struct*, whereas
> most other code uses the much more sensible `si'.  That's just rude.
> But we shouldn't change that within this fix.

Indeed, It's confusing more or less to use both 'si' and 'p'. I can 
ready for another patch to replace 'p' with 'si'.

Thanks.