From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ezequiel Garcia Subject: Re: [f2fs-dev] [PATCH] f2fs: cleanup the f2fs_bio_alloc routine Date: Mon, 10 Dec 2012 09:01:44 -0300 Message-ID: References: <1355123083-14085-1-git-send-email-jaegeuk.kim@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Cc: linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net To: Jaegeuk Kim Return-path: Received: from mail-ie0-f174.google.com ([209.85.223.174]:53198 "EHLO mail-ie0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751914Ab2LJMBp (ORCPT ); Mon, 10 Dec 2012 07:01:45 -0500 Received: by mail-ie0-f174.google.com with SMTP id c11so7589189ieb.19 for ; Mon, 10 Dec 2012 04:01:44 -0800 (PST) In-Reply-To: <1355123083-14085-1-git-send-email-jaegeuk.kim@samsung.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Mon, Dec 10, 2012 at 4:04 AM, Jaegeuk Kim wrote: > Do cleanup more for better code readability. > > - Change the parameter set of f2fs_bio_alloc() > This function should allocate a bio only since it is not something like > f2fs_bio_init(). Instead, the caller should initialize the allocated bio. > > - Introduce SECTOR_FROM_BLOCK > This macro translates a block address to its sector address. > > Signed-off-by: Jaegeuk Kim > --- > fs/f2fs/data.c | 5 +++-- > fs/f2fs/f2fs.h | 2 +- > fs/f2fs/segment.c | 33 ++++++++++++++++++--------------- > fs/f2fs/segment.h | 3 +++ > 4 files changed, 25 insertions(+), 18 deletions(-) > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 444c2a6..655aeab 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -343,11 +343,12 @@ int f2fs_readpage(struct f2fs_sb_info *sbi, struct page *page, > down_read(&sbi->bio_sem); > > /* Allocate a new bio */ > - bio = f2fs_bio_alloc(bdev, blk_addr << (sbi->log_blocksize - 9), > - 1, GFP_NOFS | __GFP_HIGH); > + bio = f2fs_bio_alloc(bdev, 1); > > /* Initialize the bio */ > + bio->bi_sector = SECTOR_FROM_BLOCK(sbi, blk_addr); > bio->bi_end_io = read_end_io; > + > if (bio_add_page(bio, page, PAGE_CACHE_SIZE, 0) < PAGE_CACHE_SIZE) { > kfree(bio->bi_private); > bio_put(bio); > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 8c3f1ef..2bce3a6 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -924,7 +924,7 @@ void clear_prefree_segments(struct f2fs_sb_info *); > int npages_for_summary_flush(struct f2fs_sb_info *); > void allocate_new_segments(struct f2fs_sb_info *); > struct page *get_sum_page(struct f2fs_sb_info *, unsigned int); > -struct bio *f2fs_bio_alloc(struct block_device *, sector_t, int, gfp_t); > +struct bio *f2fs_bio_alloc(struct block_device *, int); > void f2fs_submit_bio(struct f2fs_sb_info *, enum page_type, bool sync); > int write_meta_page(struct f2fs_sb_info *, struct page *, > struct writeback_control *); > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 8894b39..1b26e4e 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -643,23 +643,21 @@ static void f2fs_end_io_write(struct bio *bio, int err) > bio_put(bio); > } > > -struct bio *f2fs_bio_alloc(struct block_device *bdev, sector_t first_sector, > - int nr_vecs, gfp_t gfp_flags) > +struct bio *f2fs_bio_alloc(struct block_device *bdev, int npages) > { > struct bio *bio; > - > - /* allocate new bio */ > - bio = bio_alloc(gfp_flags, nr_vecs); > - > - bio->bi_bdev = bdev; > - bio->bi_sector = first_sector; > + struct bio_private *priv; > retry: > - bio->bi_private = kmalloc(sizeof(struct bio_private), > - GFP_NOFS | __GFP_HIGH); > - if (!bio->bi_private) { > + priv = kmalloc(sizeof(struct bio_private), GFP_NOFS); Suggestion: Given you're writing a f2fs specific allocator wrapper for kmalloc, you might want to use kmalloc_track_caller and friends. This way, if you use ftrace kmem events you'll be able to track the real caller, instead of f2fs_bio_alloc. Thanks, Ezequiel