On Tue, Sep 06, 2016 at 10:31:04AM -0700, Darrick J. Wong wrote:
> On Tue, Sep 06, 2016 at 07:53:49AM -0700, Christoph Hellwig wrote:
> > > v2: There's really only two kinds of per-AG reservation pools -- one
> > > to feed the AGFL (rmapbt), and one to feed everything else
> > > (refcountbt). Bearing that in mind, we can embed the reservation
> > > controls in xfs_perag and greatly simplify the block accounting.
> > > Furthermore, fix some longstanding accounting bugs that were a direct
> > > result of the goofy "allocate a block and later fix up the accounting"
> > > strategy by integrating the reservation accounting code more tightly
> > > with the allocator. This eliminates the ENOSPC complaints resulting
> > > from refcount btree splits during truncate operations.
> > >
> > > v3: Since AGFL blocks are considered to be "free", we mustn't change
> > > fdblocks in response to any AGFL grow/shrink operation. Therefore, we
> > > must give back to fdblocks at umount whatever we borrowed at mount.
> > > We have to let ag_reserved go down to zero because it's used to
> > > determine if we're critically low on reservation.
> >
> > The v2/v3 would belong below the --- line. But there's some pretty
> > useful information in here, so it might be worth incorporating some
> > of that into the main changelog.
> >
> > > +bool
> > > +xfs_ag_resv_critical(
> > > + struct xfs_perag *pag,
> > > + enum xfs_ag_resv_type type)
> > > +{
> > > + xfs_extlen_t avail;
> > > + xfs_extlen_t orig;
> > > +
> > > + switch (type) {
> > > + case XFS_AG_RESV_METADATA:
> > > + avail = pag->pagf_freeblks - pag->pag_agfl_resv.ar_reserved;
> > > + orig = pag->pag_meta_resv.ar_asked;
> > > + break;
> >
> > This doesn't actually seem to be used in the whole series. I can see
> > why you'd want it for completeness, but that also means the code here
> > is pretty much guaranteed to be unused..
Drat, missed a couple of comments.
I thought about this some more and decided that reflink should reject
if either reservation is low, since the fallback is a standard file
copy whose blocks will (probably?) come from another AG.
> >
> > > + return avail < orig / 10 || avail < XFS_BTREE_MAXLEVELS;
> >
> > Where does this magic 10 come from?
10% of the original reservation, as stated in the comment before the
function. I'll reiterate that above the return statement.
--D
> >
> > > + /*
> > > + * AGFL blocks are always considered "free", so whatever
> > > + * was reserved at mount time must be given back at umount.
> > > + */
> > > + oldresv = (type == XFS_AG_RESV_AGFL ? resv->ar_orig_reserved :
> > > + resv->ar_reserved);
> >
> > Nitpick: Using a plain old if/else here would be a fair bit more
> > readable.
>
> Ok.
>
> > > +int
> > > +xfs_ag_resv_free(
> > > + struct xfs_perag *pag)
> > > +{
> > > + int error = 0;
> > > + int err2;
> > > +
> > > + err2 = __xfs_ag_resv_free(pag, XFS_AG_RESV_AGFL);
> > > + if (err2 && !error)
> > > + error = err2;
> >
> > error is always 0 here. So a simple:
> >
> > error = __xfs_ag_resv_free(pag, XFS_AG_RESV_AGFL);
> >
> > Also why not error and error2 or err and err2 to be consistent?
> >
> > > + xfs_extlen_t ask;
> > > + xfs_extlen_t used;
> > > + int error = 0;
> > > + int err2;
> > > +
> > > + if (pag->pag_meta_resv.ar_asked)
> > > + goto init_agfl;
> > > +
> > > + /* Create the metadata reservation. */
> > > + ask = used = 0;
> > > +
> > > + err2 = __xfs_ag_resv_init(pag, XFS_AG_RESV_METADATA, ask, used);
> > > + if (err2 && !error)
> > > + error = err2;
> >
> > Same error is always null case here.
>
> A few revisions ago I would still allow XFS to limp along if any part of this
> AG reservation calculation or initialization failed, but nowadays it's been
> changed to take the FS offline at the first sign of trouble, so both can turn
> into the usual if (error) return error; paradigm.
>
> > > +
> > > +init_agfl:
> >
> > Why not a simple if instead of the goto above?
>
> --D
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
--
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
