Re: [PATCH 06/71] xfs: set up per-AG free space reservations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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



[Index of Archives]     [Linux XFS Devel]     [Linux Filesystem Development]     [Filesystem Testing]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux