Re: [PATCH v2] ext4: drop extra ea_inode ref in cached-block reuse path

On Fri 14-08-26 14:50:23, Matthias Goergens wrote:
> ext4_xattr_block_set() acquires a reference to the ea_inode via
> ext4_xattr_inode_lookup_create() before constructing the new xattr
> block.  In the new-block allocation path this extra reference is dropped
> after ext4_xattr_inode_inc_ref_all() accounts for every entry in the
> block.  But when mbcache supplies an identical existing block and that
> block is reused, the cached block already carries the correct refcount
> for its entries, and the extra reference is never dropped.
> 
> The cleanup block at the end of the function only calls
> ext4_xattr_inode_dec_ref() when error is non-zero, so the success path
> through cache reuse leaks one refcount.  Each reuse event adds another;
> eventually the saturated refcount prevents the ea_inode from being freed
> when its last real reference is dropped.
> 
> Add the matching drop when the entry was not written directly to the old
> physical buffer (bs->bh && s->base == bs->bh->b_data).  In that case the
> reference belongs to the old buffer, which the later release_block()
> handles.  When s->base is a clone or a fresh allocation and is discarded
> in favour of the cached block, the extra reference must be dropped.
> 

Good find!

> v2:
> - The v1 dropped the reference best-effort: a failure was logged but
>   masked by the later "error = 0" on the success path, so the syscall
>   could succeed with the on-disk refcount one too high.  Fail the
>   operation instead, and undo the locally-owned part of the reuse
>   through ext4_xattr_reuse_undo(), the same helper the success path
>   uses for the old block.  Any reference that nonetheless persists
>   (e.g. under errors=continue) is repaired by e2fsck.
> - A failed decrement is never retried: it may already have mutated
>   the refcount before failing (e.g. an EA inode inheriting S_SYNC from
>   the root directory can fail ext4_mark_iloc_dirty() after the
>   mutation in nojournal mode), so a retry could decrement twice.  The
>   failure paths free the quota charge and drop the inode reference
>   directly.

The only reasons why ext4_xattr_inode_dec_ref() can fail are catastrophic
fs failures => no chance of keeping the fs consistent anyway. So just
reporting the issue with ext4_error_inode() and otherwise bailing out with
the least effort is the right thing. Sadly Sashiko is giving misguiding
recommendations in these cases.

> - Propagate the same masked failure at the new-block branch's
>   identical drop: it has always been best-effort (warn and continue).

This changelog belongs below the diffstat so that it doesn't get included
into the commit message. The comment about Sashiko belongs to the proper
changelog.

> Found by the Sashiko AI review bot while reviewing the xattr
> saturation-livelock fix.
> 
> Fixes: 0a46ef234756 ("ext4: do not create EA inode under buffer lock")
> Signed-off-by: Matthias Goergens <[email protected]>
> ---
>  fs/ext4/xattr.c | 62 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 982a1f831e228..0ce937d890042 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1362,6 +1362,17 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
>  	return;
>  }
>  
> +/* Undo the locally-owned part of a cached-block reuse. */
> +static void ext4_xattr_reuse_undo(handle_t *handle, struct inode *inode,
> +				  struct buffer_head *bh)
> +{
> +	struct ext4_xattr_inode_array *ea_inode_array = NULL;
> +
> +	ext4_xattr_release_block(handle, inode, bh, &ea_inode_array,
> +				 0 /* extra_credits */);
> +	ext4_xattr_inode_array_free(ea_inode_array);
> +}
> +
>  /*
>   * Find the available free space for EAs. This also returns the total number of
>   * bytes used by EA entries.
> @@ -2107,6 +2118,39 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  			mb_cache_entry_touch(ea_block_cache, ce);
>  			mb_cache_entry_put(ea_block_cache, ce);
>  			ce = NULL;
> +			if (!(bs->bh && s->base == bs->bh->b_data) &&
> +			    ea_inode) {
> +				/*
> +				 * The reused block already holds its own
> +				 * reference; drop the extra one unless the
> +				 * entry went into the old buffer directly.
> +				 */
> +				error = ext4_xattr_inode_dec_ref(handle,
> +								 ea_inode);

I can be missing something but why don't you just call
ext4_xattr_inode_dec_ref() like:
		if (new_bh) {
			if (new_bh == bs->bh)
			else {
				...
				/* here */
				if (ea_inode)
					ext4_xattr_inode_dec_ref()
			}

and be done with it? No need for more complicated condition, also no need
for iput() or clearing of ea_inode. That would look like the easiest
solution to me but I might be missing something subtle.

								Honza

> +				if (error) {
> +					/*
> +					 * The decrement may already have run;
> +					 * do not retry it: free the charge,
> +					 * undo the reuse, and fail the op.
> +					 */
> +					ext4_error_inode(inode, __func__,
> +							 __LINE__, 0,
> +							"dec ref error=%d",
> +							 error);
> +					ext4_xattr_inode_free_quota(inode,
> +								    ea_inode,
> +								    i_size_read(ea_inode));
> +					iput(ea_inode);
> +					ea_inode = NULL;
> +					if (new_bh != bs->bh)
> +						ext4_xattr_reuse_undo(handle,
> +								      inode,
> +								      new_bh);
> +					goto cleanup;
> +				}
> +				iput(ea_inode);
> +				ea_inode = NULL;
> +			}
>  		} else if (bs->bh && s->base == bs->bh->b_data) {
>  			/* We were modifying this block in-place. */
>  			ea_bdebug(bs->bh, "keeping this block");
> @@ -2143,13 +2187,21 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  			if (error)
>  				goto getblk_failed;
>  			if (ea_inode) {
> -				/* Drop the extra ref on ea_inode. */
> +				/*
> +				 * Drop the extra ref on ea_inode; on
> +				 * failure the decrement may already have
> +				 * run, so just fail and free the block.
> +				 */
>  				error = ext4_xattr_inode_dec_ref(handle,
>  								 ea_inode);
> -				if (error)
> -					ext4_warning_inode(ea_inode,
> -							   "dec ref error=%d",
> -							   error);
> +				if (error) {
> +					ext4_xattr_inode_free_quota(inode,
> +								    ea_inode,
> +								    i_size_read(ea_inode));
> +					iput(ea_inode);
> +					ea_inode = NULL;
> +					goto getblk_failed;
> +				}
>  				iput(ea_inode);
>  				ea_inode = NULL;
>  			}
> -- 
> 2.55.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR