Re: [PATCH 5/7] repair: don't duplicate names in phase 6

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 5/7] repair: don't duplicate names in phase 6
Date: Thu, 22 Oct 2020 19:23:54 +1100	[thread overview]
Message-ID: <20201022082354.GU7391@dread.disaster.area> (raw)
In-Reply-To: <20201022062152.GQ9832@magnolia>

On Wed, Oct 21, 2020 at 11:21:52PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 22, 2020 at 04:15:35PM +1100, Dave Chinner wrote:
....
> > @@ -1387,6 +1371,7 @@ dir2_kill_block(
> >  		res_failed(error);
> >  	libxfs_trans_ijoin(tp, ip, 0);
> >  	libxfs_trans_bjoin(tp, bp);
> > +	libxfs_trans_bhold(tp, bp);
> 
> Why hold on to the buffer?  We killed the block, why keep the reference
> around so that someone else has to remember to drop it later?

Consistency in buffer handling. This "kill block" path nulled out
the buffer in the bplist array (the reason it's passed as a **bpp
to longform_dir2_entry_check_data(). This path releases the buffer
through the trans_commit(), the alternate path here:

> > @@ -1558,10 +1541,8 @@ longform_dir2_entry_check_data(
> >  			dir2_kill_block(mp, ip, da_bno, bp);
> >  		} else {
> >  			do_warn(_("would junk block\n"));
> > -			libxfs_buf_relse(bp);
> >  		}
> >  		freetab->ents[db].v = NULLDATAOFF;
> > -		*bpp = NULL;
> >  		return;
> >  	}

does an explicit release, and all other paths end up with the
buffers being released way back where the bplist is defined.

If the directory is in block form, nulling out the buffer in the
bplist here will result in dereferencing a null pointer later when
the buffer is pulled from bplist[0] without checking.

So I changed longform_dir2_entry_check_data() to never release the
buffer so that the caller knows that it has always got a valid
buffer reference and the isblock path will always work correctly....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com