Hi!
> From: Ryusuke Konishi <konishi.ryusuke@xxxxxxxxx>
>
> 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);
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Attachment:
signature.asc
Description: PGP signature
