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 <rpete...@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) >
