On Fri, Oct 26, 2018 at 03:24:24AM -0700, Christoph Hellwig wrote:
> > @@ -1767,11 +1767,10 @@ xfs_dir2_node_find_freeblk(
> > fbp = fblk->bp;
> > free = fbp->b_addr;
> > findex = fblk->index;
> > + bests = dp->d_ops->free_bests_p(free);
> > + dp->d_ops->free_hdr_from_disk(&freehdr, free);
> > if (findex >= 0) {
> > /* caller already found the freespace for us. */
> > - bests = dp->d_ops->free_bests_p(free);
> > - dp->d_ops->free_hdr_from_disk(&freehdr, free);
> > -
>
> This hunk just undoes a move in the previous patch, might be better
> idea to just keep it as before there.
>
> > + for ( ; fbno < lastfbno; fbno++) {
> > + /* If we don't have a freeblock in hand, get the next one. */
> > if (fbp == NULL) {
> > + /* If it's ifbno we already looked at it. */
> > + if (fbno == ifbno)
> > + continue;
> > +
>
> The only case where we have a fbp here is if we had a fblk passed in,
> but it it did have the index set to -1. But as far as I can tell
> searching that again doesn't make any sense at all, so I'd apply
> something like this in top of your patch (some of this also seems
> to be in your next patch, so independent of the logic change might
> be worth moving over here):
So you've done a bunch of the rework that already in the next patch
in the series, plus a "fbno = fblk->blkno + 1;" logic change.
Perhaps put this logic change at end of the series, rather than in
the middle where it breaks the rest of the changes?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
