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

[Steven Whitehouse <swhiteho@redhat.com>]

On Fri, 2023-05-05 at 08:44 +0100, Andrew Price wrote:
> 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
> 
Perhaps the more important question is why there are errors there in
the first place?

Steve.

> > 
> > 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)
>