On Fri, Oct 02, 2015 at 11:22:39AM -0500, Bill O'Donnell wrote:
> This patch implements per-filesystem stats objects in sysfs. It
> depends on the application of the previous patch series that
> develops the infrastructure to support both xfs global stats and
> xfs per-fs stats in sysfs.
>
> Stats objects are instantiated when an xfs filesystem is mounted
> and deleted on unmount. With this patch, the stats directory is
> created and populated with the familiar stats and stats_clear files.
> Example:
> /sys/fs/xfs/sda9/stats/stats
> /sys/fs/xfs/sda9/stats/stats_clear
>
> With this patch, the individual counts within the new per-fs
> stats file(s) remain at zero. Functions that use the the macros
> to increment, decrement, and add-to the per-fs stats counts will
> be covered in a separate new patch to follow this one. Note that
> the counts within the global stats file (/sys/fs/xfs/stats/stats)
> advance normally and can be cleared as it was prior to this patch.
>
> Signed-off-by: Bill O'Donnell <billodo@xxxxxxxxxx>
.....
> @@ -1047,6 +1065,10 @@ xfs_unmountfs(
> xfs_warn(mp, "Unable to update superblock counters. "
> "Freespace may not be correct on next mount.");
>
> + /* remove the stats kobject and free stats memory */
> + xfs_sysfs_del(&mp->m_stats.xs_kobj);
> + free_percpu(mp->m_stats.xs_stats);
> +
> xfs_log_unmount(mp);
Ok, there's an obvious (to me!) problem here. xfs_log_unmount() is
called while there are still active objects in the AIL, which means
there is still AIL operations that need to be done. The AIL
operations have stats in them, so this will result in use-after-free
of the mp->m_stats structure here.
Actually, looking at the alloc side of this patch, we're doing a
XFS_STATS_INC(xb_create) through:
xfs_fs_fill_super
xfs_readsb
xfs_buf_read_uncached
xfs_buf_get_uncached
_xfs_buf_alloc
XFS_STATS_INC(target->bt_mount, xb_create)
at mount time before we've set up mp->m_stats. Ok, so the
initialisation and teardown of the per-mount stats memory is being
done at the wrong layer - it needs to be done in xfs_fs_fill_super()
before we do any IO at all and dropped in xfs_fs_put_super() after
we've torn down all the IO structures. The sysfs object
initialisation is fine where it is, but the actual stats storage is
needed earlier/later than the sysfs object is.
/me now wonders why this doesn't go kaboom at mount time...
Reworked patch is inline below.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
xfs: per-filesystem stats in sysfs
From: Bill O'Donnell <billodo@xxxxxxxxxx>
This patch implements per-filesystem stats objects in sysfs. It
depends on the application of the previous patch series that
develops the infrastructure to support both xfs global stats and
xfs per-fs stats in sysfs.
Stats objects are instantiated when an xfs filesystem is mounted
and deleted on unmount. With this patch, the stats directory is
created and populated with the familiar stats and stats_clear files.
Example:
/sys/fs/xfs/sda9/stats/stats
/sys/fs/xfs/sda9/stats/stats_clear
With this patch, the individual counts within the new per-fs
stats file(s) remain at zero. Functions that use the the macros
to increment, decrement, and add-to the per-fs stats counts will
be covered in a separate new patch to follow this one. Note that
the counts within the global stats file (/sys/fs/xfs/stats/stats)
advance normally and can be cleared as it was prior to this patch.
[dchinner: move setup/teardown to xfs_fs_{fill|put}_super() so
it is down before/after any path that uses the per-mount stats. ]
Signed-off-by: Bill O'Donnell <billodo@xxxxxxxxxx>
Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>
Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>
---
fs/xfs/xfs_mount.c | 11 ++++++++++-
fs/xfs/xfs_mount.h | 1 +
fs/xfs/xfs_super.c | 14 ++++++++++++--
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index bf92e0c..eb498ce 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -693,10 +693,15 @@ xfs_mountfs(
if (error)
goto out;
- error = xfs_uuid_mount(mp);
+ error = xfs_sysfs_init(&mp->m_stats.xs_kobj, &xfs_stats_ktype,
+ &mp->m_kobj, "stats");
if (error)
goto out_remove_sysfs;
+ error = xfs_uuid_mount(mp);
+ if (error)
+ goto out_del_stats;
+
/*
* Set the minimum read and write sizes
*/
@@ -971,6 +976,8 @@ xfs_mountfs(
xfs_da_unmount(mp);
out_remove_uuid:
xfs_uuid_unmount(mp);
+ out_del_stats:
+ xfs_sysfs_del(&mp->m_stats.xs_kobj);
out_remove_sysfs:
xfs_sysfs_del(&mp->m_kobj);
out:
@@ -1047,6 +1054,7 @@ xfs_unmountfs(
xfs_warn(mp, "Unable to update superblock counters. "
"Freespace may not be correct on next mount.");
+
xfs_log_unmount(mp);
xfs_da_unmount(mp);
xfs_uuid_unmount(mp);
@@ -1056,6 +1064,7 @@ xfs_unmountfs(
#endif
xfs_free_perag(mp);
+ xfs_sysfs_del(&mp->m_stats.xs_kobj);
xfs_sysfs_del(&mp->m_kobj);
}
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 404bfa5..f20e5de 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -127,6 +127,7 @@ typedef struct xfs_mount {
int64_t m_low_space[XFS_LOWSP_MAX];
/* low free space thresholds */
struct xfs_kobj m_kobj;
+ struct xstats m_stats; /* per-fs stats */
struct workqueue_struct *m_buf_workqueue;
struct workqueue_struct *m_data_workqueue;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e1a35a5..9d11d3e 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1474,9 +1474,16 @@ xfs_fs_fill_super(
if (error)
goto out_destroy_workqueues;
+ /* Allocate stats memory before we do operations that might use it */
+ mp->m_stats.xs_stats = alloc_percpu(struct xfsstats);
+ if (!mp->m_stats.xs_stats) {
+ error = PTR_ERR(mp->m_stats.xs_stats);
+ goto out_destroy_counters;
+ }
+
error = xfs_readsb(mp, flags);
if (error)
- goto out_destroy_counters;
+ goto out_free_stats;
error = xfs_finish_flags(mp);
if (error)
@@ -1545,9 +1552,11 @@ xfs_fs_fill_super(
xfs_filestream_unmount(mp);
out_free_sb:
xfs_freesb(mp);
+ out_free_stats:
+ free_percpu(mp->m_stats.xs_stats);
out_destroy_counters:
xfs_destroy_percpu_counters(mp);
-out_destroy_workqueues:
+ out_destroy_workqueues:
xfs_destroy_mount_workqueues(mp);
out_close_devices:
xfs_close_devices(mp);
@@ -1574,6 +1583,7 @@ xfs_fs_put_super(
xfs_unmountfs(mp);
xfs_freesb(mp);
+ free_percpu(mp->m_stats.xs_stats);
xfs_destroy_percpu_counters(mp);
xfs_destroy_mount_workqueues(mp);
xfs_close_devices(mp);
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
