Re: [PATCH] jbd2: move more common code into journal_init_common()

2016-09-17 Thread Geliang Tang
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 
> > > 
> > > The patch looks good to me. You can add:
> > > 
> > > Reviewed-by: Jan Kara 
> > 
> > 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: GW   
> 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]   0246 f3c1dd3c c13faa3e f3c1dd68 c11cc9c1 
> f3c1dd54 c1087d00
> [   13.933425]  0171 f4727b0c f3c1ddac f5183bc0 f3c1dd70 c1087d44 
> 0009 
> [   13.935199]  f3c1dd68 c1969704 f3c1dd84 f3c1dda0 c11cc9c1 c19696d9 
> 0171 c1969704
> [   13.936987] Call Trace:
> [   13.937507]  [] dump_stack+0x73/0xa5
> [   13.938438]  [] ? __proc_create+0xe1/0x156
> [   13.939503]  [] __warn+0xbc/0xd3
> [   13.940404]  [] warn_slowpath_fmt+0x2d/0x32
> [   13.941470]  [] __proc_create+0xe1/0x156
> [   13.942461]  [] proc_mkdir_data+0x2c/0x6e
> [   13.943484]  [] proc_mkdir+0x13/0x15
> [   13.944425]  [] journal_init_common+0x1a8/0x26a
> [   13.945539]  [] jbd2_journal_init_inode+0xa9/0xfd
> [   13.946702]  [] ext4_fill_super+0x18e5/0x2a92
> [   13.947794]  [] ? bitmap_string.isra.6+0xa9/0xc1
> [   13.948926]  [] mount_bdev+0x114/0x15f
> [   13.950333]  [] ext4_mount+0x15/0x17
> [   13.951985]  [] ? ext4_calculate_overhead+0x30e/0x30e
> [   13.954172]  [] mount_fs+0x58/0x115
> [   13.955789]  [] vfs_kern_mount+0x4c/0xae
> [   13.957588]  [] do_mount+0x6b0/0x8d7
> [   13.959270]  [] ? _copy_from_user+0x44/0x57
> [   13.961140]  [] ? strndup_user+0x31/0x42
> [   13.962919]  [] SyS_mount+0x57/0x7b
> [   13.964609]  [] do_int80_syscall_32+0x4d/0x5f
> [   13.966555]  [] 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


Re: [PATCH] jbd2: move more common code into journal_init_common()

2016-09-15 Thread Theodore Ts'o
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 
> > 
> > The patch looks good to me. You can add:
> > 
> > Reviewed-by: Jan Kara 
> 
> 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: GW   
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]   0246 f3c1dd3c c13faa3e f3c1dd68 c11cc9c1 f3c1dd54 
c1087d00
[   13.933425]  0171 f4727b0c f3c1ddac f5183bc0 f3c1dd70 c1087d44 0009 

[   13.935199]  f3c1dd68 c1969704 f3c1dd84 f3c1dda0 c11cc9c1 c19696d9 0171 
c1969704
[   13.936987] Call Trace:
[   13.937507]  [] dump_stack+0x73/0xa5
[   13.938438]  [] ? __proc_create+0xe1/0x156
[   13.939503]  [] __warn+0xbc/0xd3
[   13.940404]  [] warn_slowpath_fmt+0x2d/0x32
[   13.941470]  [] __proc_create+0xe1/0x156
[   13.942461]  [] proc_mkdir_data+0x2c/0x6e
[   13.943484]  [] proc_mkdir+0x13/0x15
[   13.944425]  [] journal_init_common+0x1a8/0x26a
[   13.945539]  [] jbd2_journal_init_inode+0xa9/0xfd
[   13.946702]  [] ext4_fill_super+0x18e5/0x2a92
[   13.947794]  [] ? bitmap_string.isra.6+0xa9/0xc1
[   13.948926]  [] mount_bdev+0x114/0x15f
[   13.950333]  [] ext4_mount+0x15/0x17
[   13.951985]  [] ? ext4_calculate_overhead+0x30e/0x30e
[   13.954172]  [] mount_fs+0x58/0x115
[   13.955789]  [] vfs_kern_mount+0x4c/0xae
[   13.957588]  [] do_mount+0x6b0/0x8d7
[   13.959270]  [] ? _copy_from_user+0x44/0x57
[   13.961140]  [] ? strndup_user+0x31/0x42
[   13.962919]  [] SyS_mount+0x57/0x7b
[   13.964609]  [] do_int80_syscall_32+0x4d/0x5f
[   13.966555]  [] 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;
 }


Re: [PATCH] jbd2: move more common code into journal_init_common()

2016-09-15 Thread Theodore Ts'o
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 
> 
> The patch looks good to me. You can add:
> 
> Reviewed-by: Jan Kara 

Applied, thanks.

- Ted


Re: [PATCH] jbd2: move more common code into journal_init_common()

2016-09-07 Thread Jan Kara
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 

The patch looks good to me. You can add:

Reviewed-by: Jan Kara 

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 ino