> 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.
> +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.
> +
> +init_agfl:
Why not a simple if instead of the goto above?
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
