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..
>
> > + return avail < orig / 10 || avail < XFS_BTREE_MAXLEVELS;
>
> Where does this magic 10 come from?
>
> > + /*
> > + * 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
