linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] jbd2: add jbd2_journal_init() helper
@ 2016-08-31 12:23 Geliang Tang
  2016-09-03  9:33 ` Eric Ren
  0 siblings, 1 reply; 9+ messages in thread
From: Geliang Tang @ 2016-08-31 12:23 UTC (permalink / raw)
  To: Theodore Ts'o, Jan Kara; +Cc: Geliang Tang, linux-ext4, linux-kernel

There are some repetitive code in jbd2_journal_init_dev() and
jbd2_journal_init_inode(). So this patch extracts the common code into
jbd2_journal_init() helper to simplify the code. And fix the coding
style warnings reported by checkpatch.pl by the way.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 fs/jbd2/journal.c | 130 +++++++++++++++++++++++++++---------------------------
 1 file changed, 66 insertions(+), 64 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 46261a6..c10f657 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1143,30 +1143,13 @@ static journal_t * journal_init_common (void)
  *
  */
 
-/**
- *  journal_t * jbd2_journal_init_dev() - creates and initialises a journal structure
- *  @bdev: Block device on which to create the journal
- *  @fs_dev: Device which hold journalled filesystem for this journal.
- *  @start: Block nr Start of journal.
- *  @len:  Length of the journal in blocks.
- *  @blocksize: blocksize of journalling device
- *
- *  Returns: a newly created journal_t *
- *
- *  jbd2_journal_init_dev creates a journal which maps a fixed contiguous
- *  range of blocks on an arbitrary block device.
- *
- */
-journal_t * jbd2_journal_init_dev(struct block_device *bdev,
-			struct block_device *fs_dev,
-			unsigned long long start, int len, int blocksize)
+static int jbd2_journal_init(journal_t *journal, struct block_device *bdev,
+			struct block_device *fs_dev, unsigned long long start,
+			int len, int blocksize, gfp_t gfp)
 {
-	journal_t *journal = journal_init_common();
 	struct buffer_head *bh;
 	int n;
-
-	if (!journal)
-		return NULL;
+	int err = 0;
 
 	/* journal descriptor can store up to n blocks -bzzz */
 	journal->j_blocksize = blocksize;
@@ -1183,94 +1166,113 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
 	if (!journal->j_wbuf) {
 		printk(KERN_ERR "%s: Can't allocate bhs for commit thread\n",
 			__func__);
+		err = -ENOMEM;
 		goto out_err;
 	}
 
-	bh = __getblk(journal->j_dev, start, journal->j_blocksize);
+	bh = __getblk_gfp(journal->j_dev, start, journal->j_blocksize, gfp);
 	if (!bh) {
 		printk(KERN_ERR
 		       "%s: Cannot get buffer for journal superblock\n",
 		       __func__);
+		err = -EIO;
 		goto out_err;
 	}
 	journal->j_sb_buffer = bh;
 	journal->j_superblock = (journal_superblock_t *)bh->b_data;
 
-	return journal;
+	return 0;
 out_err:
 	kfree(journal->j_wbuf);
 	jbd2_stats_proc_exit(journal);
+	return err;
+}
+
+/**
+ * journal_t *jbd2_journal_init_dev() - creates and initialises a journal
+ * structure.
+ * @bdev: Block device on which to create the journal
+ * @fs_dev: Device which hold journalled filesystem for this journal.
+ * @start: Block nr Start of journal.
+ * @len:  Length of the journal in blocks.
+ * @blocksize: blocksize of journalling device
+ *
+ * Returns: a newly created journal_t *
+ *
+ * jbd2_journal_init_dev creates a journal which maps a fixed contiguous
+ * range of blocks on an arbitrary block device.
+ */
+journal_t *jbd2_journal_init_dev(struct block_device *bdev,
+			struct block_device *fs_dev,
+			unsigned long long start, int len, int blocksize)
+{
+	journal_t *journal = journal_init_common();
+	int err;
+
+	if (!journal)
+		return NULL;
+
+	err = jbd2_journal_init(journal, bdev, fs_dev, start, len,
+				blocksize, __GFP_MOVABLE);
+	if (err) {
+		pr_err("%s: journal init error\n",
+		       __func__);
+		goto out_err;
+	}
+
+	return journal;
+
+out_err:
 	kfree(journal);
 	return NULL;
 }
 
 /**
- *  journal_t * jbd2_journal_init_inode () - creates a journal which maps to a inode.
- *  @inode: An inode to create the journal in
+ * journal_t *jbd2_journal_init_inode() - creates a journal which maps to a
+ * inode.
+ * @inode: An inode to create the journal in
+ *
+ * Returns: a newly created journal_t *
  *
  * jbd2_journal_init_inode creates a journal which maps an on-disk inode as
- * the journal.  The inode must exist already, must support bmap() and
- * must have all data blocks preallocated.
+ * the journal.  The inode must exist already, must support bmap() and must
+ * have all data blocks preallocated.
  */
-journal_t * jbd2_journal_init_inode (struct inode *inode)
+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;
+	int err;
 
 	if (!journal)
 		return NULL;
 
-	journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
 	journal->j_inode = inode;
-	bdevname(journal->j_dev, journal->j_devname);
-	p = strreplace(journal->j_devname, '/', '!');
-	sprintf(p, "-%lu", 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,
-		  (long long) inode->i_size,
-		  inode->i_sb->s_blocksize_bits, inode->i_sb->s_blocksize);
-
-	journal->j_maxlen = inode->i_size >> inode->i_sb->s_blocksize_bits;
-	journal->j_blocksize = inode->i_sb->s_blocksize;
-	jbd2_stats_proc_init(journal);
-
-	/* journal descriptor can store up to n blocks -bzzz */
-	n = journal->j_blocksize / sizeof(journal_block_tag_t);
-	journal->j_wbufsize = n;
-	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_KERNEL);
-	if (!journal->j_wbuf) {
-		printk(KERN_ERR "%s: Can't allocate bhs for commit thread\n",
-			__func__);
-		goto out_err;
-	}
 
 	err = jbd2_journal_bmap(journal, 0, &blocknr);
 	/* If that failed, give up */
 	if (err) {
-		printk(KERN_ERR "%s: Cannot locate journal superblock\n",
+		pr_err("%s: Cannot locate journal superblock\n",
 		       __func__);
 		goto out_err;
 	}
 
-	bh = getblk_unmovable(journal->j_dev, blocknr, journal->j_blocksize);
-	if (!bh) {
-		printk(KERN_ERR
-		       "%s: Cannot get buffer for journal superblock\n",
+	err = jbd2_journal_init(journal, inode->i_sb->s_bdev,
+				inode->i_sb->s_bdev, blocknr,
+				inode->i_size >> inode->i_sb->s_blocksize_bits,
+				inode->i_sb->s_blocksize, 0);
+	if (err) {
+		pr_err("%s: journal init error\n",
 		       __func__);
 		goto out_err;
 	}
-	journal->j_sb_buffer = bh;
-	journal->j_superblock = (journal_superblock_t *)bh->b_data;
+
+	sprintf(journal->j_devname, "%s-%lu",
+		journal->j_devname, journal->j_inode->i_ino);
 
 	return journal;
+
 out_err:
-	kfree(journal->j_wbuf);
-	jbd2_stats_proc_exit(journal);
 	kfree(journal);
 	return NULL;
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] jbd2: add jbd2_journal_init() helper
  2016-08-31 12:23 [PATCH] jbd2: add jbd2_journal_init() helper Geliang Tang
@ 2016-09-03  9:33 ` Eric Ren
  2016-09-06 15:15   ` Jan Kara
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Ren @ 2016-09-03  9:33 UTC (permalink / raw)
  To: Geliang Tang, Theodore Ts'o, Jan Kara; +Cc: linux-ext4, linux-kernel

Hi Geliang,

On 08/31/2016 08:23 PM, Geliang Tang wrote:
> There are some repetitive code in jbd2_journal_init_dev() and
> jbd2_journal_init_inode(). So this patch extracts the common code into
> jbd2_journal_init() helper to simplify the code. And fix the coding
> style warnings reported by checkpatch.pl by the way.
journal_init_common() is already there for the same purpose, right?
Do we really need another helper?

Thanks,
Eric

>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
>   fs/jbd2/journal.c | 130 +++++++++++++++++++++++++++---------------------------
>   1 file changed, 66 insertions(+), 64 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 46261a6..c10f657 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1143,30 +1143,13 @@ static journal_t * journal_init_common (void)
>    *
>    */
>   
> -/**
> - *  journal_t * jbd2_journal_init_dev() - creates and initialises a journal structure
> - *  @bdev: Block device on which to create the journal
> - *  @fs_dev: Device which hold journalled filesystem for this journal.
> - *  @start: Block nr Start of journal.
> - *  @len:  Length of the journal in blocks.
> - *  @blocksize: blocksize of journalling device
> - *
> - *  Returns: a newly created journal_t *
> - *
> - *  jbd2_journal_init_dev creates a journal which maps a fixed contiguous
> - *  range of blocks on an arbitrary block device.
> - *
> - */
> -journal_t * jbd2_journal_init_dev(struct block_device *bdev,
> -			struct block_device *fs_dev,
> -			unsigned long long start, int len, int blocksize)
> +static int jbd2_journal_init(journal_t *journal, struct block_device *bdev,
> +			struct block_device *fs_dev, unsigned long long start,
> +			int len, int blocksize, gfp_t gfp)
>   {
> -	journal_t *journal = journal_init_common();
>   	struct buffer_head *bh;
>   	int n;
> -
> -	if (!journal)
> -		return NULL;
> +	int err = 0;
>   
>   	/* journal descriptor can store up to n blocks -bzzz */
>   	journal->j_blocksize = blocksize;
> @@ -1183,94 +1166,113 @@ journal_t * jbd2_journal_init_dev(struct block_device *bdev,
>   	if (!journal->j_wbuf) {
>   		printk(KERN_ERR "%s: Can't allocate bhs for commit thread\n",
>   			__func__);
> +		err = -ENOMEM;
>   		goto out_err;
>   	}
>   
> -	bh = __getblk(journal->j_dev, start, journal->j_blocksize);
> +	bh = __getblk_gfp(journal->j_dev, start, journal->j_blocksize, gfp);
>   	if (!bh) {
>   		printk(KERN_ERR
>   		       "%s: Cannot get buffer for journal superblock\n",
>   		       __func__);
> +		err = -EIO;
>   		goto out_err;
>   	}
>   	journal->j_sb_buffer = bh;
>   	journal->j_superblock = (journal_superblock_t *)bh->b_data;
>   
> -	return journal;
> +	return 0;
>   out_err:
>   	kfree(journal->j_wbuf);
>   	jbd2_stats_proc_exit(journal);
> +	return err;
> +}
> +
> +/**
> + * journal_t *jbd2_journal_init_dev() - creates and initialises a journal
> + * structure.
> + * @bdev: Block device on which to create the journal
> + * @fs_dev: Device which hold journalled filesystem for this journal.
> + * @start: Block nr Start of journal.
> + * @len:  Length of the journal in blocks.
> + * @blocksize: blocksize of journalling device
> + *
> + * Returns: a newly created journal_t *
> + *
> + * jbd2_journal_init_dev creates a journal which maps a fixed contiguous
> + * range of blocks on an arbitrary block device.
> + */
> +journal_t *jbd2_journal_init_dev(struct block_device *bdev,
> +			struct block_device *fs_dev,
> +			unsigned long long start, int len, int blocksize)
> +{
> +	journal_t *journal = journal_init_common();
> +	int err;
> +
> +	if (!journal)
> +		return NULL;
> +
> +	err = jbd2_journal_init(journal, bdev, fs_dev, start, len,
> +				blocksize, __GFP_MOVABLE);
> +	if (err) {
> +		pr_err("%s: journal init error\n",
> +		       __func__);
> +		goto out_err;
> +	}
> +
> +	return journal;
> +
> +out_err:
>   	kfree(journal);
>   	return NULL;
>   }
>   
>   /**
> - *  journal_t * jbd2_journal_init_inode () - creates a journal which maps to a inode.
> - *  @inode: An inode to create the journal in
> + * journal_t *jbd2_journal_init_inode() - creates a journal which maps to a
> + * inode.
> + * @inode: An inode to create the journal in
> + *
> + * Returns: a newly created journal_t *
>    *
>    * jbd2_journal_init_inode creates a journal which maps an on-disk inode as
> - * the journal.  The inode must exist already, must support bmap() and
> - * must have all data blocks preallocated.
> + * the journal.  The inode must exist already, must support bmap() and must
> + * have all data blocks preallocated.
>    */
> -journal_t * jbd2_journal_init_inode (struct inode *inode)
> +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;
> +	int err;
>   
>   	if (!journal)
>   		return NULL;
>   
> -	journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
>   	journal->j_inode = inode;
> -	bdevname(journal->j_dev, journal->j_devname);
> -	p = strreplace(journal->j_devname, '/', '!');
> -	sprintf(p, "-%lu", 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,
> -		  (long long) inode->i_size,
> -		  inode->i_sb->s_blocksize_bits, inode->i_sb->s_blocksize);
> -
> -	journal->j_maxlen = inode->i_size >> inode->i_sb->s_blocksize_bits;
> -	journal->j_blocksize = inode->i_sb->s_blocksize;
> -	jbd2_stats_proc_init(journal);
> -
> -	/* journal descriptor can store up to n blocks -bzzz */
> -	n = journal->j_blocksize / sizeof(journal_block_tag_t);
> -	journal->j_wbufsize = n;
> -	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_KERNEL);
> -	if (!journal->j_wbuf) {
> -		printk(KERN_ERR "%s: Can't allocate bhs for commit thread\n",
> -			__func__);
> -		goto out_err;
> -	}
>   
>   	err = jbd2_journal_bmap(journal, 0, &blocknr);
>   	/* If that failed, give up */
>   	if (err) {
> -		printk(KERN_ERR "%s: Cannot locate journal superblock\n",
> +		pr_err("%s: Cannot locate journal superblock\n",
>   		       __func__);
>   		goto out_err;
>   	}
>   
> -	bh = getblk_unmovable(journal->j_dev, blocknr, journal->j_blocksize);
> -	if (!bh) {
> -		printk(KERN_ERR
> -		       "%s: Cannot get buffer for journal superblock\n",
> +	err = jbd2_journal_init(journal, inode->i_sb->s_bdev,
> +				inode->i_sb->s_bdev, blocknr,
> +				inode->i_size >> inode->i_sb->s_blocksize_bits,
> +				inode->i_sb->s_blocksize, 0);
> +	if (err) {
> +		pr_err("%s: journal init error\n",
>   		       __func__);
>   		goto out_err;
>   	}
> -	journal->j_sb_buffer = bh;
> -	journal->j_superblock = (journal_superblock_t *)bh->b_data;
> +
> +	sprintf(journal->j_devname, "%s-%lu",
> +		journal->j_devname, journal->j_inode->i_ino);
>   
>   	return journal;
> +
>   out_err:
> -	kfree(journal->j_wbuf);
> -	jbd2_stats_proc_exit(journal);
>   	kfree(journal);
>   	return NULL;
>   }

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] jbd2: add jbd2_journal_init() helper
  2016-09-03  9:33 ` Eric Ren
@ 2016-09-06 15:15   ` Jan Kara
  2016-09-07 12:41     ` [PATCH] " Geliang Tang
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Kara @ 2016-09-06 15:15 UTC (permalink / raw)
  To: Eric Ren
  Cc: Geliang Tang, Theodore Ts'o, Jan Kara, linux-ext4, linux-kernel

On Sat 03-09-16 17:33:39, Eric Ren wrote:
> Hi Geliang,
> 
> On 08/31/2016 08:23 PM, Geliang Tang wrote:
> >There are some repetitive code in jbd2_journal_init_dev() and
> >jbd2_journal_init_inode(). So this patch extracts the common code into
> >jbd2_journal_init() helper to simplify the code. And fix the coding
> >style warnings reported by checkpatch.pl by the way.
> journal_init_common() is already there for the same purpose, right?
> Do we really need another helper?

Agreed, just move the common code into journal_init_common(). Also the
__getblk() in jbd2_journal_init_dev() should be getblk_unmovable() so that
would be a good preparatory fix to remove the need of the gfp argument.


								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] Re: [PATCH] jbd2: add jbd2_journal_init() helper
  2016-09-06 15:15   ` Jan Kara
@ 2016-09-07 12:41     ` Geliang Tang
  2016-09-07 12:41       ` [PATCH] jbd2: move more common code into journal_init_common() Geliang Tang
  0 siblings, 1 reply; 9+ messages in thread
From: Geliang Tang @ 2016-09-07 12:41 UTC (permalink / raw)
  To: Theodore Ts'o, Jan Kara, Eric Ren
  Cc: Geliang Tang, linux-ext4, linux-kernel

On Tue, Sep 06, 2016 at 05:15:54PM +0200, Jan Kara wrote:
> On Sat 03-09-16 17:33:39, Eric Ren wrote:
> > Hi Geliang,
> > 
> > On 08/31/2016 08:23 PM, Geliang Tang wrote:
> > >There are some repetitive code in jbd2_journal_init_dev() and
> > >jbd2_journal_init_inode(). So this patch extracts the common code into
> > >jbd2_journal_init() helper to simplify the code. And fix the coding
> > >style warnings reported by checkpatch.pl by the way.
> > journal_init_common() is already there for the same purpose, right?
> > Do we really need another helper?
> 
> Agreed, just move the common code into journal_init_common(). Also the
> __getblk() in jbd2_journal_init_dev() should be getblk_unmovable() so that
> would be a good preparatory fix to remove the need of the gfp argument.
> 

Thanks for your review. I updated the patch as you suggested.

-Geliang

Geliang Tang (1):
  jbd2: move more common code into journal_init_common()

 fs/jbd2/journal.c | 136 +++++++++++++++++++++---------------------------------
 1 file changed, 53 insertions(+), 83 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] jbd2: move more common code into journal_init_common()
  2016-09-07 12:41     ` [PATCH] " Geliang Tang
@ 2016-09-07 12:41       ` Geliang Tang
  2016-09-07 13:16         ` Jan Kara
  0 siblings, 1 reply; 9+ messages in thread
From: Geliang Tang @ 2016-09-07 12:41 UTC (permalink / raw)
  To: Theodore Ts'o, Jan Kara, Eric Ren
  Cc: Geliang Tang, linux-ext4, linux-kernel

There are some repetitive code in jbd2_journal_init_dev() and
jbd2_journal_init_inode(). So this patch moves the common code into
journal_init_common() helper to simplify the code. And fix the coding
style warnings reported by checkpatch.pl by the way.

Signed-off-by: Geliang Tang <geliangtang@gmail.com>
---
 fs/jbd2/journal.c | 136 +++++++++++++++++++++---------------------------------
 1 file changed, 53 insertions(+), 83 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 46261a6..07e14ef 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1090,11 +1090,15 @@ static void jbd2_stats_proc_exit(journal_t *journal)
  * very few fields yet: that has to wait until we have created the
  * journal structures from from scratch, or loaded them from disk. */
 
-static journal_t * journal_init_common (void)
+static journal_t *journal_init_common(struct block_device *bdev,
+			struct block_device *fs_dev,
+			unsigned long long start, int len, int blocksize)
 {
 	static struct lock_class_key jbd2_trans_commit_key;
 	journal_t *journal;
 	int err;
+	struct buffer_head *bh;
+	int n;
 
 	journal = kzalloc(sizeof(*journal), GFP_KERNEL);
 	if (!journal)
@@ -1131,6 +1135,35 @@ static journal_t * journal_init_common (void)
 	lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
 			 &jbd2_trans_commit_key, 0);
 
+	/* journal descriptor can store up to n blocks -bzzz */
+	journal->j_blocksize = blocksize;
+	journal->j_dev = bdev;
+	journal->j_fs_dev = fs_dev;
+	journal->j_blk_offset = start;
+	journal->j_maxlen = len;
+	jbd2_stats_proc_init(journal);
+	n = journal->j_blocksize / sizeof(journal_block_tag_t);
+	journal->j_wbufsize = n;
+	journal->j_wbuf = kmalloc_array(n, sizeof(struct buffer_head *),
+					GFP_KERNEL);
+	if (!journal->j_wbuf) {
+		jbd2_stats_proc_exit(journal);
+		kfree(journal);
+		return NULL;
+	}
+
+	bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
+	if (!bh) {
+		pr_err("%s: Cannot get buffer for journal superblock\n",
+			__func__);
+		kfree(journal->j_wbuf);
+		jbd2_stats_proc_exit(journal);
+		kfree(journal);
+		return NULL;
+	}
+	journal->j_sb_buffer = bh;
+	journal->j_superblock = (journal_superblock_t *)bh->b_data;
+
 	return journal;
 }
 
@@ -1157,51 +1190,20 @@ static journal_t * journal_init_common (void)
  *  range of blocks on an arbitrary block device.
  *
  */
-journal_t * jbd2_journal_init_dev(struct block_device *bdev,
+journal_t *jbd2_journal_init_dev(struct block_device *bdev,
 			struct block_device *fs_dev,
 			unsigned long long start, int len, int blocksize)
 {
-	journal_t *journal = journal_init_common();
-	struct buffer_head *bh;
-	int n;
+	journal_t *journal;
 
+	journal = journal_init_common(bdev, fs_dev, start, len, blocksize);
 	if (!journal)
 		return NULL;
 
-	/* journal descriptor can store up to n blocks -bzzz */
-	journal->j_blocksize = blocksize;
-	journal->j_dev = bdev;
-	journal->j_fs_dev = fs_dev;
-	journal->j_blk_offset = start;
-	journal->j_maxlen = len;
 	bdevname(journal->j_dev, journal->j_devname);
 	strreplace(journal->j_devname, '/', '!');
-	jbd2_stats_proc_init(journal);
-	n = journal->j_blocksize / sizeof(journal_block_tag_t);
-	journal->j_wbufsize = n;
-	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_KERNEL);
-	if (!journal->j_wbuf) {
-		printk(KERN_ERR "%s: Can't allocate bhs for commit thread\n",
-			__func__);
-		goto out_err;
-	}
-
-	bh = __getblk(journal->j_dev, start, journal->j_blocksize);
-	if (!bh) {
-		printk(KERN_ERR
-		       "%s: Cannot get buffer for journal superblock\n",
-		       __func__);
-		goto out_err;
-	}
-	journal->j_sb_buffer = bh;
-	journal->j_superblock = (journal_superblock_t *)bh->b_data;
 
 	return journal;
-out_err:
-	kfree(journal->j_wbuf);
-	jbd2_stats_proc_exit(journal);
-	kfree(journal);
-	return NULL;
 }
 
 /**
@@ -1212,67 +1214,35 @@ out_err:
  * the journal.  The inode must exist already, must support bmap() and
  * must have all data blocks preallocated.
  */
-journal_t * jbd2_journal_init_inode (struct inode *inode)
+journal_t *jbd2_journal_init_inode(struct inode *inode)
 {
-	struct buffer_head *bh;
-	journal_t *journal = journal_init_common();
+	journal_t *journal;
 	char *p;
-	int err;
-	int n;
 	unsigned long long blocknr;
 
+	blocknr = bmap(inode, 0);
+	if (!blocknr) {
+		pr_err("%s: Cannot locate journal superblock\n",
+			__func__);
+		return NULL;
+	}
+
+	jbd_debug(1, "JBD2: inode %s/%ld, size %lld, bits %d, blksize %ld\n",
+		  inode->i_sb->s_id, inode->i_ino, (long long) inode->i_size,
+		  inode->i_sb->s_blocksize_bits, inode->i_sb->s_blocksize);
+
+	journal = journal_init_common(inode->i_sb->s_bdev, inode->i_sb->s_bdev,
+			blocknr, inode->i_size >> inode->i_sb->s_blocksize_bits,
+			inode->i_sb->s_blocksize);
 	if (!journal)
 		return NULL;
 
-	journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
 	journal->j_inode = inode;
 	bdevname(journal->j_dev, journal->j_devname);
 	p = strreplace(journal->j_devname, '/', '!');
 	sprintf(p, "-%lu", 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,
-		  (long long) inode->i_size,
-		  inode->i_sb->s_blocksize_bits, inode->i_sb->s_blocksize);
-
-	journal->j_maxlen = inode->i_size >> inode->i_sb->s_blocksize_bits;
-	journal->j_blocksize = inode->i_sb->s_blocksize;
-	jbd2_stats_proc_init(journal);
-
-	/* journal descriptor can store up to n blocks -bzzz */
-	n = journal->j_blocksize / sizeof(journal_block_tag_t);
-	journal->j_wbufsize = n;
-	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_KERNEL);
-	if (!journal->j_wbuf) {
-		printk(KERN_ERR "%s: Can't allocate bhs for commit thread\n",
-			__func__);
-		goto out_err;
-	}
-
-	err = jbd2_journal_bmap(journal, 0, &blocknr);
-	/* If that failed, give up */
-	if (err) {
-		printk(KERN_ERR "%s: Cannot locate journal superblock\n",
-		       __func__);
-		goto out_err;
-	}
-
-	bh = getblk_unmovable(journal->j_dev, blocknr, journal->j_blocksize);
-	if (!bh) {
-		printk(KERN_ERR
-		       "%s: Cannot get buffer for journal superblock\n",
-		       __func__);
-		goto out_err;
-	}
-	journal->j_sb_buffer = bh;
-	journal->j_superblock = (journal_superblock_t *)bh->b_data;
 
 	return journal;
-out_err:
-	kfree(journal->j_wbuf);
-	jbd2_stats_proc_exit(journal);
-	kfree(journal);
-	return NULL;
 }
 
 /*
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] jbd2: move more common code into journal_init_common()
  2016-09-07 12:41       ` [PATCH] jbd2: move more common code into journal_init_common() Geliang Tang
@ 2016-09-07 13:16         ` Jan Kara
  2016-09-15 16:03           ` Theodore Ts'o
  0 siblings, 1 reply; 9+ messages in thread
From: Jan Kara @ 2016-09-07 13:16 UTC (permalink / raw)
  To: Geliang Tang
  Cc: Theodore Ts'o, Jan Kara, Eric Ren, linux-ext4, linux-kernel

On Wed 07-09-16 20:41:13, Geliang Tang wrote:
> There are some repetitive code in jbd2_journal_init_dev() and
> jbd2_journal_init_inode(). So this patch moves the common code into
> journal_init_common() helper to simplify the code. And fix the coding
> style warnings reported by checkpatch.pl by the way.
> 
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/jbd2/journal.c | 136 +++++++++++++++++++++---------------------------------
>  1 file changed, 53 insertions(+), 83 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 46261a6..07e14ef 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1090,11 +1090,15 @@ static void jbd2_stats_proc_exit(journal_t *journal)
>   * very few fields yet: that has to wait until we have created the
>   * journal structures from from scratch, or loaded them from disk. */
>  
> -static journal_t * journal_init_common (void)
> +static journal_t *journal_init_common(struct block_device *bdev,
> +			struct block_device *fs_dev,
> +			unsigned long long start, int len, int blocksize)
>  {
>  	static struct lock_class_key jbd2_trans_commit_key;
>  	journal_t *journal;
>  	int err;
> +	struct buffer_head *bh;
> +	int n;
>  
>  	journal = kzalloc(sizeof(*journal), GFP_KERNEL);
>  	if (!journal)
> @@ -1131,6 +1135,35 @@ static journal_t * journal_init_common (void)
>  	lockdep_init_map(&journal->j_trans_commit_map, "jbd2_handle",
>  			 &jbd2_trans_commit_key, 0);
>  
> +	/* journal descriptor can store up to n blocks -bzzz */
> +	journal->j_blocksize = blocksize;
> +	journal->j_dev = bdev;
> +	journal->j_fs_dev = fs_dev;
> +	journal->j_blk_offset = start;
> +	journal->j_maxlen = len;
> +	jbd2_stats_proc_init(journal);
> +	n = journal->j_blocksize / sizeof(journal_block_tag_t);
> +	journal->j_wbufsize = n;
> +	journal->j_wbuf = kmalloc_array(n, sizeof(struct buffer_head *),
> +					GFP_KERNEL);
> +	if (!journal->j_wbuf) {
> +		jbd2_stats_proc_exit(journal);
> +		kfree(journal);
> +		return NULL;
> +	}
> +
> +	bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
> +	if (!bh) {
> +		pr_err("%s: Cannot get buffer for journal superblock\n",
> +			__func__);
> +		kfree(journal->j_wbuf);
> +		jbd2_stats_proc_exit(journal);
> +		kfree(journal);
> +		return NULL;
> +	}
> +	journal->j_sb_buffer = bh;
> +	journal->j_superblock = (journal_superblock_t *)bh->b_data;
> +
>  	return journal;
>  }
>  
> @@ -1157,51 +1190,20 @@ static journal_t * journal_init_common (void)
>   *  range of blocks on an arbitrary block device.
>   *
>   */
> -journal_t * jbd2_journal_init_dev(struct block_device *bdev,
> +journal_t *jbd2_journal_init_dev(struct block_device *bdev,
>  			struct block_device *fs_dev,
>  			unsigned long long start, int len, int blocksize)
>  {
> -	journal_t *journal = journal_init_common();
> -	struct buffer_head *bh;
> -	int n;
> +	journal_t *journal;
>  
> +	journal = journal_init_common(bdev, fs_dev, start, len, blocksize);
>  	if (!journal)
>  		return NULL;
>  
> -	/* journal descriptor can store up to n blocks -bzzz */
> -	journal->j_blocksize = blocksize;
> -	journal->j_dev = bdev;
> -	journal->j_fs_dev = fs_dev;
> -	journal->j_blk_offset = start;
> -	journal->j_maxlen = len;
>  	bdevname(journal->j_dev, journal->j_devname);
>  	strreplace(journal->j_devname, '/', '!');
> -	jbd2_stats_proc_init(journal);
> -	n = journal->j_blocksize / sizeof(journal_block_tag_t);
> -	journal->j_wbufsize = n;
> -	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_KERNEL);
> -	if (!journal->j_wbuf) {
> -		printk(KERN_ERR "%s: Can't allocate bhs for commit thread\n",
> -			__func__);
> -		goto out_err;
> -	}
> -
> -	bh = __getblk(journal->j_dev, start, journal->j_blocksize);
> -	if (!bh) {
> -		printk(KERN_ERR
> -		       "%s: Cannot get buffer for journal superblock\n",
> -		       __func__);
> -		goto out_err;
> -	}
> -	journal->j_sb_buffer = bh;
> -	journal->j_superblock = (journal_superblock_t *)bh->b_data;
>  
>  	return journal;
> -out_err:
> -	kfree(journal->j_wbuf);
> -	jbd2_stats_proc_exit(journal);
> -	kfree(journal);
> -	return NULL;
>  }
>  
>  /**
> @@ -1212,67 +1214,35 @@ out_err:
>   * the journal.  The inode must exist already, must support bmap() and
>   * must have all data blocks preallocated.
>   */
> -journal_t * jbd2_journal_init_inode (struct inode *inode)
> +journal_t *jbd2_journal_init_inode(struct inode *inode)
>  {
> -	struct buffer_head *bh;
> -	journal_t *journal = journal_init_common();
> +	journal_t *journal;
>  	char *p;
> -	int err;
> -	int n;
>  	unsigned long long blocknr;
>  
> +	blocknr = bmap(inode, 0);
> +	if (!blocknr) {
> +		pr_err("%s: Cannot locate journal superblock\n",
> +			__func__);
> +		return NULL;
> +	}
> +
> +	jbd_debug(1, "JBD2: inode %s/%ld, size %lld, bits %d, blksize %ld\n",
> +		  inode->i_sb->s_id, inode->i_ino, (long long) inode->i_size,
> +		  inode->i_sb->s_blocksize_bits, inode->i_sb->s_blocksize);
> +
> +	journal = journal_init_common(inode->i_sb->s_bdev, inode->i_sb->s_bdev,
> +			blocknr, inode->i_size >> inode->i_sb->s_blocksize_bits,
> +			inode->i_sb->s_blocksize);
>  	if (!journal)
>  		return NULL;
>  
> -	journal->j_dev = journal->j_fs_dev = inode->i_sb->s_bdev;
>  	journal->j_inode = inode;
>  	bdevname(journal->j_dev, journal->j_devname);
>  	p = strreplace(journal->j_devname, '/', '!');
>  	sprintf(p, "-%lu", 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,
> -		  (long long) inode->i_size,
> -		  inode->i_sb->s_blocksize_bits, inode->i_sb->s_blocksize);
> -
> -	journal->j_maxlen = inode->i_size >> inode->i_sb->s_blocksize_bits;
> -	journal->j_blocksize = inode->i_sb->s_blocksize;
> -	jbd2_stats_proc_init(journal);
> -
> -	/* journal descriptor can store up to n blocks -bzzz */
> -	n = journal->j_blocksize / sizeof(journal_block_tag_t);
> -	journal->j_wbufsize = n;
> -	journal->j_wbuf = kmalloc(n * sizeof(struct buffer_head*), GFP_KERNEL);
> -	if (!journal->j_wbuf) {
> -		printk(KERN_ERR "%s: Can't allocate bhs for commit thread\n",
> -			__func__);
> -		goto out_err;
> -	}
> -
> -	err = jbd2_journal_bmap(journal, 0, &blocknr);
> -	/* If that failed, give up */
> -	if (err) {
> -		printk(KERN_ERR "%s: Cannot locate journal superblock\n",
> -		       __func__);
> -		goto out_err;
> -	}
> -
> -	bh = getblk_unmovable(journal->j_dev, blocknr, journal->j_blocksize);
> -	if (!bh) {
> -		printk(KERN_ERR
> -		       "%s: Cannot get buffer for journal superblock\n",
> -		       __func__);
> -		goto out_err;
> -	}
> -	journal->j_sb_buffer = bh;
> -	journal->j_superblock = (journal_superblock_t *)bh->b_data;
>  
>  	return journal;
> -out_err:
> -	kfree(journal->j_wbuf);
> -	jbd2_stats_proc_exit(journal);
> -	kfree(journal);
> -	return NULL;
>  }
>  
>  /*
> -- 
> 2.7.4
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] jbd2: move more common code into journal_init_common()
  2016-09-07 13:16         ` Jan Kara
@ 2016-09-15 16:03           ` Theodore Ts'o
  2016-09-15 19:58             ` Theodore Ts'o
  0 siblings, 1 reply; 9+ messages in thread
From: Theodore Ts'o @ 2016-09-15 16:03 UTC (permalink / raw)
  To: Jan Kara; +Cc: Geliang Tang, Jan Kara, Eric Ren, linux-ext4, linux-kernel

On Wed, Sep 07, 2016 at 03:16:24PM +0200, Jan Kara wrote:
> On Wed 07-09-16 20:41:13, Geliang Tang wrote:
> > There are some repetitive code in jbd2_journal_init_dev() and
> > jbd2_journal_init_inode(). So this patch moves the common code into
> > journal_init_common() helper to simplify the code. And fix the coding
> > style warnings reported by checkpatch.pl by the way.
> > 
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> 
> The patch looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Applied, thanks.

						- Ted

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] jbd2: move more common code into journal_init_common()
  2016-09-15 16:03           ` Theodore Ts'o
@ 2016-09-15 19:58             ` Theodore Ts'o
  2016-09-18  2:39               ` Geliang Tang
  0 siblings, 1 reply; 9+ messages in thread
From: Theodore Ts'o @ 2016-09-15 19:58 UTC (permalink / raw)
  To: Jan Kara, Geliang Tang, Jan Kara, Eric Ren, linux-ext4, linux-kernel

On Thu, Sep 15, 2016 at 12:03:09PM -0400, Theodore Ts'o wrote:
> On Wed, Sep 07, 2016 at 03:16:24PM +0200, Jan Kara wrote:
> > On Wed 07-09-16 20:41:13, Geliang Tang wrote:
> > > There are some repetitive code in jbd2_journal_init_dev() and
> > > jbd2_journal_init_inode(). So this patch moves the common code into
> > > journal_init_common() helper to simplify the code. And fix the coding
> > > style warnings reported by checkpatch.pl by the way.
> > > 
> > > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > 
> > The patch looks good to me. You can add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Applied, thanks.
> 

Hi Geiliang,

This patch is causing a WARN_ON:

[   13.923139] ------------[ cut here ]------------
[   13.924644] WARNING: CPU: 0 PID: 2534 at /usr/projects/linux/ext4/fs/proc/generic.c:369 __proc_create+0xe1/0x156
[   13.926751] name len 0
[   13.927283] Modules linked in:
[   13.927954] CPU: 0 PID: 2534 Comm: mount Tainted: G        W       4.8.0-rc4-00139-g52c4278 #685
[   13.929809] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
[   13.931643]  00000000 00000246 f3c1dd3c c13faa3e f3c1dd68 c11cc9c1 f3c1dd54 c1087d00
[   13.933425]  00000171 f4727b0c f3c1ddac f5183bc0 f3c1dd70 c1087d44 00000009 00000000
[   13.935199]  f3c1dd68 c1969704 f3c1dd84 f3c1dda0 c11cc9c1 c19696d9 00000171 c1969704
[   13.936987] Call Trace:
[   13.937507]  [<c13faa3e>] dump_stack+0x73/0xa5
[   13.938438]  [<c11cc9c1>] ? __proc_create+0xe1/0x156
[   13.939503]  [<c1087d00>] __warn+0xbc/0xd3
[   13.940404]  [<c1087d44>] warn_slowpath_fmt+0x2d/0x32
[   13.941470]  [<c11cc9c1>] __proc_create+0xe1/0x156
[   13.942461]  [<c11ccc8d>] proc_mkdir_data+0x2c/0x6e
[   13.943484]  [<c11cccf6>] proc_mkdir+0x13/0x15
[   13.944425]  [<c122a597>] journal_init_common+0x1a8/0x26a
[   13.945539]  [<c122a753>] jbd2_journal_init_inode+0xa9/0xfd
[   13.946702]  [<c11fa5cf>] ext4_fill_super+0x18e5/0x2a92
[   13.947794]  [<c1402d01>] ? bitmap_string.isra.6+0xa9/0xc1
[   13.948926]  [<c117ec1f>] mount_bdev+0x114/0x15f
[   13.950333]  [<c11f48c4>] ext4_mount+0x15/0x17
[   13.951985]  [<c11f8cea>] ? ext4_calculate_overhead+0x30e/0x30e
[   13.954172]  [<c117f49d>] mount_fs+0x58/0x115
[   13.955789]  [<c11943cb>] vfs_kern_mount+0x4c/0xae
[   13.957588]  [<c11966dc>] do_mount+0x6b0/0x8d7
[   13.959270]  [<c1405620>] ? _copy_from_user+0x44/0x57
[   13.961140]  [<c1150873>] ? strndup_user+0x31/0x42
[   13.962919]  [<c1196aab>] SyS_mount+0x57/0x7b
[   13.964609]  [<c10015b2>] do_int80_syscall_32+0x4d/0x5f
[   13.966555]  [<c16f6ccb>] entry_INT80_32+0x2f/0x2f
[   13.968402] ---[ end trace 2eb7cc6d9a94f309 ]---
[   14.017482] EXT4-fs (vdc): mounted filesystem with ordered data mode. Opts: (null)

The problem is that journal->j_devname isn't initialized until *after*
journal_init_common(), and it's used by jbd2_stats_proc_init(), which
is called by journal_init_common().

To fix it I applied the following fix on top of your patch.

       	    	    		      	     - Ted

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 07e14ef..927da49 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1141,13 +1141,11 @@ static journal_t *journal_init_common(struct block_device *bdev,
 	journal->j_fs_dev = fs_dev;
 	journal->j_blk_offset = start;
 	journal->j_maxlen = len;
-	jbd2_stats_proc_init(journal);
 	n = journal->j_blocksize / sizeof(journal_block_tag_t);
 	journal->j_wbufsize = n;
 	journal->j_wbuf = kmalloc_array(n, sizeof(struct buffer_head *),
 					GFP_KERNEL);
 	if (!journal->j_wbuf) {
-		jbd2_stats_proc_exit(journal);
 		kfree(journal);
 		return NULL;
 	}
@@ -1157,7 +1155,6 @@ static journal_t *journal_init_common(struct block_device *bdev,
 		pr_err("%s: Cannot get buffer for journal superblock\n",
 			__func__);
 		kfree(journal->j_wbuf);
-		jbd2_stats_proc_exit(journal);
 		kfree(journal);
 		return NULL;
 	}
@@ -1202,6 +1199,7 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
 
 	bdevname(journal->j_dev, journal->j_devname);
 	strreplace(journal->j_devname, '/', '!');
+	jbd2_stats_proc_init(journal);
 
 	return journal;
 }
@@ -1241,6 +1239,7 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
 	bdevname(journal->j_dev, journal->j_devname);
 	p = strreplace(journal->j_devname, '/', '!');
 	sprintf(p, "-%lu", journal->j_inode->i_ino);
+	jbd2_stats_proc_init(journal);
 
 	return journal;
 }

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] jbd2: move more common code into journal_init_common()
  2016-09-15 19:58             ` Theodore Ts'o
@ 2016-09-18  2:39               ` Geliang Tang
  0 siblings, 0 replies; 9+ messages in thread
From: Geliang Tang @ 2016-09-18  2:39 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Jan Kara, Eric Ren, linux-ext4, linux-kernel

On Thu, Sep 15, 2016 at 03:58:18PM -0400, Theodore Ts'o wrote:
> On Thu, Sep 15, 2016 at 12:03:09PM -0400, Theodore Ts'o wrote:
> > On Wed, Sep 07, 2016 at 03:16:24PM +0200, Jan Kara wrote:
> > > On Wed 07-09-16 20:41:13, Geliang Tang wrote:
> > > > There are some repetitive code in jbd2_journal_init_dev() and
> > > > jbd2_journal_init_inode(). So this patch moves the common code into
> > > > journal_init_common() helper to simplify the code. And fix the coding
> > > > style warnings reported by checkpatch.pl by the way.
> > > > 
> > > > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > > 
> > > The patch looks good to me. You can add:
> > > 
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > Applied, thanks.
> > 
> 
> Hi Geiliang,
> 
> This patch is causing a WARN_ON:
> 
> [   13.923139] ------------[ cut here ]------------
> [   13.924644] WARNING: CPU: 0 PID: 2534 at /usr/projects/linux/ext4/fs/proc/generic.c:369 __proc_create+0xe1/0x156
> [   13.926751] name len 0
> [   13.927283] Modules linked in:
> [   13.927954] CPU: 0 PID: 2534 Comm: mount Tainted: G        W       4.8.0-rc4-00139-g52c4278 #685
> [   13.929809] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
> [   13.931643]  00000000 00000246 f3c1dd3c c13faa3e f3c1dd68 c11cc9c1 f3c1dd54 c1087d00
> [   13.933425]  00000171 f4727b0c f3c1ddac f5183bc0 f3c1dd70 c1087d44 00000009 00000000
> [   13.935199]  f3c1dd68 c1969704 f3c1dd84 f3c1dda0 c11cc9c1 c19696d9 00000171 c1969704
> [   13.936987] Call Trace:
> [   13.937507]  [<c13faa3e>] dump_stack+0x73/0xa5
> [   13.938438]  [<c11cc9c1>] ? __proc_create+0xe1/0x156
> [   13.939503]  [<c1087d00>] __warn+0xbc/0xd3
> [   13.940404]  [<c1087d44>] warn_slowpath_fmt+0x2d/0x32
> [   13.941470]  [<c11cc9c1>] __proc_create+0xe1/0x156
> [   13.942461]  [<c11ccc8d>] proc_mkdir_data+0x2c/0x6e
> [   13.943484]  [<c11cccf6>] proc_mkdir+0x13/0x15
> [   13.944425]  [<c122a597>] journal_init_common+0x1a8/0x26a
> [   13.945539]  [<c122a753>] jbd2_journal_init_inode+0xa9/0xfd
> [   13.946702]  [<c11fa5cf>] ext4_fill_super+0x18e5/0x2a92
> [   13.947794]  [<c1402d01>] ? bitmap_string.isra.6+0xa9/0xc1
> [   13.948926]  [<c117ec1f>] mount_bdev+0x114/0x15f
> [   13.950333]  [<c11f48c4>] ext4_mount+0x15/0x17
> [   13.951985]  [<c11f8cea>] ? ext4_calculate_overhead+0x30e/0x30e
> [   13.954172]  [<c117f49d>] mount_fs+0x58/0x115
> [   13.955789]  [<c11943cb>] vfs_kern_mount+0x4c/0xae
> [   13.957588]  [<c11966dc>] do_mount+0x6b0/0x8d7
> [   13.959270]  [<c1405620>] ? _copy_from_user+0x44/0x57
> [   13.961140]  [<c1150873>] ? strndup_user+0x31/0x42
> [   13.962919]  [<c1196aab>] SyS_mount+0x57/0x7b
> [   13.964609]  [<c10015b2>] do_int80_syscall_32+0x4d/0x5f
> [   13.966555]  [<c16f6ccb>] entry_INT80_32+0x2f/0x2f
> [   13.968402] ---[ end trace 2eb7cc6d9a94f309 ]---
> [   14.017482] EXT4-fs (vdc): mounted filesystem with ordered data mode. Opts: (null)
> 
> The problem is that journal->j_devname isn't initialized until *after*
> journal_init_common(), and it's used by jbd2_stats_proc_init(), which
> is called by journal_init_common().
> 
> To fix it I applied the following fix on top of your patch.
> 
>        	    	    		      	     - Ted
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 07e14ef..927da49 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1141,13 +1141,11 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	journal->j_fs_dev = fs_dev;
>  	journal->j_blk_offset = start;
>  	journal->j_maxlen = len;
> -	jbd2_stats_proc_init(journal);
>  	n = journal->j_blocksize / sizeof(journal_block_tag_t);
>  	journal->j_wbufsize = n;
>  	journal->j_wbuf = kmalloc_array(n, sizeof(struct buffer_head *),
>  					GFP_KERNEL);
>  	if (!journal->j_wbuf) {
> -		jbd2_stats_proc_exit(journal);
>  		kfree(journal);
>  		return NULL;
>  	}
> @@ -1157,7 +1155,6 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  		pr_err("%s: Cannot get buffer for journal superblock\n",
>  			__func__);
>  		kfree(journal->j_wbuf);
> -		jbd2_stats_proc_exit(journal);
>  		kfree(journal);
>  		return NULL;
>  	}
> @@ -1202,6 +1199,7 @@ journal_t *jbd2_journal_init_dev(struct block_device *bdev,
>  
>  	bdevname(journal->j_dev, journal->j_devname);
>  	strreplace(journal->j_devname, '/', '!');
> +	jbd2_stats_proc_init(journal);
>  
>  	return journal;
>  }
> @@ -1241,6 +1239,7 @@ journal_t *jbd2_journal_init_inode(struct inode *inode)
>  	bdevname(journal->j_dev, journal->j_devname);
>  	p = strreplace(journal->j_devname, '/', '!');
>  	sprintf(p, "-%lu", journal->j_inode->i_ino);
> +	jbd2_stats_proc_init(journal);
>  
>  	return journal;
>  }

Thanks Ted, this looks fine to me.

-Geliang

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-09-18  2:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-31 12:23 [PATCH] jbd2: add jbd2_journal_init() helper Geliang Tang
2016-09-03  9:33 ` Eric Ren
2016-09-06 15:15   ` Jan Kara
2016-09-07 12:41     ` [PATCH] " Geliang Tang
2016-09-07 12:41       ` [PATCH] jbd2: move more common code into journal_init_common() Geliang Tang
2016-09-07 13:16         ` Jan Kara
2016-09-15 16:03           ` Theodore Ts'o
2016-09-15 19:58             ` Theodore Ts'o
2016-09-18  2:39               ` Geliang Tang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).