On Tue, Mar 13, 2018 at 10:07:27PM -0500, Dave Chiluk wrote:
> On Mon, Mar 12, 2018 at 12:35 PM, Brian Foster <bfoster@xxxxxxxxxx> wrote:
> >
> > On Mon, Mar 12, 2018 at 09:11:58AM -0400, Brian Foster wrote:
> > Here's a variant of that patch that does a reset. It's definitely much
> > simpler. Thoughts?
> >
> > Brian
> >
> > --- 8< ---
> >
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index c02781a4c091..7d313bb4677d 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2053,6 +2053,59 @@ xfs_alloc_space_available(
> > return true;
> > }
> >
> > +static bool
> > +xfs_agf_verify_flcount(
> > + struct xfs_mount *mp,
> > + struct xfs_agf *agf)
> > +{
> > + int f = be32_to_cpu(agf->agf_flfirst);
> > + int l = be32_to_cpu(agf->agf_fllast);
> > + int c = be32_to_cpu(agf->agf_flcount);
> > + int active = c;
> > + int agfl_size = XFS_AGFL_SIZE(mp);
> > +
> > + if (!xfs_sb_version_hascrc(&mp->m_sb))
> > + return true;
> > +
> > + if (c && l >= f)
> > + active = l - f + 1;
> > + else if (c)
> > + active = agfl_size - f + l + 1;
> > +
> > + if (active != c)
> > + return false;
> > + if (f >= agfl_size || l >= agfl_size)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static void
> > +xfs_agfl_reset(
> > + struct xfs_trans *tp,
> > + struct xfs_buf *agbp,
> > + struct xfs_perag *pag)
> > +{
> > + struct xfs_mount *mp = tp->t_mountp;
> > + struct xfs_agf *agf = XFS_BUF_TO_AGF(agbp);
> > +
> > + if (!pag->pagf_needreset)
> > + return;
> > +
> > + trace_xfs_agfl_reset(pag);
> > + xfs_warn(mp, "agfl reset agno %u flcount %d", pag->pag_agno,
> > + pag->pagf_flcount);
> > +
>
> Before completely leaking the entirety of the agfl couldn't we nicely
> release and recover all blocks but the 119th first? That way we'd
> only be leaking the possibly problematic 119th item? I understand we
> would lose the benefit of being able to recover from otherwise corrupt
> AGFLs.
>
This is mostly covered in the discussions over the previously explored
methods. The synopsis is that yes, we could try to do something like
that, but the point of this approach is that we don't have to trust the
agfl content at all. This simplifies and genericizes the logic because
every kernel already knows how to populate a sane agfl from an empty
one.
> If we are going to blindly leak blocks wouldn't an xfs_repair recover
> these leaked blocks? I think it would be perfectly fine to leak these
> blocks if it means not crashing and then recover them at one's
> convenience with an xfs_repair.
>
Yes, an xfs_repair is necessary. An xfs_repair was already necessary to
fix the padding mismatch or whatever else might have been wrong. This
changes the side effect of the problem from a crash into a free space
accounting inconsistency.
> > + agf->agf_flfirst = 0;
> > + agf->agf_fllast = cpu_to_be32(XFS_AGFL_SIZE(mp) - 1);
> > + agf->agf_flcount = 0;
>
> Also I was under the impression that we should pre-allocate blocks in
> the agfl for fast allocation of free b+tree nodes. Wouldn't we want
> to pre-allocate some blocks as would be done by xfs_repair (I have a
> feeling someone is going to tell me where this happens elsewhere in
> the codebase or can be handled at block run time with little ill
> effect)?
>
The function that calls the reset (xfs_alloc_fix_freelist()) will
repopulate it once it sees that it is empty.
> If I'm correct in either case I'd appreciate a
> Reviewed by: Dave Chiluk <chiluk+linuxxfs@xxxxxxxxxx>
>
I'm going to defer this until posting a legitimate patch because it has
changed a bit (though not fundamentally). This post was more of a first
pass to sanity check the idea. I'd appreciate another look once a
legitimate v1 is posted.. thanks!
Brian
> Thanks,
> Dave
>
>
> > + xfs_alloc_log_agf(tp, agbp, XFS_AGF_FLFIRST | XFS_AGF_FLLAST |
> > + XFS_AGF_FLCOUNT);
> > +
> > + pag->pagf_flcount = 0;
> > + pag->pagf_needreset = false;
> > +}
> > +
> > /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
