Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
On Fri, 2007-11-30 at 17:08 -0600, Eric Sandeen wrote: Mingming Cao wrote: [PATCH] jbd2 stats through procfs The patch below updates the jbd stats patch to 2.6.20/jbd2. The initial patch was posted by Alex Tomas in December 2005 (http://marc.info/?l=linux-ext4m=113538565128617w=2). It provides statistics via procfs such as transaction lifetime and size. [ This probably should be rewritten to use debugfs? -- Ted] Signed-off-by: Johann Lombardi [EMAIL PROTECTED] I've started going through this one to clean it up to the point where it can go forward. It's been sitting at the top of the unstable portion of the patch queue for long enough, I think :) Thanks! I've converted the msecs to jiffies until the user boundary, changed the union #defines as suggested by Andrew, and various other little issues etc. Remaining to do is a generic time-difference calculator (instead of jbd2_time_diff), and looking into whether it should be made a config option; I tend to think it should, but it's fairly well sprinkled through the code, so I'll see how well that works. Also did we ever decided if this should go to debugfs? I thought it was decided to keep it on procfs as debugfs is not always on... Thanks, -Eric - To unsubscribe from this list: send the line unsubscribe linux-fsdevel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
Mingming Cao wrote: [PATCH] jbd2 stats through procfs The patch below updates the jbd stats patch to 2.6.20/jbd2. The initial patch was posted by Alex Tomas in December 2005 (http://marc.info/?l=linux-ext4m=113538565128617w=2). It provides statistics via procfs such as transaction lifetime and size. [ This probably should be rewritten to use debugfs? -- Ted] Signed-off-by: Johann Lombardi [EMAIL PROTECTED] I've started going through this one to clean it up to the point where it can go forward. It's been sitting at the top of the unstable portion of the patch queue for long enough, I think :) I've converted the msecs to jiffies until the user boundary, changed the union #defines as suggested by Andrew, and various other little issues etc. Remaining to do is a generic time-difference calculator (instead of jbd2_time_diff), and looking into whether it should be made a config option; I tend to think it should, but it's fairly well sprinkled through the code, so I'll see how well that works. Also did we ever decided if this should go to debugfs? Thanks, -Eric - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
On Tue, 2007-07-10 at 19:31 -0700, Andrew Morton wrote:
On Sun, 01 Jul 2007 03:38:10 -0400 Mingming Cao [EMAIL PROTECTED] wrote:
[PATCH] jbd2 stats through procfs
The patch below updates the jbd stats patch to 2.6.20/jbd2.
The initial patch was posted by Alex Tomas in December 2005
(http://marc.info/?l=linux-ext4m=113538565128617w=2).
It provides statistics via procfs such as transaction lifetime and size.
[ This probably should be rewritten to use debugfs? -- Ted]
I suppose that given that we're creating a spot in debugfs for the jbd2
debug code, yes, this also should be moved over.
But the jbd2 debug debugfs entries were kernel-wide whereas this is
per-superblock. I think.
I take this comment as we still keep the stats info in proc fs...
That email from Alex contains pretty important information. I suggest that
it be added to the changelog after accuracy-checking. Addition to
Documentation/filesystems/ext4.txt would be good.
Done.
I hope Alex can help address the rest of comments.
--
Index: linux-2.6.22-rc4/include/linux/jbd2.h
===
--- linux-2.6.22-rc4.orig/include/linux/jbd2.h 2007-06-11
17:28:17.0 -0700
+++ linux-2.6.22-rc4/include/linux/jbd2.h 2007-06-13 10:45:21.0
-0700
@@ -408,6 +408,16 @@
};
+/*
+ * Some stats for checkpoint phase
+ */
+struct transaction_chp_stats_s {
+ unsigned long cs_chp_time;
+ unsigned long cs_forced_to_close;
+ unsigned long cs_written;
+ unsigned long cs_dropped;
+};
It would be nice to document what units all these fields are in. Jiffies,
I assume.
/* The transaction_t type is the guts of the journaling mechanism. It
* tracks a compound transaction through its various states:
*
@@ -543,6 +553,21 @@
spinlock_t t_handle_lock;
/*
+* Longest time some handle had to wait for running transaction
+*/
+ unsigned long t_max_wait;
+
+ /*
+* When transaction started
+*/
+ unsigned long t_start;
+
+ /*
+* Checkpointing stats [j_checkpoint_sem]
+*/
+ struct transaction_chp_stats_s t_chp_stats;
+
+ /*
* Number of outstanding updates running on this transaction
* [t_handle_lock]
*/
@@ -573,6 +598,57 @@
};
+struct transaction_run_stats_s {
+ unsigned long rs_wait;
+ unsigned long rs_running;
+ unsigned long rs_locked;
+ unsigned long rs_flushing;
+ unsigned long rs_logging;
+
+ unsigned long rs_handle_count;
+ unsigned long rs_blocks;
+ unsigned long rs_blocks_logged;
+};
+
+struct transaction_stats_s
+{
+ int ts_type;
+ unsigned long ts_tid;
+ union {
+ struct transaction_run_stats_s run;
+ struct transaction_chp_stats_s chp;
+ } u;
+};
+
+#define JBD2_STATS_RUN 1
+#define JBD2_STATS_CHECKPOINT 2
+
+#define ts_waitu.run.rs_wait
+#define ts_running u.run.rs_running
+#define ts_locked u.run.rs_locked
+#define ts_flushingu.run.rs_flushing
+#define ts_logging u.run.rs_logging
+#define ts_handle_countu.run.rs_handle_count
+#define ts_blocks u.run.rs_blocks
+#define ts_blocks_logged u.run.rs_blocks_logged
+
+#define ts_chp_timeu.chp.cs_chp_time
+#define ts_forced_to_close u.chp.cs_forced_to_close
+#define ts_written u.chp.cs_written
+#define ts_dropped u.chp.cs_dropped
That's a bit sleazy. We can drop the u from 'struct transaction_stats_s'
and make it an anonymous union, then open-code foo.run.rs_wait everywhere.
But that's a bit sleazy too, because the reader of the code would not know
that a write to foo.run.rs_wait will stomp on the value of
foo.chp.cs_chp_time.
So to minimize reader confusion it would be best I think to just open-code
the full u.run.rs_wait at all code-sites.
The macros above are the worst possible choice: they hide information from
the code-reader just to save the code-writer a bit of typing. But we very
much want to optimise for code-readers, not for code-writers.
+#define CURRENT_MSECS (jiffies_to_msecs(jiffies))
hm, that isn't something which should be in an ext4 header file. And it
shouldn't be in UPPER CASE and it shouldn't pretend to be a constant (or a
global scalar).
IOW: yuk.
How's about raising a separate, standalone patch which creates a new
kernel-wide coded-in-C function such as
unsigned long jiffies_in_msecs(void);
? (That's assuming we don't already have one. Most likely we have seven
of them hiding in various dark corners).
+static inline
Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
On Tue, 2007-07-10 at 21:42 -0700, Andrew Morton wrote:
On Tue, 10 Jul 2007 23:21:49 -0400 Cédric Augonnet [EMAIL PROTECTED]
wrote:
2007/7/10, Andrew Morton [EMAIL PROTECTED]:
Hi all,
+ size = sizeof(struct transaction_stats_s);
+ s-stats = kmalloc(size, GFP_KERNEL);
+ if (s == NULL) {
+ kfree(s);
+ return -EIO;
ENOMEM
I'm sorry if i missed some point, but i just don't see the use of such
a kfree here, not sure Andrew meant you should only return ENOMEM
instead, but why issuing those kfree(NULL), instead of just a if (!s)
return ENOMEM ?
You found a bug. It was meant to be
if (s-stats == NULL)
The incremental fix patch is attached just FYI. It will be folded to the
parent patch once the parent patch is being updated.
---
fs/jbd2/journal.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6.22/fs/jbd2/journal.c
===
--- linux-2.6.22.orig/fs/jbd2/journal.c 2007-07-16 00:05:03.0 -0700
+++ linux-2.6.22/fs/jbd2/journal.c 2007-07-16 00:05:59.0 -0700
@@ -751,7 +751,7 @@ static int jbd2_seq_history_open(struct
return -EIO;
size = sizeof(struct transaction_stats_s) * journal-j_history_max;
s-stats = kmalloc(size, GFP_KERNEL);
- if (s == NULL) {
+ if (s-stats == NULL) {
kfree(s);
return -EIO;
}
-
To unsubscribe from this list: send the line unsubscribe linux-ext4 in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
On Sun, 01 Jul 2007 03:38:10 -0400 Mingming Cao [EMAIL PROTECTED] wrote:
[PATCH] jbd2 stats through procfs
The patch below updates the jbd stats patch to 2.6.20/jbd2.
The initial patch was posted by Alex Tomas in December 2005
(http://marc.info/?l=linux-ext4m=113538565128617w=2).
It provides statistics via procfs such as transaction lifetime and size.
[ This probably should be rewritten to use debugfs? -- Ted]
I suppose that given that we're creating a spot in debugfs for the jbd2
debug code, yes, this also should be moved over.
But the jbd2 debug debugfs entries were kernel-wide whereas this is
per-superblock. I think.
That email from Alex contains pretty important information. I suggest that
it be added to the changelog after accuracy-checking. Addition to
Documentation/filesystems/ext4.txt would be good.
--
Index: linux-2.6.22-rc4/include/linux/jbd2.h
===
--- linux-2.6.22-rc4.orig/include/linux/jbd2.h2007-06-11
17:28:17.0 -0700
+++ linux-2.6.22-rc4/include/linux/jbd2.h 2007-06-13 10:45:21.0
-0700
@@ -408,6 +408,16 @@
};
+/*
+ * Some stats for checkpoint phase
+ */
+struct transaction_chp_stats_s {
+ unsigned long cs_chp_time;
+ unsigned long cs_forced_to_close;
+ unsigned long cs_written;
+ unsigned long cs_dropped;
+};
It would be nice to document what units all these fields are in. Jiffies,
I assume.
/* The transaction_t type is the guts of the journaling mechanism. It
* tracks a compound transaction through its various states:
*
@@ -543,6 +553,21 @@
spinlock_t t_handle_lock;
/*
+ * Longest time some handle had to wait for running transaction
+ */
+ unsigned long t_max_wait;
+
+ /*
+ * When transaction started
+ */
+ unsigned long t_start;
+
+ /*
+ * Checkpointing stats [j_checkpoint_sem]
+ */
+ struct transaction_chp_stats_s t_chp_stats;
+
+ /*
* Number of outstanding updates running on this transaction
* [t_handle_lock]
*/
@@ -573,6 +598,57 @@
};
+struct transaction_run_stats_s {
+ unsigned long rs_wait;
+ unsigned long rs_running;
+ unsigned long rs_locked;
+ unsigned long rs_flushing;
+ unsigned long rs_logging;
+
+ unsigned long rs_handle_count;
+ unsigned long rs_blocks;
+ unsigned long rs_blocks_logged;
+};
+
+struct transaction_stats_s
+{
+ int ts_type;
+ unsigned long ts_tid;
+ union {
+ struct transaction_run_stats_s run;
+ struct transaction_chp_stats_s chp;
+ } u;
+};
+
+#define JBD2_STATS_RUN 1
+#define JBD2_STATS_CHECKPOINT2
+
+#define ts_wait u.run.rs_wait
+#define ts_running u.run.rs_running
+#define ts_lockedu.run.rs_locked
+#define ts_flushing u.run.rs_flushing
+#define ts_logging u.run.rs_logging
+#define ts_handle_count u.run.rs_handle_count
+#define ts_blocksu.run.rs_blocks
+#define ts_blocks_logged u.run.rs_blocks_logged
+
+#define ts_chp_time u.chp.cs_chp_time
+#define ts_forced_to_close u.chp.cs_forced_to_close
+#define ts_written u.chp.cs_written
+#define ts_dropped u.chp.cs_dropped
That's a bit sleazy. We can drop the u from 'struct transaction_stats_s'
and make it an anonymous union, then open-code foo.run.rs_wait everywhere.
But that's a bit sleazy too, because the reader of the code would not know
that a write to foo.run.rs_wait will stomp on the value of
foo.chp.cs_chp_time.
So to minimize reader confusion it would be best I think to just open-code
the full u.run.rs_wait at all code-sites.
The macros above are the worst possible choice: they hide information from
the code-reader just to save the code-writer a bit of typing. But we very
much want to optimise for code-readers, not for code-writers.
+#define CURRENT_MSECS(jiffies_to_msecs(jiffies))
hm, that isn't something which should be in an ext4 header file. And it
shouldn't be in UPPER CASE and it shouldn't pretend to be a constant (or a
global scalar).
IOW: yuk.
How's about raising a separate, standalone patch which creates a new
kernel-wide coded-in-C function such as
unsigned long jiffies_in_msecs(void);
? (That's assuming we don't already have one. Most likely we have seven
of them hiding in various dark corners).
+static inline unsigned int
+jbd2_time_diff(unsigned int start, unsigned int end)
+{
+ if (unlikely(start end))
+ end = end + (~0UL - start);
+ else
+ end -= start;
+ return end;
+}
I don't know what this does
Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
2007/7/10, Andrew Morton [EMAIL PROTECTED]:
Hi all,
+ size = sizeof(struct transaction_stats_s);
+ s-stats = kmalloc(size, GFP_KERNEL);
+ if (s == NULL) {
+ kfree(s);
+ return -EIO;
ENOMEM
I'm sorry if i missed some point, but i just don't see the use of such
a kfree here, not sure Andrew meant you should only return ENOMEM
instead, but why issuing those kfree(NULL), instead of just a if (!s)
return ENOMEM ?
Regards,
Cédric
-
To unsubscribe from this list: send the line unsubscribe linux-ext4 in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
On Tue, 2007-07-10 at 21:42 -0700, Andrew Morton wrote:
On Tue, 10 Jul 2007 23:21:49 -0400 Cédric Augonnet [EMAIL PROTECTED]
wrote:
2007/7/10, Andrew Morton [EMAIL PROTECTED]:
Hi all,
+ size = sizeof(struct transaction_stats_s);
+ s-stats = kmalloc(size, GFP_KERNEL);
+ if (s == NULL) {
+ kfree(s);
+ return -EIO;
ENOMEM
I'm sorry if i missed some point, but i just don't see the use of such
a kfree here, not sure Andrew meant you should only return ENOMEM
instead, but why issuing those kfree(NULL), instead of just a if (!s)
return ENOMEM ?
You found a bug. It was meant to be
if (s-stats == NULL)
Thanks, I will make sure this get fixed in ext4 patch queue.
-
To unsubscribe from this list: send the line unsubscribe linux-ext4 in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
[PATCH] jbd2 stats through procfs
The patch below updates the jbd stats patch to 2.6.20/jbd2.
The initial patch was posted by Alex Tomas in December 2005
(http://marc.info/?l=linux-ext4m=113538565128617w=2).
It provides statistics via procfs such as transaction lifetime and size.
[ This probably should be rewritten to use debugfs? -- Ted]
Signed-off-by: Johann Lombardi [EMAIL PROTECTED]
--
Index: linux-2.6.22-rc4/include/linux/jbd2.h
===
--- linux-2.6.22-rc4.orig/include/linux/jbd2.h 2007-06-11 17:28:17.0
-0700
+++ linux-2.6.22-rc4/include/linux/jbd2.h 2007-06-13 10:45:21.0
-0700
@@ -408,6 +408,16 @@
};
+/*
+ * Some stats for checkpoint phase
+ */
+struct transaction_chp_stats_s {
+ unsigned long cs_chp_time;
+ unsigned long cs_forced_to_close;
+ unsigned long cs_written;
+ unsigned long cs_dropped;
+};
+
/* The transaction_t type is the guts of the journaling mechanism. It
* tracks a compound transaction through its various states:
*
@@ -543,6 +553,21 @@
spinlock_t t_handle_lock;
/*
+* Longest time some handle had to wait for running transaction
+*/
+ unsigned long t_max_wait;
+
+ /*
+* When transaction started
+*/
+ unsigned long t_start;
+
+ /*
+* Checkpointing stats [j_checkpoint_sem]
+*/
+ struct transaction_chp_stats_s t_chp_stats;
+
+ /*
* Number of outstanding updates running on this transaction
* [t_handle_lock]
*/
@@ -573,6 +598,57 @@
};
+struct transaction_run_stats_s {
+ unsigned long rs_wait;
+ unsigned long rs_running;
+ unsigned long rs_locked;
+ unsigned long rs_flushing;
+ unsigned long rs_logging;
+
+ unsigned long rs_handle_count;
+ unsigned long rs_blocks;
+ unsigned long rs_blocks_logged;
+};
+
+struct transaction_stats_s
+{
+ int ts_type;
+ unsigned long ts_tid;
+ union {
+ struct transaction_run_stats_s run;
+ struct transaction_chp_stats_s chp;
+ } u;
+};
+
+#define JBD2_STATS_RUN 1
+#define JBD2_STATS_CHECKPOINT 2
+
+#define ts_waitu.run.rs_wait
+#define ts_running u.run.rs_running
+#define ts_locked u.run.rs_locked
+#define ts_flushingu.run.rs_flushing
+#define ts_logging u.run.rs_logging
+#define ts_handle_countu.run.rs_handle_count
+#define ts_blocks u.run.rs_blocks
+#define ts_blocks_logged u.run.rs_blocks_logged
+
+#define ts_chp_timeu.chp.cs_chp_time
+#define ts_forced_to_close u.chp.cs_forced_to_close
+#define ts_written u.chp.cs_written
+#define ts_dropped u.chp.cs_dropped
+
+#define CURRENT_MSECS (jiffies_to_msecs(jiffies))
+
+static inline unsigned int
+jbd2_time_diff(unsigned int start, unsigned int end)
+{
+ if (unlikely(start end))
+ end = end + (~0UL - start);
+ else
+ end -= start;
+ return end;
+}
+
/**
* struct journal_s - The journal_s type is the concrete type associated with
* journal_t.
@@ -634,6 +710,12 @@
* @j_wbufsize: maximum number of buffer_heads allowed in j_wbuf, the
* number that will fit in j_blocksize
* @j_last_sync_writer: most recent pid which did a synchronous write
+ * @j_history: Buffer storing the transactions statistics history
+ * @j_history_max: Maximum number of transactions in the statistics history
+ * @j_history_cur: Current number of transactions in the statistics history
+ * @j_history_lock: Protect the transactions statistics history
+ * @j_proc_entry: procfs entry for the jbd statistics directory
+ * @j_stats: Overall statistics
* @j_private: An opaque pointer to fs-private information.
*/
@@ -826,6 +908,16 @@
pid_t j_last_sync_writer;
/*
+* Journal statistics
+*/
+ struct transaction_stats_s *j_history;
+ int j_history_max;
+ int j_history_cur;
+ spinlock_t j_history_lock;
+ struct proc_dir_entry *j_proc_entry;
+ struct transaction_stats_s j_stats;
+
+ /*
* An opaque pointer to fs-private information. ext3 puts its
* superblock pointer here
*/
Index: linux-2.6.22-rc4/fs/jbd2/transaction.c
===
--- linux-2.6.22-rc4.orig/fs/jbd2/transaction.c 2007-06-11 17:22:14.0
-0700
+++ linux-2.6.22-rc4/fs/jbd2/transaction.c 2007-06-13 10:47:56.0
-0700
@@ -59,6 +59,8 @@
J_ASSERT(journal-j_running_transaction
Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
On Sun, 01 Jul 2007 03:38:10 -0400 Mingming Cao [EMAIL PROTECTED] wrote: [PATCH] jbd2 stats through procfs The patch below updates the jbd stats patch to 2.6.20/jbd2. The initial patch was posted by Alex Tomas in December 2005 (http://marc.info/?l=linux-ext4m=113538565128617w=2). It provides statistics via procfs such as transaction lifetime and size. [ This probably should be rewritten to use debugfs? -- Ted] Was a decision ever made on whether this should remain on procfs or be move to use debugfs? I can recall this being discuss but don't recall a firm decision on it. -JRS - To unsubscribe from this list: send the line unsubscribe linux-ext4 in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
