* [PATCH] f2fs: cleanup the f2fs_bio_alloc routine
@ 2012-12-10 7:04 Jaegeuk Kim
2012-12-10 8:46 ` Namjae Jeon
2012-12-10 12:01 ` [f2fs-dev] " Ezequiel Garcia
0 siblings, 2 replies; 4+ messages in thread
From: Jaegeuk Kim @ 2012-12-10 7:04 UTC (permalink / raw)
To: linux-fsdevel, linux-f2fs-devel; +Cc: Jaegeuk Kim
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);
+ if (!priv) {
cond_resched();
goto retry;
}
+
+ /* No failure on bio allocation */
+ bio = bio_alloc(GFP_NOIO, npages);
+ bio->bi_bdev = bdev;
+ bio->bi_private = priv;
return bio;
}
@@ -711,10 +709,15 @@ static void submit_write_page(struct f2fs_sb_info *sbi, struct page *page,
if (sbi->bio[type] && sbi->last_block_in_bio[type] != blk_addr - 1)
do_submit_bio(sbi, type, false);
alloc_new:
- if (sbi->bio[type] == NULL)
- sbi->bio[type] = f2fs_bio_alloc(bdev,
- blk_addr << (sbi->log_blocksize - 9),
- bio_get_nr_vecs(bdev), GFP_NOFS | __GFP_HIGH);
+ if (sbi->bio[type] == NULL) {
+ sbi->bio[type] = f2fs_bio_alloc(bdev, bio_get_nr_vecs(bdev));
+ sbi->bio[type]->bi_sector = SECTOR_FROM_BLOCK(sbi, blk_addr);
+ /*
+ * The end_io will be assigned at the sumbission phase.
+ * Until then, let bio_add_page() merge consecutive IOs as much
+ * as possible.
+ */
+ }
if (bio_add_page(sbi->bio[type], page, PAGE_CACHE_SIZE, 0) <
PAGE_CACHE_SIZE) {
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 2c445f8..0948405 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -82,6 +82,9 @@
(BITS_TO_LONGS(nr) * sizeof(unsigned long))
#define TOTAL_SEGS(sbi) (SM_I(sbi)->main_segments)
+#define SECTOR_FROM_BLOCK(sbi, blk_addr) \
+ (blk_addr << ((sbi)->log_blocksize - F2FS_LOG_SECTOR_SIZE))
+
/* during checkpoint, bio_private is used to synchronize the last bio */
struct bio_private {
struct f2fs_sb_info *sbi;
--
1.8.0.1.250.gb7973fb
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] f2fs: cleanup the f2fs_bio_alloc routine
2012-12-10 7:04 [PATCH] f2fs: cleanup the f2fs_bio_alloc routine Jaegeuk Kim
@ 2012-12-10 8:46 ` Namjae Jeon
2012-12-10 12:01 ` [f2fs-dev] " Ezequiel Garcia
1 sibling, 0 replies; 4+ messages in thread
From: Namjae Jeon @ 2012-12-10 8:46 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-fsdevel, linux-f2fs-devel
2012/12/10, Jaegeuk Kim <jaegeuk.kim@samsung.com>:
> 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.
>
Looks good to me!
> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
Reviewed-by: Namjae Jeon <namjae.jeon@samsung.com>
Thanks.
> ---
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: cleanup the f2fs_bio_alloc routine
2012-12-10 7:04 [PATCH] f2fs: cleanup the f2fs_bio_alloc routine Jaegeuk Kim
2012-12-10 8:46 ` Namjae Jeon
@ 2012-12-10 12:01 ` Ezequiel Garcia
2012-12-11 3:00 ` Jaegeuk Kim
1 sibling, 1 reply; 4+ messages in thread
From: Ezequiel Garcia @ 2012-12-10 12:01 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-fsdevel, linux-f2fs-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: cleanup the f2fs_bio_alloc routine
2012-12-10 12:01 ` [f2fs-dev] " Ezequiel Garcia
@ 2012-12-11 3:00 ` Jaegeuk Kim
0 siblings, 0 replies; 4+ messages in thread
From: Jaegeuk Kim @ 2012-12-11 3:00 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: linux-fsdevel, linux-f2fs-devel
[-- Attachment #1: Type: text/plain, Size: 4025 bytes --]
2012-12-10 (월), 09:01 -0300, Ezequiel Garcia:
> 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.
Ok, I'll keep in mind.
But, well, currently it seems there is no need to use that.
Thanks,
>
> Thanks,
>
> Ezequiel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jaegeuk Kim
Samsung
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-12-11 3:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-10 7:04 [PATCH] f2fs: cleanup the f2fs_bio_alloc routine Jaegeuk Kim
2012-12-10 8:46 ` Namjae Jeon
2012-12-10 12:01 ` [f2fs-dev] " Ezequiel Garcia
2012-12-11 3:00 ` Jaegeuk Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).