[Devel] [RFC PATCH vz9 v6 02/62] dm-ploop: Use READ_ONCE/WRITE_ONCE to access md page data

[Pavel Tikhomirov]

On 1/20/25 16:03, Alexander Atanasov wrote:
> On 20.01.25 9:23, Pavel Tikhomirov wrote:
>>
>>
>> On 12/6/24 05:55, Alexander Atanasov wrote:
>>> @@ -1090,9 +1090,9 @@ static int ploop_alloc_cluster(struct ploop 
>>> *ploop, struct ploop_index_wb *piwb,
>>>       clu -= piwb->page_id * PAGE_SIZE / sizeof(map_index_t) - 
>>> PLOOP_MAP_OFFSET;
>>>       to = piwb->kmpage;
>>> -    if (to[clu]) {
>>> +    if (READ_ONCE(to[clu])) {
>>>           /* Already mapped by one of previous bios */
>>> -        *dst_clu = to[clu];
>>> +        *dst_clu = READ_ONCE(to[clu]);
>>>           already_alloced = true;
>>>       }
>>
>> The above hunk does not look good, we explicitly do 
>> "READ_ONCE(to[clu])" twice, likely that's not what we want.
> 
> 
> We can do without inner READ_ONCE but for consistency i think it is 
> better. /it would be optimised by the compiler anyway/
> 
> The idea is that if a clu is already mapped we read !=0 in the if,
> inside we will always read the !=0 too, since discard is synchronized by 
> the holes bitmap.

That may be, if clu can't be un-mapped or somehow reset... Properly 
validating it will require checking all 24 uses of ->kmpage.

It would be much cleaner and simpler to do:

tmp_var = READ_ONCE(smth)
if (tmp_var) {
     *ptr_var = tmp_var;
}

I doubt that you will find any other example of doing READ_ONCE of the 
same shared memory/variable twice in one function.

> 

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.