Re: [PATCH] nilfs2: suppress false positive WARN_ONs for sufile after an FS error

On Thu, Aug 6, 2026 at 9:58 PM Zhan Xusheng wrote:
>
> On Thu,  6 Aug 2026 18:02:00 +0900 Ryusuke Konishi wrote:
> > +#define nilfs_sufile_warn_on_error(sufile, err)              \
> > +     ({                                              \
> > +             int _err = (err);                       \
> > +                                                     \
> > +             if (unlikely(_err))                     \
> > +                     _err = WARN_ONCE(!sb_rdonly((sufile)->i_sb),    \
> > +                             "unexpected sufile error %d\n", _err) ? \
> > +                             -EIO : -EROFS;          \
> > +             unlikely(_err);                         \
> > +     })
>
> The warning-suppression itself looks right to me: WARN_ONCE(!sb_rdonly)
> only fires when the fs has not degraded to read-only, so the false
> positives from the log writer go away.
>
> But I think the value the macro evaluates to is not what the kernel-doc
> above describes. The last statement is unlikely(_err), and unlikely(x)
> expands to __builtin_expect(!!(x), 0), so the statement expression
> evaluates to !!(_err), i.e. 0 or 1, not _err.  So the macro returns:
>
>         err == 0        -> 0
>         not read-only   -> 1   (documented as -EIO)
>         read-only       -> 1   (documented as -EROFS)
>
> For the WARN_ON() replacements that discard the return value this does
> not matter.  But nilfs_segctor_truncate_segments() consumes it as an
> error code:
>
>         ret = nilfs_sufile_warn_on_error(
>                 sufile, nilfs_sufile_free(sufile, segbuf->sb_nextnum));
>         if (ret && err != -EROFS)
>                 err = ret;
>         ...
>         return err;
>
> so it stores 1 rather than -EIO/-EROFS, nilfs_segctor_collect() then
> returns 1, and the "err != -EROFS" preference cannot distinguish the two
> codes.  That seems to contradict both the kernel-doc and the intent to
> "return the error code appropriately".
>
> Wouldn't ending the macro with plain
>
>                 _err;
>
> do what's intended?  The branch hint is already provided by the
> if (unlikely(_err)) above.

Thanks for pointing that out, Zhan Xusheng!

You are absolutely right; it was just a simple mistake on my part.
I'll make the correction and submit a v2 patch.

Thanks,
Ryusuke Konishi