From: Jan Kara Subject: Re: [PATCH] jbd2: move more common code into journal_init_common() Date: Wed, 7 Sep 2016 15:16:24 +0200 Message-ID: <20160907131624.GU28922@quack2.suse.cz> References: <50f2f9e8f6e503a3bd9a54b4dcb58531e18dbf52.1473240936.git.geliangtang@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Theodore Ts'o , Jan Kara , Eric Ren , linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org To: Geliang Tang Return-path: Content-Disposition: inline In-Reply-To: <50f2f9e8f6e503a3bd9a54b4dcb58531e18dbf52.1473240936.git.geliangtang@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-ext4.vger.kernel.org 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 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 SUSE Labs, CR