Re: [PATCH 1/1] jffs2: fix sparse warning: unexpected unlock

Re: [PATCH 1/1] jffs2: fix sparse warning: unexpected unlock

[Brian Norris]

+ linux-sparse

On Thu, Sep 18, 2014 at 08:46:16PM +0200, Fabian Frederick wrote:
> fs/jffs2/summary.c:846:5: warning: context imbalance in 'jffs2_sum_write_sumnode' - unexpected unlock
> 
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> ---
>  fs/jffs2/summary.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/jffs2/summary.c b/fs/jffs2/summary.c
> index c522d09..a0bac7b 100644
> --- a/fs/jffs2/summary.c
> +++ b/fs/jffs2/summary.c
> @@ -844,6 +844,8 @@ static int jffs2_sum_write_data(struct jffs2_sb_info *c, struct jffs2_eraseblock
>  /* Write out summary information - called from jffs2_do_reserve_space */
>  
>  int jffs2_sum_write_sumnode(struct jffs2_sb_info *c)
> +	__releases(&c->erase_completion_lock)
> +	__acquires(&c->erase_completion_lock)

I'm not too familiar with sparse notations, but Documentation/sparse.txt
suggests the above is wrong, and the following is more accurate:

	__must_hold(&c->erase_completion_lock)

But it looks like there are several other examples which do this.
Anyway, here's the relevant doc text, in case someone wants to clarify
it for me, or else tell me the documentation is wrong:

    __must_hold - The specified lock is held on function entry and exit.

    __acquires - The specified lock is held on function exit, but not entry.

    __releases - The specified lock is held on function entry, but not exit.

So __acquires and __releases look mutually exclusive, but it's not clear
if __must_hold will actually cover what we want. (I haven't tested it.)

>  {
>  	int datasize, infosize, padsize;
>  	struct jffs2_eraseblock *jeb;

Brian

Re: [PATCH 1/1] jffs2: fix sparse warning: unexpected unlock

[josh]

On Mon, Sep 22, 2014 at 11:12:50AM -0700, Brian Norris wrote:
> + linux-sparse
> 
> On Thu, Sep 18, 2014 at 08:46:16PM +0200, Fabian Frederick wrote:
> > fs/jffs2/summary.c:846:5: warning: context imbalance in 'jffs2_sum_write_sumnode' - unexpected unlock
> > 
> > Signed-off-by: Fabian Frederick <fabf@skynet.be>
> > ---
> >  fs/jffs2/summary.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/jffs2/summary.c b/fs/jffs2/summary.c
> > index c522d09..a0bac7b 100644
> > --- a/fs/jffs2/summary.c
> > +++ b/fs/jffs2/summary.c
> > @@ -844,6 +844,8 @@ static int jffs2_sum_write_data(struct jffs2_sb_info *c, struct jffs2_eraseblock
> >  /* Write out summary information - called from jffs2_do_reserve_space */
> >  
> >  int jffs2_sum_write_sumnode(struct jffs2_sb_info *c)
> > +	__releases(&c->erase_completion_lock)
> > +	__acquires(&c->erase_completion_lock)
> 
> I'm not too familiar with sparse notations, but Documentation/sparse.txt
> suggests the above is wrong, and the following is more accurate:
> 
> 	__must_hold(&c->erase_completion_lock)
> 
> But it looks like there are several other examples which do this.
> Anyway, here's the relevant doc text, in case someone wants to clarify
> it for me, or else tell me the documentation is wrong:
> 
>     __must_hold - The specified lock is held on function entry and exit.
> 
>     __acquires - The specified lock is held on function exit, but not entry.
> 
>     __releases - The specified lock is held on function entry, but not exit.
> 
> So __acquires and __releases look mutually exclusive, but it's not clear
> if __must_hold will actually cover what we want. (I haven't tested it.)

__must_hold is indeed the correct annotation.  (There isn't currently
anything enforcing that, though.)

- Josh Triplett

Re: [PATCH 1/1] jffs2: fix sparse warning: unexpected unlock

[Fabian Frederick]

> On 27 September 2014 at 01:17 josh@joshtriplett.org wrote:
>
>
> On Mon, Sep 22, 2014 at 11:12:50AM -0700, Brian Norris wrote:
> > + linux-sparse
> >
> > On Thu, Sep 18, 2014 at 08:46:16PM +0200, Fabian Frederick wrote:
> > > fs/jffs2/summary.c:846:5: warning: context imbalance in
> > > 'jffs2_sum_write_sumnode' - unexpected unlock
> > >
> > > Signed-off-by: Fabian Frederick <fabf@skynet.be>
> > > ---
> > >  fs/jffs2/summary.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/jffs2/summary.c b/fs/jffs2/summary.c
> > > index c522d09..a0bac7b 100644
> > > --- a/fs/jffs2/summary.c
> > > +++ b/fs/jffs2/summary.c
> > > @@ -844,6 +844,8 @@ static int jffs2_sum_write_data(struct jffs2_sb_info
> > > *c, struct jffs2_eraseblock
> > >  /* Write out summary information - called from jffs2_do_reserve_space */
> > > 
> > >  int jffs2_sum_write_sumnode(struct jffs2_sb_info *c)
> > > + __releases(&c->erase_completion_lock)
> > > + __acquires(&c->erase_completion_lock)
> >
> > I'm not too familiar with sparse notations, but Documentation/sparse.txt
> > suggests the above is wrong, and the following is more accurate:
> >
> >     __must_hold(&c->erase_completion_lock)
> >
> > But it looks like there are several other examples which do this.
> > Anyway, here's the relevant doc text, in case someone wants to clarify
> > it for me, or else tell me the documentation is wrong:
> >
> >     __must_hold - The specified lock is held on function entry and exit.
> >
> >     __acquires - The specified lock is held on function exit, but not entry.
> >
> >     __releases - The specified lock is held on function entry, but not exit.
> >
> > So __acquires and __releases look mutually exclusive, but it's not clear
> > if __must_hold will actually cover what we want. (I haven't tested it.)
>
> __must_hold is indeed the correct annotation.  (There isn't currently
> anything enforcing that, though.)
>
> - Josh Triplett

There are 137 __releases && __acquires annotated functions in stable.

AFAICS those are based on lock held on function entry / lock held on exit

See
fs/fuse/file.c:1527
fs/kernfs/dir.c:341
drivers/block/nbd.c:564

Does it mean that all of these should be updated to __must_hold ?

Fabian

Re: [PATCH 1/1] jffs2: fix sparse warning: unexpected unlock

[Josh Triplett]

On Sat, Sep 27, 2014 at 10:41:55AM +0200, Fabian Frederick wrote:
> 
> 
> > On 27 September 2014 at 01:17 josh@joshtriplett.org wrote:
> >
> >
> > On Mon, Sep 22, 2014 at 11:12:50AM -0700, Brian Norris wrote:
> > > + linux-sparse
> > >
> > > On Thu, Sep 18, 2014 at 08:46:16PM +0200, Fabian Frederick wrote:
> > > > fs/jffs2/summary.c:846:5: warning: context imbalance in
> > > > 'jffs2_sum_write_sumnode' - unexpected unlock
> > > >
> > > > Signed-off-by: Fabian Frederick <fabf@skynet.be>
> > > > ---
> > > >  fs/jffs2/summary.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/fs/jffs2/summary.c b/fs/jffs2/summary.c
> > > > index c522d09..a0bac7b 100644
> > > > --- a/fs/jffs2/summary.c
> > > > +++ b/fs/jffs2/summary.c
> > > > @@ -844,6 +844,8 @@ static int jffs2_sum_write_data(struct jffs2_sb_info
> > > > *c, struct jffs2_eraseblock
> > > >  /* Write out summary information - called from jffs2_do_reserve_space */
> > > > 
> > > >  int jffs2_sum_write_sumnode(struct jffs2_sb_info *c)
> > > > + __releases(&c->erase_completion_lock)
> > > > + __acquires(&c->erase_completion_lock)
> > >
> > > I'm not too familiar with sparse notations, but Documentation/sparse.txt
> > > suggests the above is wrong, and the following is more accurate:
> > >
> > >     __must_hold(&c->erase_completion_lock)
> > >
> > > But it looks like there are several other examples which do this.
> > > Anyway, here's the relevant doc text, in case someone wants to clarify
> > > it for me, or else tell me the documentation is wrong:
> > >
> > >     __must_hold - The specified lock is held on function entry and exit.
> > >
> > >     __acquires - The specified lock is held on function exit, but not entry.
> > >
> > >     __releases - The specified lock is held on function entry, but not exit.
> > >
> > > So __acquires and __releases look mutually exclusive, but it's not clear
> > > if __must_hold will actually cover what we want. (I haven't tested it.)
> >
> > __must_hold is indeed the correct annotation.  (There isn't currently
> > anything enforcing that, though.)
> >
> > - Josh Triplett
> 
> There are 137 __releases && __acquires annotated functions in stable.
> 
> AFAICS those are based on lock held on function entry / lock held on exit
> 
> See
> fs/fuse/file.c:1527
> fs/kernfs/dir.c:341
> drivers/block/nbd.c:564
> 
> Does it mean that all of these should be updated to __must_hold ?

Not really worth changing yet until something actually pays closer
attention to those annotations.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html