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.
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);