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.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
