Re: [f2fs-dev] [PATCH] f2fs: cleanup the f2fs_bio_alloc routine

[Ezequiel Garcia <elezegarcia@gmail.com>]

On Mon, Dec 10, 2012 at 4:04 AM, Jaegeuk Kim <jaegeuk.kim@samsung.com> 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 <jaegeuk.kim@samsung.com>
> ---
>  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