[Cluster-devel] [PATCH] gfs2: ignore rindex_update failure in dinode_dealloc

[Andrew Price <anprice@redhat.com>]

Hi Bob,

On 04/05/2023 18:43, Bob Peterson wrote:
> Before this patch function gfs2_dinode_dealloc would abort if it got a
> bad return code from gfs2_rindex_update. The problem is that it left the
> dinode in the unlinked (not free) state, which meant subsequent fsck
> would clean it up and flag an error. That meant some of our QE tests
> would fail.

As I understand it the test is an interrupted rename loop workload and 
gfs2_grow at the same time, and the bad return code is -EINTR, right?

> The sole purpose of gfs2_rindex_update, in this code path, is to read in
> any newer rgrps added by gfs2_grow. But since this is a delete operation
> it won't actually use any of those new rgrps. It can really only twiddle
> the bits from "Unlinked" to "Free" in an existing rgrp. Therefore the
> error should not prevent the transition from unlinked to free.
> 
> This patch makes gfs2_dinode_dealloc ignore the bad return code and
> proceed with freeing the dinode so the QE tests will not be tripped up.

Is it really ok to ignore all potential errors here? I wonder if it 
should just ignore -EINTR (or whichever error the test produces) so that 
it can still fail well for errors like -EIO.

Cheers,
Andy

> 
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/super.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index d3b5c6278be0..1f23d7845123 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -1131,9 +1131,7 @@ static int gfs2_dinode_dealloc(struct gfs2_inode *ip)
>   		return -EIO;
>   	}
>   
> -	error = gfs2_rindex_update(sdp);
> -	if (error)
> -		return error;
> +	gfs2_rindex_update(sdp);



>   
>   	error = gfs2_quota_hold(ip, NO_UID_QUOTA_CHANGE, NO_GID_QUOTA_CHANGE);
>   	if (error)