On Mon, Nov 29, 2010 at 11:36:40AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> XFS has a per-cpu counter implementation for in-core superblock
> counters that pre-dated the generic implementation. It is complex
> and baroque as it is tailored directly to the needs of ENOSPC
> detection.
>
> Now that the generic percpu counter infrastructure has the
> percpu_counter_add_unless_lt() function that implements the
> necessary threshold checks for us, switch the XFS per-cpu
> superblock counters to use the generic percpu counter
> infrastructure.
Looks good, but a few comments below:
> +/*
> + * Per-cpu incore superblock counters
> + *
> + * Simple concept, difficult implementation, now somewhat simplified by generic
> + * per-cpu counter support. This provides distributed per cpu counters for
> + * contended fields (e.g. free block count).
The kind of historic comments like now simplified by .. don't make any
sense after only a short while. I'd just remove the first senstence
above, as the details of the problems are explained much better later.
> +static inline int
> +xfs_icsb_add(
> + struct xfs_mount *mp,
> + int counter,
> + int64_t delta,
> + int64_t threshold)
> +{
> + int ret;
> +
> + ret = percpu_counter_add_unless_lt(&mp->m_icsb[counter], delta,
> + threshold);
> + if (ret < 0)
> + return -ENOSPC;
> + return 0;
> +}
> +
> +static inline void
> +xfs_icsb_set(
> + struct xfs_mount *mp,
> + int counter,
> + int64_t value)
> +{
> + percpu_counter_set(&mp->m_icsb[counter], value);
> +}
> +
> +static inline int64_t
> +xfs_icsb_sum(
> + struct xfs_mount *mp,
> + int counter)
> +{
> + return percpu_counter_sum_positive(&mp->m_icsb[counter]);
> +}
I still don't like these wrappers. They are all local to xfs_mount.c,
and only have a single function calling them. See the RFC patch below
which removes them, and imho makes the code more readable. Especially
in xfs _add case where we can get rid of one level of error remapping,
and go directly from the weird percpu return values to the positive
xfs errors instead of doing a detour via the negative linux errors.
> +static inline int64_t
> +xfs_icsb_read(
> + struct xfs_mount *mp,
> + int counter)
> +{
> + return percpu_counter_read_positive(&mp->m_icsb[counter]);
> +}
this one is entirely unused.
Index: xfs/fs/xfs/xfs_mount.c
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.c 2010-11-29 09:43:31.423011248 +0100
+++ xfs/fs/xfs/xfs_mount.c 2010-11-29 09:56:32.546255345 +0100
@@ -282,54 +282,14 @@ xfs_free_perag(
* so we only need to modify the fast path to handle per-cpu counter error
* cases.
*/
-static inline int
-xfs_icsb_add(
- struct xfs_mount *mp,
- int counter,
- int64_t delta,
- int64_t threshold)
-{
- int ret;
-
- ret = percpu_counter_add_unless_lt(&mp->m_icsb[counter], delta,
- threshold);
- if (ret < 0)
- return -ENOSPC;
- return 0;
-}
-
-static inline void
-xfs_icsb_set(
- struct xfs_mount *mp,
- int counter,
- int64_t value)
-{
- percpu_counter_set(&mp->m_icsb[counter], value);
-}
-
-static inline int64_t
-xfs_icsb_sum(
- struct xfs_mount *mp,
- int counter)
-{
- return percpu_counter_sum_positive(&mp->m_icsb[counter]);
-}
-
-static inline int64_t
-xfs_icsb_read(
- struct xfs_mount *mp,
- int counter)
-{
- return percpu_counter_read_positive(&mp->m_icsb[counter]);
-}
-
void
xfs_icsb_reinit_counters(
struct xfs_mount *mp)
{
- xfs_icsb_set(mp, XFS_ICSB_FDBLOCKS, mp->m_sb.sb_fdblocks);
- xfs_icsb_set(mp, XFS_ICSB_IFREE, mp->m_sb.sb_ifree);
- xfs_icsb_set(mp, XFS_ICSB_ICOUNT, mp->m_sb.sb_icount);
+ percpu_counter_set(&mp->m_icsb[XFS_ICSB_FDBLOCKS],
+ mp->m_sb.sb_fdblocks);
+ percpu_counter_set(&mp->m_icsb[XFS_ICSB_IFREE], mp->m_sb.sb_ifree);
+ percpu_counter_set(&mp->m_icsb[XFS_ICSB_ICOUNT], mp->m_sb.sb_icount);
}
int
@@ -368,9 +328,12 @@ xfs_icsb_sync_counters(
xfs_mount_t *mp)
{
assert_spin_locked(&mp->m_sb_lock);
- mp->m_sb.sb_icount = xfs_icsb_sum(mp, XFS_ICSB_ICOUNT);
- mp->m_sb.sb_ifree = xfs_icsb_sum(mp, XFS_ICSB_IFREE);
- mp->m_sb.sb_fdblocks = xfs_icsb_sum(mp, XFS_ICSB_FDBLOCKS);
+ mp->m_sb.sb_icount =
+ percpu_counter_sum_positive(&mp->m_icsb[XFS_ICSB_ICOUNT]);
+ mp->m_sb.sb_ifree =
+ percpu_counter_sum_positive(&mp->m_icsb[XFS_ICSB_IFREE]);
+ mp->m_sb.sb_fdblocks =
+ percpu_counter_sum_positive(&mp->m_icsb[XFS_ICSB_FDBLOCKS]);
}
/*
@@ -1953,7 +1916,7 @@ xfs_icsb_modify_counters(
switch (field) {
case XFS_SBS_ICOUNT:
- ret = xfs_icsb_add(mp, cntr, delta, 0);
+ ret = percpu_counter_add_unless_lt(&mp->m_icsb[XFS_SBS_ICOUNT], delta, 0);
if (ret < 0) {
ASSERT(0);
return XFS_ERROR(EINVAL);
@@ -1961,7 +1924,7 @@ xfs_icsb_modify_counters(
return 0;
case XFS_SBS_IFREE:
- ret = xfs_icsb_add(mp, XFS_ICSB_IFREE, delta, 0);
+ ret = percpu_counter_add_unless_lt(&mp->m_icsb[XFS_SBS_IFREE], delta, 0);
if (ret < 0) {
ASSERT(0);
return XFS_ERROR(EINVAL);
@@ -1990,13 +1953,12 @@ xfs_icsb_modify_counters(
}
/* try the change */
- ret = xfs_icsb_add(mp, XFS_ICSB_FDBLOCKS, delta,
- XFS_ALLOC_SET_ASIDE(mp));
+ ret = percpu_counter_add_unless_lt(&mp->m_icsb[XFS_ICSB_FDBLOCKS],
+ delta, XFS_ALLOC_SET_ASIDE(mp));
if (likely(ret >= 0))
return 0;
/* ENOSPC */
- ASSERT(ret == -ENOSPC);
ASSERT(delta < 0);
if (!rsvd)
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
