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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> }
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 <[email protected]>
SUSE Labs, CR
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
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 <[email protected]>
---
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
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 <[email protected]>
The patch looks good to me. You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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 <[email protected]>
>
> The patch looks good to me. You can add:
>
> Reviewed-by: Jan Kara <[email protected]>
Applied, thanks.
- Ted
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 <[email protected]>
> >
> > The patch looks good to me. You can add:
> >
> > Reviewed-by: Jan Kara <[email protected]>
>
> 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;
}
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 <[email protected]>
> > >
> > > The patch looks good to me. You can add:
> > >
> > > Reviewed-by: Jan Kara <[email protected]>
> >
> > 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