Hi, On Wed, Nov 16, 2022 at 7:20 PM Pavel Machek wrote:
Hi!
From: Ryusuke Konishi konishi.ryusuke@gmail.com
commit 8ac932a4921a96ca52f61935dbba64ea87bbd5dc upstream.
...
However, there is actually no need to take rwsem A in nilfs_count_free_blocks() because it, within the lock section, only reads a single integer data on a shared struct with nilfs_sufile_get_ncleansegs(). This has been the case after commit aa474a220180 ("nilfs2: add local variable to cache the number of clean segments"), that is, even before this bug was introduced.
Ok, but these days we have checkers that don't like reading variables unlocked, and compiler theoretically could do something funny.
So this should have READ/WRITE_ONCE annotations... this is incomplete, but should illustrate what is needed. Likely some helper for updating ncleansegs should be introduced.
If the compiler omits an update in the middle, it's okay for nilfs_count_free_blocks() and other read-only callers.
In fact if I introduce such helper function like nilfs_sufile_set_ncleansegs() with WRITE_ONCE, I use nilfs_sufile_get_ncleansegs() first and nilfs_sufile_set_ncleansegs() last within functions that update the counter, rather than each time they increment or decrement it. But, I didn't see any point in using WRITE_ONCE/READ_ONCE like this.
Just be sure, the functions that update the counter are already exclusive with another semaphore (sufile->mi_sem), so updating it without the intermediate result won't break the counter.
If there's a real problem with some kind of checkers giving warnings as you mentioned, that's probably the real reason to apply those annotations. I would like to deal with it for the mainline in that case.
Thanks, Ryusuke Konishi
Best regards, Pavel
diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c index 63722475e17e..58dddc0b1d88 100644 --- a/fs/nilfs2/sufile.c +++ b/fs/nilfs2/sufile.c @@ -122,7 +122,7 @@ static void nilfs_sufile_mod_counter(struct buffer_head *header_bh, */ unsigned long nilfs_sufile_get_ncleansegs(struct inode *sufile) {
return NILFS_SUI(sufile)->ncleansegs;
return READ_ONCE(NILFS_SUI(sufile)->ncleansegs);}
/** @@ -418,7 +418,9 @@ void nilfs_sufile_do_cancel_free(struct inode *sufile, __u64 segnum, kunmap_atomic(kaddr);
nilfs_sufile_mod_counter(header_bh, -1, 1);
NILFS_SUI(sufile)->ncleansegs--;
/* nilfs_sufile_get_ncleansegs() leads this without taking lock */WRITE_ONCE(NILFS_SUI(sufile)->ncleansegs,READ_ONCE(NILFS_SUI(sufile)->ncleansegs) - 1); mark_buffer_dirty(su_bh); nilfs_mdt_mark_dirty(sufile);-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany