Received-SPF: pass (google.com: domain of linux-kernel+bounces-49348-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Subject: Re: [PATCH v4] md/raid5: fix atomicity violation in raid5_cache_count
To: Song Liu <song@kernel.org>, Gui-Dong Han <2045gemini@gmail.com>
Cc: linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org,
 baijiaju1990@outlook.com, stable@vger.kernel.org,
 "yukuai (C)" <yukuai3@huawei.com>
References: <20240112071017.16313-1-2045gemini@gmail.com>
 <CAPhsuW64FjJEqTFKX9WVzrXvC4rpcjojBw5A3StUq0C20F_vyA@mail.gmail.com>
From: Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <e0c5f9d4-6b65-02f7-2a8c-0484fd1f9815@huaweicloud.com>
Date: Fri, 2 Feb 2024 15:11:17 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <CAPhsuW64FjJEqTFKX9WVzrXvC4rpcjojBw5A3StUq0C20F_vyA@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi,

在 2024/01/30 15:37, Song Liu 写道:
> On Thu, Jan 11, 2024 at 11:10 PM Gui-Dong Han <2045gemini@gmail.com> wrote:
>>
> [...]
>>
>>          raid5_release_stripe(sh);
>> -       conf->max_nr_stripes++;
>> +       WRITE_ONCE(conf->max_nr_stripes, conf->max_nr_stripes + 1);
> 
> This is weird. We are reading max_nr_stripes without READ_ONCE.

We don't need READ_ONCE() here because writers are protected by
'cache_size_mutex', there are no concurrent writers, it's safe to
read 'max_nr_stripes' directly.

Thanks,
Kuai

> 
>>          return 1;
>>   }
>>
>> @@ -2707,7 +2707,7 @@ static int drop_one_stripe(struct r5conf *conf)
>>          shrink_buffers(sh);
>>          free_stripe(conf->slab_cache, sh);
>>          atomic_dec(&conf->active_stripes);
>> -       conf->max_nr_stripes--;
>> +       WRITE_ONCE(conf->max_nr_stripes, conf->max_nr_stripes - 1);
> 
> Same here.
> 
>>          return 1;
>>   }
>>
>> @@ -6820,7 +6820,7 @@ raid5_set_cache_size(struct mddev *mddev, int size)
>>          if (size <= 16 || size > 32768)
>>                  return -EINVAL;
>>
>> -       conf->min_nr_stripes = size;
>> +       WRITE_ONCE(conf->min_nr_stripes, size);
>>          mutex_lock(&conf->cache_size_mutex);
>>          while (size < conf->max_nr_stripes &&
>>                 drop_one_stripe(conf))
>> @@ -6832,7 +6832,7 @@ raid5_set_cache_size(struct mddev *mddev, int size)
>>          mutex_lock(&conf->cache_size_mutex);
>>          while (size > conf->max_nr_stripes)
>>                  if (!grow_one_stripe(conf, GFP_KERNEL)) {
>> -                       conf->min_nr_stripes = conf->max_nr_stripes;
>> +                       WRITE_ONCE(conf->min_nr_stripes, conf->max_nr_stripes);
> 
> And here.
> 
>>                          result = -ENOMEM;
>>                          break;
>>                  }
> 
> Thanks,
> Song
> .
>