Hi,
On Wed, Nov 16, 2022 at 7:20 PM Pavel Machek wrote:
>
> 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.
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
