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

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,
Zhan Xusheng