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-ext4&m=113538565128617&w=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-ext4&m=113538565128617&w=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 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 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-ext4&m=113538565128617&w=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 w
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
Re: [EXT4 set 6][PATCH 1/1]Export jbd stats through procfs
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) - 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, Jul 10, 2007 at 11:21:49PM -0400, Cédric Augonnet 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 That, and if (s->stats == NULL) kfree(s); > 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 ? kfree() is correct, check isn't. - 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
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 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-ext4&m=113538565128617&w=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 + (~
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-ext4&m=113538565128617&w=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