jbd2 currently creates statistics files for each loaded journal in
/proc/fs/jbd2/<bdevname>. ocfs2 loads multiple journals when recovering
other nodes, and the multiple journals on a given bdev collide in the
proc namespace.
This patch changes the location to /proc/fs/jbd2/<bdevname>-<i_ino> as
proposed by Jan Kara. ocfs2 can load multiple journals safely without
collision. There are, to my knowledge, no programmatic users, so the
name change shouldn't (haha) cause any problems.
ocfs2 has 64bit inode numbers, so a collision could theoretically happen
with inodes above 32bits, but ocfs2 journals are generally created at
the front of the volume during mkfs(8). In addition, ocfs2 restricts
inodes to 32bits unless the 'inode64' option is specified.
Signed-off-by: Joel Becker <[email protected]>
---
fs/jbd2/journal.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 8207a01..18d3063 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -901,9 +901,11 @@ static struct proc_dir_entry *proc_jbd2_stats;
static void jbd2_stats_proc_init(journal_t *journal)
{
- char name[BDEVNAME_SIZE];
+ char dname[BDEVNAME_SIZE];
+ char name[NAME_MAX];
- bdevname(journal->j_dev, name);
+ bdevname(journal->j_dev, dname);
+ snprintf(name, NAME_MAX, "%s-%lu", dname, journal->j_inode->i_ino);
journal->j_proc_entry = proc_mkdir(name, proc_jbd2_stats);
if (journal->j_proc_entry) {
proc_create_data("history", S_IRUGO, journal->j_proc_entry,
@@ -915,9 +917,11 @@ static void jbd2_stats_proc_init(journal_t *journal)
static void jbd2_stats_proc_exit(journal_t *journal)
{
- char name[BDEVNAME_SIZE];
+ char dname[BDEVNAME_SIZE];
+ char name[NAME_MAX];
- bdevname(journal->j_dev, name);
+ bdevname(journal->j_dev, dname);
+ snprintf(name, NAME_MAX, "%s-%lu", dname, journal->j_inode->i_ino);
remove_proc_entry("info", journal->j_proc_entry);
remove_proc_entry("history", journal->j_proc_entry);
remove_proc_entry(name, proc_jbd2_stats);
--
1.5.6.3
--
None of our men are "experts." We have most unfortunately found
it necessary to get rid of a man as soon as he thinks himself an
expert -- because no one ever considers himself expert if he really
knows his job. A man who knows a job sees so much more to be done
than he has done, that he is always pressing forward and never
gives up an instant of thought to how good and how efficient he is.
Thinking always ahead, thinking always of trying to do more, brings
a state of mind in which nothing is impossible. The moment one gets
into the "expert" state of mind a great number of things become
impossible.
- From Henry Ford Sr., "My Life and Work"
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Mon, Sep 08, 2008 at 04:31:14PM -0700, Joel Becker wrote:
> jbd2 currently creates statistics files for each loaded journal in
> /proc/fs/jbd2/<bdevname>. ocfs2 loads multiple journals when recovering
> other nodes, and the multiple journals on a given bdev collide in the
> proc namespace.
Did this patch ever get reviewied? We'd really like it in so that Ocfs2 can
use JBD2...
Thanks,
--Mark
--
Mark Fasheh
On Sun, Sep 14, 2008 at 01:40:03AM -0700, Mark Fasheh wrote:
> On Mon, Sep 08, 2008 at 04:31:14PM -0700, Joel Becker wrote:
> > jbd2 currently creates statistics files for each loaded journal in
> > /proc/fs/jbd2/<bdevname>. ocfs2 loads multiple journals when recovering
> > other nodes, and the multiple journals on a given bdev collide in the
> > proc namespace.
>
> Did this patch ever get reviewied? We'd really like it in so that Ocfs2 can
> use JBD2...
Thanks for pinging me; it fell through the cracks. I'll make sure it
gets put into the queue for merging at the next merge window....
- Ted
On Sun, Sep 14, 2008 at 05:03:06AM -0400, Theodore Tso wrote:
> > Did this patch ever get reviewied? We'd really like it in so that Ocfs2 can
> > use JBD2...
>
> Thanks for pinging me; it fell through the cracks. I'll make sure it
> gets put into the queue for merging at the next merge window....
Great, thanks Ted! We'll be pushing Ocfs2/JBD2 support upstream next merge
window too, so this lines things up well for us.
Btw, if anyone is interested in checking out the patches,
Pull:
git://git.kernel.org/pub/scm/linux/kernel/git/mfasheh/ocfs2.git jbd2
or via gitweb:
http://git.kernel.org/?p=linux/kernel/git/mfasheh/ocfs2.git;a=shortlog;h=jbd2
Of course, they'll also hit lkml along with the rest of our Ocfs2 features.
--Mark
--
Mark Fasheh
On Sun, Sep 14, 2008 at 05:03:06AM -0400, Theodore Tso wrote:
> On Sun, Sep 14, 2008 at 01:40:03AM -0700, Mark Fasheh wrote:
> > On Mon, Sep 08, 2008 at 04:31:14PM -0700, Joel Becker wrote:
> > > jbd2 currently creates statistics files for each loaded journal in
> > > /proc/fs/jbd2/<bdevname>. ocfs2 loads multiple journals when recovering
> > > other nodes, and the multiple journals on a given bdev collide in the
> > > proc namespace.
> >
> > Did this patch ever get reviewied? We'd really like it in so that Ocfs2 can
> > use JBD2...
>
> Thanks for pinging me; it fell through the cracks. I'll make sure it
> gets put into the queue for merging at the next merge window....
Could you please do it the proper way as describe in my reply to the
thread?
This is what plan to push instead. It should address ocfs2's
requirement. In addition, it reduces jbd2 and ext4's stack usage,
removes 30 lines of code, and fixes a potential problem with pesky
block devices which contain '/' character in their names. Four for
the price of one!
Could you try this out confirm this fixes the problem for you?
(Currently it only passes the "It builds, ship it!" test, but it's
pretty straightforward; and I'm on an airplane at the moment. :-)
Thanks, regards,
- Ted
>From 172393695ea4daea0061aa7c4f7ee0d56fbda239 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <[email protected]>
Date: Sun, 14 Sep 2008 12:22:35 -0400
Subject: [PATCH] jbd2: clean up how the journal device name is printed
Calculate the journal device name once and stash it away in the
journal_s structure. This avoids needing to call bdevname()
everywhere and reduces stack usage by not needing to allocate an
on-stack buffer. In addition, we eliminate the '/' that can appear in
device names (e.g. "cciss/c0d0p9" --- see kernel bugzilla #11321) that
can cause problems when creating proc directory names, and include the
inode number to support ocfs2 which creates multiple journals with
different inode numbers.
Signed-off-by: "Theodore Ts'o" <[email protected]>
---
fs/ext4/super.c | 12 +++---------
fs/jbd2/commit.c | 11 +++--------
fs/jbd2/journal.c | 48 ++++++++++++++++--------------------------------
include/linux/jbd2.h | 3 ++-
4 files changed, 24 insertions(+), 50 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index f58cc03..64e1c21 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1476,15 +1476,9 @@ static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es,
EXT4_INODES_PER_GROUP(sb),
sbi->s_mount_opt);
- if (EXT4_SB(sb)->s_journal->j_inode == NULL) {
- char b[BDEVNAME_SIZE];
-
- printk(KERN_INFO "EXT4 FS on %s, external journal on %s\n",
- sb->s_id, bdevname(EXT4_SB(sb)->s_journal->j_dev, b));
- } else {
- printk(KERN_INFO "EXT4 FS on %s, internal journal\n",
- sb->s_id);
- }
+ printk(KERN_INFO "EXT4 FS on %s, %s journal on %s\n",
+ sb->s_id, EXT4_SB(sb)->s_journal->j_inode ? "internal" :
+ "external", EXT4_SB(sb)->s_journal->j_devname);
return res;
}
diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index f2ad061..b091e53 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -147,12 +147,9 @@ static int journal_submit_commit_record(journal_t *journal,
* to remember if we sent a barrier request
*/
if (ret == -EOPNOTSUPP && barrier_done) {
- char b[BDEVNAME_SIZE];
-
printk(KERN_WARNING
- "JBD: barrier-based sync failed on %s - "
- "disabling barriers\n",
- bdevname(journal->j_dev, b));
+ "JBD: barrier-based sync failed on %s - "
+ "disabling barriers\n", journal->j_devname);
spin_lock(&journal->j_state_lock);
journal->j_flags &= ~JBD2_BARRIER;
spin_unlock(&journal->j_state_lock);
@@ -681,11 +678,9 @@ start_journal_io:
*/
err = journal_finish_inode_data_buffers(journal, commit_transaction);
if (err) {
- char b[BDEVNAME_SIZE];
-
printk(KERN_WARNING
"JBD2: Detected IO errors while flushing file data "
- "on %s\n", bdevname(journal->j_fs_dev, b));
+ "on %s\n", journal->j_devname);
err = 0;
}
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 8207a01..0c4dcc2 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -597,13 +597,9 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
if (ret)
*retp = ret;
else {
- char b[BDEVNAME_SIZE];
-
printk(KERN_ALERT "%s: journal block not found "
"at offset %lu on %s\n",
- __func__,
- blocknr,
- bdevname(journal->j_dev, b));
+ __func__, blocknr, journal->j_devname);
err = -EIO;
__journal_abort_soft(journal, err);
}
@@ -901,10 +897,7 @@ static struct proc_dir_entry *proc_jbd2_stats;
static void jbd2_stats_proc_init(journal_t *journal)
{
- char name[BDEVNAME_SIZE];
-
- bdevname(journal->j_dev, name);
- journal->j_proc_entry = proc_mkdir(name, proc_jbd2_stats);
+ journal->j_proc_entry = proc_mkdir(journal->j_devname, proc_jbd2_stats);
if (journal->j_proc_entry) {
proc_create_data("history", S_IRUGO, journal->j_proc_entry,
&jbd2_seq_history_fops, journal);
@@ -915,12 +908,9 @@ static void jbd2_stats_proc_init(journal_t *journal)
static void jbd2_stats_proc_exit(journal_t *journal)
{
- char name[BDEVNAME_SIZE];
-
- bdevname(journal->j_dev, name);
remove_proc_entry("info", journal->j_proc_entry);
remove_proc_entry("history", journal->j_proc_entry);
- remove_proc_entry(name, proc_jbd2_stats);
+ remove_proc_entry(journal->j_devname, proc_jbd2_stats);
}
static void journal_init_stats(journal_t *journal)
@@ -1018,6 +1008,7 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
{
journal_t *journal = journal_init_common();
struct buffer_head *bh;
+ char *p;
int n;
if (!journal)
@@ -1039,6 +1030,10 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
journal->j_fs_dev = fs_dev;
journal->j_blk_offset = start;
journal->j_maxlen = len;
+ bdevname(journal->j_dev, journal->j_devname);
+ p = journal->j_devname;
+ while ((p = strchr(p, '/')))
+ *p = '!';
jbd2_stats_proc_init(journal);
bh = __getblk(journal->j_dev, start, journal->j_blocksize);
@@ -1061,6 +1056,7 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
{
struct buffer_head *bh;
journal_t *journal = journal_init_common();
+ char *p;
int err;
int n;
unsigned long long blocknr;
@@ -1070,6 +1066,12 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
journal->j_inode = inode;
+ bdevname(journal->j_dev, journal->j_devname);
+ p = journal->j_devname;
+ while ((p = strchr(p, '/')))
+ *p = '!';
+ p = journal->j_devname + strlen(journal->j_devname);
+ sprintf(p, ":%lu\n", journal->j_inode->i_ino);
jbd_debug(1,
"journal %p: inode %s/%ld, size %Ld, bits %d, blksize %ld\n",
journal, inode->i_sb->s_id, inode->i_ino,
@@ -1761,23 +1763,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
}
/*
- * journal_dev_name: format a character string to describe on what
- * device this journal is present.
- */
-
-static const char *journal_dev_name(journal_t *journal, char *buffer)
-{
- struct block_device *bdev;
-
- if (journal->j_inode)
- bdev = journal->j_inode->i_sb->s_bdev;
- else
- bdev = journal->j_dev;
-
- return bdevname(bdev, buffer);
-}
-
-/*
* Journal abort has very specific semantics, which we describe
* for journal abort.
*
@@ -1793,13 +1778,12 @@ static const char *journal_dev_name(journal_t *journal, char *buffer)
void __jbd2_journal_abort_hard(journal_t *journal)
{
transaction_t *transaction;
- char b[BDEVNAME_SIZE];
if (journal->j_flags & JBD2_ABORT)
return;
printk(KERN_ERR "Aborting journal on device %s.\n",
- journal_dev_name(journal, b));
+ journal->j_devname);
spin_lock(&journal->j_state_lock);
journal->j_flags |= JBD2_ABORT;
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 3dd2090..66c3499 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -850,7 +850,8 @@ struct journal_s
*/
struct block_device *j_dev;
int j_blocksize;
- unsigned long long j_blk_offset;
+ unsigned long long j_blk_offset;
+ char j_devname[BDEVNAME_SIZE+24];
/*
* Device which holds the client fs. For internal journal this will be
--
1.5.6.1.205.ge2c7.dirty
On Sun, Sep 14, 2008 at 12:06:35PM -0400, Christoph Hellwig wrote:
> On Sun, Sep 14, 2008 at 05:03:06AM -0400, Theodore Tso wrote:
> > On Sun, Sep 14, 2008 at 01:40:03AM -0700, Mark Fasheh wrote:
> > > On Mon, Sep 08, 2008 at 04:31:14PM -0700, Joel Becker wrote:
> > > > jbd2 currently creates statistics files for each loaded journal in
> > > > /proc/fs/jbd2/<bdevname>. ocfs2 loads multiple journals when recovering
> > > > other nodes, and the multiple journals on a given bdev collide in the
> > > > proc namespace.
> > >
> > > Did this patch ever get reviewied? We'd really like it in so that Ocfs2 can
> > > use JBD2...
> >
> > Thanks for pinging me; it fell through the cracks. I'll make sure it
> > gets put into the queue for merging at the next merge window....
>
> Could you please do it the proper way as describe in my reply to the
> thread?
Christoph,
Not sure what you mean here. Do you mean switching over
wholesale without the JBD compat config option? We're currently
tracking a spinlock deadlock in JBD2, so I don't know if we're ready for
that.
If you don't get this, I'll try to track you down tomorrow.
Joel
--
"You look in her eyes, the music begins to play.
Hopeless romantics, here we go again."
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Sun, Sep 14, 2008 at 12:31:17PM -0400, Theodore Tso wrote:
> This is what plan to push instead. It should address ocfs2's
> requirement. In addition, it reduces jbd2 and ext4's stack usage,
> removes 30 lines of code, and fixes a potential problem with pesky
> block devices which contain '/' character in their names. Four for
> the price of one!
I like it! I don't have time to test it (KS/LPC), but hopefully
Sunil can.
Joel
--
Life's Little Instruction Book #222
"Think twice before burdening a friend with a secret."
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
On Sun, Sep 14, 2008 at 06:10:42PM -0700, Joel Becker wrote:
> Christoph,
> Not sure what you mean here. Do you mean switching over
> wholesale without the JBD compat config option? We're currently
> tracking a spinlock deadlock in JBD2, so I don't know if we're ready for
> that.
> If you don't get this, I'll try to track you down tomorrow.
Sorry, not related to jbd2 in ocfs2, but the bdevname bits in this
thread. I saw Ted posted another patch later that basically does the
same for the journal dev as the VFS does for sb->s_id so this is okay.
I still think we should do the / substitution in get_sb_bdev, too and
will send a separate patch for it.
Thanks.
So we are testing this atop Joel's jbd2 branch (after reversing the
top patch).
http://oss.oracle.com/git/?p=jlbec/linux-2.6.git;a=shortlog;h=jbd2
Theodore Tso wrote:
> This is what plan to push instead. It should address ocfs2's
> requirement. In addition, it reduces jbd2 and ext4's stack usage,
> removes 30 lines of code, and fixes a potential problem with pesky
> block devices which contain '/' character in their names. Four for
> the price of one!
>
> Could you try this out confirm this fixes the problem for you?
> (Currently it only passes the "It builds, ship it!" test, but it's
> pretty straightforward; and I'm on an airplane at the moment. :-)
>
> Thanks, regards,
>
> - Ted
>
> From 172393695ea4daea0061aa7c4f7ee0d56fbda239 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <[email protected]>
> Date: Sun, 14 Sep 2008 12:22:35 -0400
> Subject: [PATCH] jbd2: clean up how the journal device name is printed
>
> Calculate the journal device name once and stash it away in the
> journal_s structure. This avoids needing to call bdevname()
> everywhere and reduces stack usage by not needing to allocate an
> on-stack buffer. In addition, we eliminate the '/' that can appear in
> device names (e.g. "cciss/c0d0p9" --- see kernel bugzilla #11321) that
> can cause problems when creating proc directory names, and include the
> inode number to support ocfs2 which creates multiple journals with
> different inode numbers.
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> fs/ext4/super.c | 12 +++---------
> fs/jbd2/commit.c | 11 +++--------
> fs/jbd2/journal.c | 48 ++++++++++++++++--------------------------------
> include/linux/jbd2.h | 3 ++-
> 4 files changed, 24 insertions(+), 50 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index f58cc03..64e1c21 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1476,15 +1476,9 @@ static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es,
> EXT4_INODES_PER_GROUP(sb),
> sbi->s_mount_opt);
>
> - if (EXT4_SB(sb)->s_journal->j_inode == NULL) {
> - char b[BDEVNAME_SIZE];
> -
> - printk(KERN_INFO "EXT4 FS on %s, external journal on %s\n",
> - sb->s_id, bdevname(EXT4_SB(sb)->s_journal->j_dev, b));
> - } else {
> - printk(KERN_INFO "EXT4 FS on %s, internal journal\n",
> - sb->s_id);
> - }
> + printk(KERN_INFO "EXT4 FS on %s, %s journal on %s\n",
> + sb->s_id, EXT4_SB(sb)->s_journal->j_inode ? "internal" :
> + "external", EXT4_SB(sb)->s_journal->j_devname);
> return res;
> }
>
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index f2ad061..b091e53 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -147,12 +147,9 @@ static int journal_submit_commit_record(journal_t *journal,
> * to remember if we sent a barrier request
> */
> if (ret == -EOPNOTSUPP && barrier_done) {
> - char b[BDEVNAME_SIZE];
> -
> printk(KERN_WARNING
> - "JBD: barrier-based sync failed on %s - "
> - "disabling barriers\n",
> - bdevname(journal->j_dev, b));
> + "JBD: barrier-based sync failed on %s - "
> + "disabling barriers\n", journal->j_devname);
> spin_lock(&journal->j_state_lock);
> journal->j_flags &= ~JBD2_BARRIER;
> spin_unlock(&journal->j_state_lock);
> @@ -681,11 +678,9 @@ start_journal_io:
> */
> err = journal_finish_inode_data_buffers(journal, commit_transaction);
> if (err) {
> - char b[BDEVNAME_SIZE];
> -
> printk(KERN_WARNING
> "JBD2: Detected IO errors while flushing file data "
> - "on %s\n", bdevname(journal->j_fs_dev, b));
> + "on %s\n", journal->j_devname);
> err = 0;
> }
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 8207a01..0c4dcc2 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -597,13 +597,9 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
> if (ret)
> *retp = ret;
> else {
> - char b[BDEVNAME_SIZE];
> -
> printk(KERN_ALERT "%s: journal block not found "
> "at offset %lu on %s\n",
> - __func__,
> - blocknr,
> - bdevname(journal->j_dev, b));
> + __func__, blocknr, journal->j_devname);
> err = -EIO;
> __journal_abort_soft(journal, err);
> }
> @@ -901,10 +897,7 @@ static struct proc_dir_entry *proc_jbd2_stats;
>
> static void jbd2_stats_proc_init(journal_t *journal)
> {
> - char name[BDEVNAME_SIZE];
> -
> - bdevname(journal->j_dev, name);
> - journal->j_proc_entry = proc_mkdir(name, proc_jbd2_stats);
> + journal->j_proc_entry = proc_mkdir(journal->j_devname, proc_jbd2_stats);
> if (journal->j_proc_entry) {
> proc_create_data("history", S_IRUGO, journal->j_proc_entry,
> &jbd2_seq_history_fops, journal);
> @@ -915,12 +908,9 @@ static void jbd2_stats_proc_init(journal_t *journal)
>
> static void jbd2_stats_proc_exit(journal_t *journal)
> {
> - char name[BDEVNAME_SIZE];
> -
> - bdevname(journal->j_dev, name);
> remove_proc_entry("info", journal->j_proc_entry);
> remove_proc_entry("history", journal->j_proc_entry);
> - remove_proc_entry(name, proc_jbd2_stats);
> + remove_proc_entry(journal->j_devname, proc_jbd2_stats);
> }
>
> static void journal_init_stats(journal_t *journal)
> @@ -1018,6 +1008,7 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
> {
> journal_t *journal = journal_init_common();
> struct buffer_head *bh;
> + char *p;
> int n;
>
> if (!journal)
> @@ -1039,6 +1030,10 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
> journal->j_fs_dev = fs_dev;
> journal->j_blk_offset = start;
> journal->j_maxlen = len;
> + bdevname(journal->j_dev, journal->j_devname);
> + p = journal->j_devname;
> + while ((p = strchr(p, '/')))
> + *p = '!';
> jbd2_stats_proc_init(journal);
>
> bh = __getblk(journal->j_dev, start, journal->j_blocksize);
> @@ -1061,6 +1056,7 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
> {
> struct buffer_head *bh;
> journal_t *journal = journal_init_common();
> + char *p;
> int err;
> int n;
> unsigned long long blocknr;
> @@ -1070,6 +1066,12 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
>
> journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
> journal->j_inode = inode;
> + bdevname(journal->j_dev, journal->j_devname);
> + p = journal->j_devname;
> + while ((p = strchr(p, '/')))
> + *p = '!';
> + p = journal->j_devname + strlen(journal->j_devname);
> + sprintf(p, ":%lu\n", journal->j_inode->i_ino);
> jbd_debug(1,
> "journal %p: inode %s/%ld, size %Ld, bits %d, blksize %ld\n",
> journal, inode->i_sb->s_id, inode->i_ino,
> @@ -1761,23 +1763,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
> }
>
> /*
> - * journal_dev_name: format a character string to describe on what
> - * device this journal is present.
> - */
> -
> -static const char *journal_dev_name(journal_t *journal, char *buffer)
> -{
> - struct block_device *bdev;
> -
> - if (journal->j_inode)
> - bdev = journal->j_inode->i_sb->s_bdev;
> - else
> - bdev = journal->j_dev;
> -
> - return bdevname(bdev, buffer);
> -}
> -
> -/*
> * Journal abort has very specific semantics, which we describe
> * for journal abort.
> *
> @@ -1793,13 +1778,12 @@ static const char *journal_dev_name(journal_t *journal, char *buffer)
> void __jbd2_journal_abort_hard(journal_t *journal)
> {
> transaction_t *transaction;
> - char b[BDEVNAME_SIZE];
>
> if (journal->j_flags & JBD2_ABORT)
> return;
>
> printk(KERN_ERR "Aborting journal on device %s.\n",
> - journal_dev_name(journal, b));
> + journal->j_devname);
>
> spin_lock(&journal->j_state_lock);
> journal->j_flags |= JBD2_ABORT;
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 3dd2090..66c3499 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -850,7 +850,8 @@ struct journal_s
> */
> struct block_device *j_dev;
> int j_blocksize;
> - unsigned long long j_blk_offset;
> + unsigned long long j_blk_offset;
> + char j_devname[BDEVNAME_SIZE+24];
>
> /*
> * Device which holds the client fs. For internal journal this will be
>
The newline looks like a typo here.
@@ -1070,6 +1066,12 @@ journal_t * jbd2_journal_init_inode (struct inode
*inode)
+ sprintf(p, ":%lu\n", journal->j_inode->i_ino);
Other than that, the test ran fine. Marcos successfully ran multiple
recoveries in a 6 node cluster.
Sunil Mushran wrote:
> Thanks.
>
> So we are testing this atop Joel's jbd2 branch (after reversing the
> top patch).
>
> http://oss.oracle.com/git/?p=jlbec/linux-2.6.git;a=shortlog;h=jbd2
>
> Theodore Tso wrote:
>
>> This is what plan to push instead. It should address ocfs2's
>> requirement. In addition, it reduces jbd2 and ext4's stack usage,
>> removes 30 lines of code, and fixes a potential problem with pesky
>> block devices which contain '/' character in their names. Four for
>> the price of one!
>>
>> Could you try this out confirm this fixes the problem for you?
>> (Currently it only passes the "It builds, ship it!" test, but it's
>> pretty straightforward; and I'm on an airplane at the moment. :-)
>>
>> Thanks, regards,
>>
>> - Ted
>>
>> From 172393695ea4daea0061aa7c4f7ee0d56fbda239 Mon Sep 17 00:00:00 2001
>> From: Theodore Ts'o <[email protected]>
>> Date: Sun, 14 Sep 2008 12:22:35 -0400
>> Subject: [PATCH] jbd2: clean up how the journal device name is printed
>>
>> Calculate the journal device name once and stash it away in the
>> journal_s structure. This avoids needing to call bdevname()
>> everywhere and reduces stack usage by not needing to allocate an
>> on-stack buffer. In addition, we eliminate the '/' that can appear in
>> device names (e.g. "cciss/c0d0p9" --- see kernel bugzilla #11321) that
>> can cause problems when creating proc directory names, and include the
>> inode number to support ocfs2 which creates multiple journals with
>> different inode numbers.
>>
>> Signed-off-by: "Theodore Ts'o" <[email protected]>
>> ---
>> fs/ext4/super.c | 12 +++---------
>> fs/jbd2/commit.c | 11 +++--------
>> fs/jbd2/journal.c | 48 ++++++++++++++++--------------------------------
>> include/linux/jbd2.h | 3 ++-
>> 4 files changed, 24 insertions(+), 50 deletions(-)
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index f58cc03..64e1c21 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -1476,15 +1476,9 @@ static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es,
>> EXT4_INODES_PER_GROUP(sb),
>> sbi->s_mount_opt);
>>
>> - if (EXT4_SB(sb)->s_journal->j_inode == NULL) {
>> - char b[BDEVNAME_SIZE];
>> -
>> - printk(KERN_INFO "EXT4 FS on %s, external journal on %s\n",
>> - sb->s_id, bdevname(EXT4_SB(sb)->s_journal->j_dev, b));
>> - } else {
>> - printk(KERN_INFO "EXT4 FS on %s, internal journal\n",
>> - sb->s_id);
>> - }
>> + printk(KERN_INFO "EXT4 FS on %s, %s journal on %s\n",
>> + sb->s_id, EXT4_SB(sb)->s_journal->j_inode ? "internal" :
>> + "external", EXT4_SB(sb)->s_journal->j_devname);
>> return res;
>> }
>>
>> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
>> index f2ad061..b091e53 100644
>> --- a/fs/jbd2/commit.c
>> +++ b/fs/jbd2/commit.c
>> @@ -147,12 +147,9 @@ static int journal_submit_commit_record(journal_t *journal,
>> * to remember if we sent a barrier request
>> */
>> if (ret == -EOPNOTSUPP && barrier_done) {
>> - char b[BDEVNAME_SIZE];
>> -
>> printk(KERN_WARNING
>> - "JBD: barrier-based sync failed on %s - "
>> - "disabling barriers\n",
>> - bdevname(journal->j_dev, b));
>> + "JBD: barrier-based sync failed on %s - "
>> + "disabling barriers\n", journal->j_devname);
>> spin_lock(&journal->j_state_lock);
>> journal->j_flags &= ~JBD2_BARRIER;
>> spin_unlock(&journal->j_state_lock);
>> @@ -681,11 +678,9 @@ start_journal_io:
>> */
>> err = journal_finish_inode_data_buffers(journal, commit_transaction);
>> if (err) {
>> - char b[BDEVNAME_SIZE];
>> -
>> printk(KERN_WARNING
>> "JBD2: Detected IO errors while flushing file data "
>> - "on %s\n", bdevname(journal->j_fs_dev, b));
>> + "on %s\n", journal->j_devname);
>> err = 0;
>> }
>>
>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
>> index 8207a01..0c4dcc2 100644
>> --- a/fs/jbd2/journal.c
>> +++ b/fs/jbd2/journal.c
>> @@ -597,13 +597,9 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
>> if (ret)
>> *retp = ret;
>> else {
>> - char b[BDEVNAME_SIZE];
>> -
>> printk(KERN_ALERT "%s: journal block not found "
>> "at offset %lu on %s\n",
>> - __func__,
>> - blocknr,
>> - bdevname(journal->j_dev, b));
>> + __func__, blocknr, journal->j_devname);
>> err = -EIO;
>> __journal_abort_soft(journal, err);
>> }
>> @@ -901,10 +897,7 @@ static struct proc_dir_entry *proc_jbd2_stats;
>>
>> static void jbd2_stats_proc_init(journal_t *journal)
>> {
>> - char name[BDEVNAME_SIZE];
>> -
>> - bdevname(journal->j_dev, name);
>> - journal->j_proc_entry = proc_mkdir(name, proc_jbd2_stats);
>> + journal->j_proc_entry = proc_mkdir(journal->j_devname, proc_jbd2_stats);
>> if (journal->j_proc_entry) {
>> proc_create_data("history", S_IRUGO, journal->j_proc_entry,
>> &jbd2_seq_history_fops, journal);
>> @@ -915,12 +908,9 @@ static void jbd2_stats_proc_init(journal_t *journal)
>>
>> static void jbd2_stats_proc_exit(journal_t *journal)
>> {
>> - char name[BDEVNAME_SIZE];
>> -
>> - bdevname(journal->j_dev, name);
>> remove_proc_entry("info", journal->j_proc_entry);
>> remove_proc_entry("history", journal->j_proc_entry);
>> - remove_proc_entry(name, proc_jbd2_stats);
>> + remove_proc_entry(journal->j_devname, proc_jbd2_stats);
>> }
>>
>> static void journal_init_stats(journal_t *journal)
>> @@ -1018,6 +1008,7 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
>> {
>> journal_t *journal = journal_init_common();
>> struct buffer_head *bh;
>> + char *p;
>> int n;
>>
>> if (!journal)
>> @@ -1039,6 +1030,10 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
>> journal->j_fs_dev = fs_dev;
>> journal->j_blk_offset = start;
>> journal->j_maxlen = len;
>> + bdevname(journal->j_dev, journal->j_devname);
>> + p = journal->j_devname;
>> + while ((p = strchr(p, '/')))
>> + *p = '!';
>> jbd2_stats_proc_init(journal);
>>
>> bh = __getblk(journal->j_dev, start, journal->j_blocksize);
>> @@ -1061,6 +1056,7 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
>> {
>> struct buffer_head *bh;
>> journal_t *journal = journal_init_common();
>> + char *p;
>> int err;
>> int n;
>> unsigned long long blocknr;
>> @@ -1070,6 +1066,12 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
>>
>> journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
>> journal->j_inode = inode;
>> + bdevname(journal->j_dev, journal->j_devname);
>> + p = journal->j_devname;
>> + while ((p = strchr(p, '/')))
>> + *p = '!';
>> + p = journal->j_devname + strlen(journal->j_devname);
>> + sprintf(p, ":%lu\n", journal->j_inode->i_ino);
>> jbd_debug(1,
>> "journal %p: inode %s/%ld, size %Ld, bits %d, blksize %ld\n",
>> journal, inode->i_sb->s_id, inode->i_ino,
>> @@ -1761,23 +1763,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
>> }
>>
>> /*
>> - * journal_dev_name: format a character string to describe on what
>> - * device this journal is present.
>> - */
>> -
>> -static const char *journal_dev_name(journal_t *journal, char *buffer)
>> -{
>> - struct block_device *bdev;
>> -
>> - if (journal->j_inode)
>> - bdev = journal->j_inode->i_sb->s_bdev;
>> - else
>> - bdev = journal->j_dev;
>> -
>> - return bdevname(bdev, buffer);
>> -}
>> -
>> -/*
>> * Journal abort has very specific semantics, which we describe
>> * for journal abort.
>> *
>> @@ -1793,13 +1778,12 @@ static const char *journal_dev_name(journal_t *journal, char *buffer)
>> void __jbd2_journal_abort_hard(journal_t *journal)
>> {
>> transaction_t *transaction;
>> - char b[BDEVNAME_SIZE];
>>
>> if (journal->j_flags & JBD2_ABORT)
>> return;
>>
>> printk(KERN_ERR "Aborting journal on device %s.\n",
>> - journal_dev_name(journal, b));
>> + journal->j_devname);
>>
>> spin_lock(&journal->j_state_lock);
>> journal->j_flags |= JBD2_ABORT;
>> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
>> index 3dd2090..66c3499 100644
>> --- a/include/linux/jbd2.h
>> +++ b/include/linux/jbd2.h
>> @@ -850,7 +850,8 @@ struct journal_s
>> */
>> struct block_device *j_dev;
>> int j_blocksize;
>> - unsigned long long j_blk_offset;
>> + unsigned long long j_blk_offset;
>> + char j_devname[BDEVNAME_SIZE+24];
>>
>> /*
>> * Device which holds the client fs. For internal journal this will be
>>
>>
>
>
> _______________________________________________
> Ocfs2-devel mailing list
> [email protected]
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
>
> This is what plan to push instead. It should address ocfs2's
> requirement. In addition, it reduces jbd2 and ext4's stack usage,
> removes 30 lines of code, and fixes a potential problem with pesky
> block devices which contain '/' character in their names. Four for
> the price of one!
>
> Could you try this out confirm this fixes the problem for you?
> (Currently it only passes the "It builds, ship it!" test, but it's
> pretty straightforward; and I'm on an airplane at the moment. :-)
The patch looks fine. You can add:
Acked-by: Jan Kara <[email protected]>
Honza
> >From 172393695ea4daea0061aa7c4f7ee0d56fbda239 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <[email protected]>
> Date: Sun, 14 Sep 2008 12:22:35 -0400
> Subject: [PATCH] jbd2: clean up how the journal device name is printed
>
> Calculate the journal device name once and stash it away in the
> journal_s structure. This avoids needing to call bdevname()
> everywhere and reduces stack usage by not needing to allocate an
> on-stack buffer. In addition, we eliminate the '/' that can appear in
> device names (e.g. "cciss/c0d0p9" --- see kernel bugzilla #11321) that
> can cause problems when creating proc directory names, and include the
> inode number to support ocfs2 which creates multiple journals with
> different inode numbers.
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> ---
> fs/ext4/super.c | 12 +++---------
> fs/jbd2/commit.c | 11 +++--------
> fs/jbd2/journal.c | 48 ++++++++++++++++--------------------------------
> include/linux/jbd2.h | 3 ++-
> 4 files changed, 24 insertions(+), 50 deletions(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index f58cc03..64e1c21 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1476,15 +1476,9 @@ static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es,
> EXT4_INODES_PER_GROUP(sb),
> sbi->s_mount_opt);
>
> - if (EXT4_SB(sb)->s_journal->j_inode == NULL) {
> - char b[BDEVNAME_SIZE];
> -
> - printk(KERN_INFO "EXT4 FS on %s, external journal on %s\n",
> - sb->s_id, bdevname(EXT4_SB(sb)->s_journal->j_dev, b));
> - } else {
> - printk(KERN_INFO "EXT4 FS on %s, internal journal\n",
> - sb->s_id);
> - }
> + printk(KERN_INFO "EXT4 FS on %s, %s journal on %s\n",
> + sb->s_id, EXT4_SB(sb)->s_journal->j_inode ? "internal" :
> + "external", EXT4_SB(sb)->s_journal->j_devname);
> return res;
> }
>
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index f2ad061..b091e53 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -147,12 +147,9 @@ static int journal_submit_commit_record(journal_t *journal,
> * to remember if we sent a barrier request
> */
> if (ret == -EOPNOTSUPP && barrier_done) {
> - char b[BDEVNAME_SIZE];
> -
> printk(KERN_WARNING
> - "JBD: barrier-based sync failed on %s - "
> - "disabling barriers\n",
> - bdevname(journal->j_dev, b));
> + "JBD: barrier-based sync failed on %s - "
> + "disabling barriers\n", journal->j_devname);
> spin_lock(&journal->j_state_lock);
> journal->j_flags &= ~JBD2_BARRIER;
> spin_unlock(&journal->j_state_lock);
> @@ -681,11 +678,9 @@ start_journal_io:
> */
> err = journal_finish_inode_data_buffers(journal, commit_transaction);
> if (err) {
> - char b[BDEVNAME_SIZE];
> -
> printk(KERN_WARNING
> "JBD2: Detected IO errors while flushing file data "
> - "on %s\n", bdevname(journal->j_fs_dev, b));
> + "on %s\n", journal->j_devname);
> err = 0;
> }
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 8207a01..0c4dcc2 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -597,13 +597,9 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
> if (ret)
> *retp = ret;
> else {
> - char b[BDEVNAME_SIZE];
> -
> printk(KERN_ALERT "%s: journal block not found "
> "at offset %lu on %s\n",
> - __func__,
> - blocknr,
> - bdevname(journal->j_dev, b));
> + __func__, blocknr, journal->j_devname);
> err = -EIO;
> __journal_abort_soft(journal, err);
> }
> @@ -901,10 +897,7 @@ static struct proc_dir_entry *proc_jbd2_stats;
>
> static void jbd2_stats_proc_init(journal_t *journal)
> {
> - char name[BDEVNAME_SIZE];
> -
> - bdevname(journal->j_dev, name);
> - journal->j_proc_entry = proc_mkdir(name, proc_jbd2_stats);
> + journal->j_proc_entry = proc_mkdir(journal->j_devname, proc_jbd2_stats);
> if (journal->j_proc_entry) {
> proc_create_data("history", S_IRUGO, journal->j_proc_entry,
> &jbd2_seq_history_fops, journal);
> @@ -915,12 +908,9 @@ static void jbd2_stats_proc_init(journal_t *journal)
>
> static void jbd2_stats_proc_exit(journal_t *journal)
> {
> - char name[BDEVNAME_SIZE];
> -
> - bdevname(journal->j_dev, name);
> remove_proc_entry("info", journal->j_proc_entry);
> remove_proc_entry("history", journal->j_proc_entry);
> - remove_proc_entry(name, proc_jbd2_stats);
> + remove_proc_entry(journal->j_devname, proc_jbd2_stats);
> }
>
> static void journal_init_stats(journal_t *journal)
> @@ -1018,6 +1008,7 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
> {
> journal_t *journal = journal_init_common();
> struct buffer_head *bh;
> + char *p;
> int n;
>
> if (!journal)
> @@ -1039,6 +1030,10 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
> journal->j_fs_dev = fs_dev;
> journal->j_blk_offset = start;
> journal->j_maxlen = len;
> + bdevname(journal->j_dev, journal->j_devname);
> + p = journal->j_devname;
> + while ((p = strchr(p, '/')))
> + *p = '!';
> jbd2_stats_proc_init(journal);
>
> bh = __getblk(journal->j_dev, start, journal->j_blocksize);
> @@ -1061,6 +1056,7 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
> {
> struct buffer_head *bh;
> journal_t *journal = journal_init_common();
> + char *p;
> int err;
> int n;
> unsigned long long blocknr;
> @@ -1070,6 +1066,12 @@ journal_t * jbd2_journal_init_inode (struct inode *inode)
>
> journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
> journal->j_inode = inode;
> + bdevname(journal->j_dev, journal->j_devname);
> + p = journal->j_devname;
> + while ((p = strchr(p, '/')))
> + *p = '!';
> + p = journal->j_devname + strlen(journal->j_devname);
> + sprintf(p, ":%lu\n", journal->j_inode->i_ino);
> jbd_debug(1,
> "journal %p: inode %s/%ld, size %Ld, bits %d, blksize %ld\n",
> journal, inode->i_sb->s_id, inode->i_ino,
> @@ -1761,23 +1763,6 @@ int jbd2_journal_wipe(journal_t *journal, int write)
> }
>
> /*
> - * journal_dev_name: format a character string to describe on what
> - * device this journal is present.
> - */
> -
> -static const char *journal_dev_name(journal_t *journal, char *buffer)
> -{
> - struct block_device *bdev;
> -
> - if (journal->j_inode)
> - bdev = journal->j_inode->i_sb->s_bdev;
> - else
> - bdev = journal->j_dev;
> -
> - return bdevname(bdev, buffer);
> -}
> -
> -/*
> * Journal abort has very specific semantics, which we describe
> * for journal abort.
> *
> @@ -1793,13 +1778,12 @@ static const char *journal_dev_name(journal_t *journal, char *buffer)
> void __jbd2_journal_abort_hard(journal_t *journal)
> {
> transaction_t *transaction;
> - char b[BDEVNAME_SIZE];
>
> if (journal->j_flags & JBD2_ABORT)
> return;
>
> printk(KERN_ERR "Aborting journal on device %s.\n",
> - journal_dev_name(journal, b));
> + journal->j_devname);
>
> spin_lock(&journal->j_state_lock);
> journal->j_flags |= JBD2_ABORT;
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 3dd2090..66c3499 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -850,7 +850,8 @@ struct journal_s
> */
> struct block_device *j_dev;
> int j_blocksize;
> - unsigned long long j_blk_offset;
> + unsigned long long j_blk_offset;
> + char j_devname[BDEVNAME_SIZE+24];
>
> /*
> * Device which holds the client fs. For internal journal this will be
> --
> 1.5.6.1.205.ge2c7.dirty
>
> --
> 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
--
Jan Kara <[email protected]>
SuSE CR Labs