* Re: [PATCH] f2fs: allocate a bio for discarding when actually issuing it
2017-03-08 2:33 [PATCH] f2fs: allocate a bio for discarding when actually issuing it Jaegeuk Kim
@ 2017-03-08 15:03 ` Christoph Hellwig
2017-03-08 21:43 ` Jaegeuk Kim
2017-03-09 6:35 ` [f2fs-dev] " Chao Yu
2017-03-09 22:49 ` [PATCH v2] " Jaegeuk Kim
2 siblings, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2017-03-08 15:03 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel, linux-block
On Tue, Mar 07, 2017 at 06:33:33PM -0800, Jaegeuk Kim wrote:
> Let's allocate a bio when issuing discard commands later.
Does this solve the issue with your queue stalls?
>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> fs/f2fs/f2fs.h | 4 +-
> fs/f2fs/segment.c | 113 ++++++++++++++++++++++++++++--------------------------
> 2 files changed, 62 insertions(+), 55 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index a58c2e43bd2a..870bb4d9bc65 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -197,10 +197,12 @@ enum {
> struct discard_cmd {
> struct list_head list; /* command list */
> struct completion wait; /* compleation */
> + struct block_device *bdev; /* bdev */
> block_t lstart; /* logical start address */
> + block_t start; /* actual start address in dev */
> block_t len; /* length */
> - struct bio *bio; /* bio */
> int state; /* state */
> + int error; /* bio error */
> };
>
> struct discard_cmd_control {
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 50c65cc4645a..d8f9e6c895cd 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -636,7 +636,8 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
> }
>
> static void __add_discard_cmd(struct f2fs_sb_info *sbi,
> - struct bio *bio, block_t lstart, block_t len)
> + struct block_device *bdev, block_t lstart,
> + block_t start, block_t len)
> {
> struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> struct list_head *cmd_list = &(dcc->discard_cmd_list);
> @@ -644,9 +645,9 @@ static void __add_discard_cmd(struct f2fs_sb_info *sbi,
>
> dc = f2fs_kmem_cache_alloc(discard_cmd_slab, GFP_NOFS);
> INIT_LIST_HEAD(&dc->list);
> - dc->bio = bio;
> - bio->bi_private = dc;
> + dc->bdev = bdev;
> dc->lstart = lstart;
> + dc->start = start;
> dc->len = len;
> dc->state = D_PREP;
> init_completion(&dc->wait);
> @@ -658,22 +659,66 @@ static void __add_discard_cmd(struct f2fs_sb_info *sbi,
>
> static void __remove_discard_cmd(struct f2fs_sb_info *sbi, struct discard_cmd *dc)
> {
> - int err = dc->bio->bi_error;
> -
> if (dc->state == D_DONE)
> atomic_dec(&(SM_I(sbi)->dcc_info->submit_discard));
>
> - if (err == -EOPNOTSUPP)
> - err = 0;
> + if (dc->error == -EOPNOTSUPP)
> + dc->error = 0;
>
> - if (err)
> + if (dc->error)
> f2fs_msg(sbi->sb, KERN_INFO,
> - "Issue discard failed, ret: %d", err);
> - bio_put(dc->bio);
> + "Issue discard failed, ret: %d", dc->error);
> list_del(&dc->list);
> kmem_cache_free(discard_cmd_slab, dc);
> }
>
> +static void f2fs_submit_discard_endio(struct bio *bio)
> +{
> + struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> +
> + complete(&dc->wait);
> + dc->error = bio->bi_error;
> + dc->state = D_DONE;
> + bio_put(bio);
> +}
> +
> +/* this function is copied from blkdev_issue_discard from block/blk-lib.c */
> +static int __submit_discard_cmd(struct discard_cmd *dc)
> +{
> + struct bio *bio = NULL;
> + int err;
> +
> + err = __blkdev_issue_discard(dc->bdev,
> + SECTOR_FROM_BLOCK(dc->start),
> + SECTOR_FROM_BLOCK(dc->len),
> + GFP_NOFS, 0, &bio);
> + if (!err && bio) {
> + bio->bi_private = dc;
> + bio->bi_end_io = f2fs_submit_discard_endio;
> + bio->bi_opf |= REQ_SYNC;
> + submit_bio(bio);
> + }
> + dc->state = D_SUBMIT;
> + return err;
> +}
> +
> +static int __queue_discard_cmd(struct f2fs_sb_info *sbi,
> + struct block_device *bdev, block_t blkstart, block_t blklen)
> +{
> + block_t lblkstart = blkstart;
> +
> + trace_f2fs_issue_discard(bdev, blkstart, blklen);
> +
> + if (sbi->s_ndevs) {
> + int devi = f2fs_target_device_index(sbi, blkstart);
> +
> + blkstart -= FDEV(devi).start_blk;
> + }
> + __add_discard_cmd(sbi, bdev, lblkstart, blkstart, blklen);
> + wake_up(&SM_I(sbi)->dcc_info->discard_wait_queue);
> + return 0;
> +}
> +
> /* This should be covered by global mutex, &sit_i->sentry_lock */
> void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> {
> @@ -690,8 +735,7 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
>
> if (blkaddr == NULL_ADDR) {
> if (dc->state == D_PREP) {
> - dc->state = D_SUBMIT;
> - submit_bio(dc->bio);
> + __submit_discard_cmd(dc);
> atomic_inc(&dcc->submit_discard);
> }
> continue;
> @@ -716,14 +760,6 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> mutex_unlock(&dcc->cmd_lock);
> }
>
> -static void f2fs_submit_discard_endio(struct bio *bio)
> -{
> - struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> -
> - complete(&dc->wait);
> - dc->state = D_DONE;
> -}
> -
> static int issue_discard_thread(void *data)
> {
> struct f2fs_sb_info *sbi = data;
> @@ -742,8 +778,7 @@ static int issue_discard_thread(void *data)
> mutex_lock(&dcc->cmd_lock);
> list_for_each_entry_safe(dc, tmp, cmd_list, list) {
> if (dc->state == D_PREP) {
> - dc->state = D_SUBMIT;
> - submit_bio(dc->bio);
> + __submit_discard_cmd(dc);
> atomic_inc(&dcc->submit_discard);
> if (iter++ > DISCARD_ISSUE_RATE)
> break;
> @@ -763,36 +798,6 @@ static int issue_discard_thread(void *data)
> goto repeat;
> }
>
> -
> -/* this function is copied from blkdev_issue_discard from block/blk-lib.c */
> -static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
> - struct block_device *bdev, block_t blkstart, block_t blklen)
> -{
> - struct bio *bio = NULL;
> - block_t lblkstart = blkstart;
> - int err;
> -
> - trace_f2fs_issue_discard(bdev, blkstart, blklen);
> -
> - if (sbi->s_ndevs) {
> - int devi = f2fs_target_device_index(sbi, blkstart);
> -
> - blkstart -= FDEV(devi).start_blk;
> - }
> - err = __blkdev_issue_discard(bdev,
> - SECTOR_FROM_BLOCK(blkstart),
> - SECTOR_FROM_BLOCK(blklen),
> - GFP_NOFS, 0, &bio);
> - if (!err && bio) {
> - bio->bi_end_io = f2fs_submit_discard_endio;
> - bio->bi_opf |= REQ_SYNC;
> -
> - __add_discard_cmd(sbi, bio, lblkstart, blklen);
> - wake_up(&SM_I(sbi)->dcc_info->discard_wait_queue);
> - }
> - return err;
> -}
> -
> #ifdef CONFIG_BLK_DEV_ZONED
> static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
> struct block_device *bdev, block_t blkstart, block_t blklen)
> @@ -816,7 +821,7 @@ static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
> case BLK_ZONE_TYPE_CONVENTIONAL:
> if (!blk_queue_discard(bdev_get_queue(bdev)))
> return 0;
> - return __f2fs_issue_discard_async(sbi, bdev, lblkstart, blklen);
> + return __queue_discard_cmd(sbi, bdev, lblkstart, blklen);
> case BLK_ZONE_TYPE_SEQWRITE_REQ:
> case BLK_ZONE_TYPE_SEQWRITE_PREF:
> sector = SECTOR_FROM_BLOCK(blkstart);
> @@ -848,7 +853,7 @@ static int __issue_discard_async(struct f2fs_sb_info *sbi,
> bdev_zoned_model(bdev) != BLK_ZONED_NONE)
> return __f2fs_issue_discard_zone(sbi, bdev, blkstart, blklen);
> #endif
> - return __f2fs_issue_discard_async(sbi, bdev, blkstart, blklen);
> + return __queue_discard_cmd(sbi, bdev, blkstart, blklen);
> }
>
> static int f2fs_issue_discard(struct f2fs_sb_info *sbi,
> --
> 2.11.0
>
---end quoted text---
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] f2fs: allocate a bio for discarding when actually issuing it
2017-03-08 15:03 ` Christoph Hellwig
@ 2017-03-08 21:43 ` Jaegeuk Kim
0 siblings, 0 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2017-03-08 21:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel, linux-block
On 03/08, Christoph Hellwig wrote:
> On Tue, Mar 07, 2017 at 06:33:33PM -0800, Jaegeuk Kim wrote:
> > Let's allocate a bio when issuing discard commands later.
>
> Does this solve the issue with your queue stalls?
No, the patch just changes bio_alloc timings, but the stall happens when doing
submit_bio.
Thanks,
>
> >
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> > fs/f2fs/f2fs.h | 4 +-
> > fs/f2fs/segment.c | 113 ++++++++++++++++++++++++++++--------------------------
> > 2 files changed, 62 insertions(+), 55 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index a58c2e43bd2a..870bb4d9bc65 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -197,10 +197,12 @@ enum {
> > struct discard_cmd {
> > struct list_head list; /* command list */
> > struct completion wait; /* compleation */
> > + struct block_device *bdev; /* bdev */
> > block_t lstart; /* logical start address */
> > + block_t start; /* actual start address in dev */
> > block_t len; /* length */
> > - struct bio *bio; /* bio */
> > int state; /* state */
> > + int error; /* bio error */
> > };
> >
> > struct discard_cmd_control {
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 50c65cc4645a..d8f9e6c895cd 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -636,7 +636,8 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
> > }
> >
> > static void __add_discard_cmd(struct f2fs_sb_info *sbi,
> > - struct bio *bio, block_t lstart, block_t len)
> > + struct block_device *bdev, block_t lstart,
> > + block_t start, block_t len)
> > {
> > struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> > struct list_head *cmd_list = &(dcc->discard_cmd_list);
> > @@ -644,9 +645,9 @@ static void __add_discard_cmd(struct f2fs_sb_info *sbi,
> >
> > dc = f2fs_kmem_cache_alloc(discard_cmd_slab, GFP_NOFS);
> > INIT_LIST_HEAD(&dc->list);
> > - dc->bio = bio;
> > - bio->bi_private = dc;
> > + dc->bdev = bdev;
> > dc->lstart = lstart;
> > + dc->start = start;
> > dc->len = len;
> > dc->state = D_PREP;
> > init_completion(&dc->wait);
> > @@ -658,22 +659,66 @@ static void __add_discard_cmd(struct f2fs_sb_info *sbi,
> >
> > static void __remove_discard_cmd(struct f2fs_sb_info *sbi, struct discard_cmd *dc)
> > {
> > - int err = dc->bio->bi_error;
> > -
> > if (dc->state == D_DONE)
> > atomic_dec(&(SM_I(sbi)->dcc_info->submit_discard));
> >
> > - if (err == -EOPNOTSUPP)
> > - err = 0;
> > + if (dc->error == -EOPNOTSUPP)
> > + dc->error = 0;
> >
> > - if (err)
> > + if (dc->error)
> > f2fs_msg(sbi->sb, KERN_INFO,
> > - "Issue discard failed, ret: %d", err);
> > - bio_put(dc->bio);
> > + "Issue discard failed, ret: %d", dc->error);
> > list_del(&dc->list);
> > kmem_cache_free(discard_cmd_slab, dc);
> > }
> >
> > +static void f2fs_submit_discard_endio(struct bio *bio)
> > +{
> > + struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > +
> > + complete(&dc->wait);
> > + dc->error = bio->bi_error;
> > + dc->state = D_DONE;
> > + bio_put(bio);
> > +}
> > +
> > +/* this function is copied from blkdev_issue_discard from block/blk-lib.c */
> > +static int __submit_discard_cmd(struct discard_cmd *dc)
> > +{
> > + struct bio *bio = NULL;
> > + int err;
> > +
> > + err = __blkdev_issue_discard(dc->bdev,
> > + SECTOR_FROM_BLOCK(dc->start),
> > + SECTOR_FROM_BLOCK(dc->len),
> > + GFP_NOFS, 0, &bio);
> > + if (!err && bio) {
> > + bio->bi_private = dc;
> > + bio->bi_end_io = f2fs_submit_discard_endio;
> > + bio->bi_opf |= REQ_SYNC;
> > + submit_bio(bio);
> > + }
> > + dc->state = D_SUBMIT;
> > + return err;
> > +}
> > +
> > +static int __queue_discard_cmd(struct f2fs_sb_info *sbi,
> > + struct block_device *bdev, block_t blkstart, block_t blklen)
> > +{
> > + block_t lblkstart = blkstart;
> > +
> > + trace_f2fs_issue_discard(bdev, blkstart, blklen);
> > +
> > + if (sbi->s_ndevs) {
> > + int devi = f2fs_target_device_index(sbi, blkstart);
> > +
> > + blkstart -= FDEV(devi).start_blk;
> > + }
> > + __add_discard_cmd(sbi, bdev, lblkstart, blkstart, blklen);
> > + wake_up(&SM_I(sbi)->dcc_info->discard_wait_queue);
> > + return 0;
> > +}
> > +
> > /* This should be covered by global mutex, &sit_i->sentry_lock */
> > void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> > {
> > @@ -690,8 +735,7 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> >
> > if (blkaddr == NULL_ADDR) {
> > if (dc->state == D_PREP) {
> > - dc->state = D_SUBMIT;
> > - submit_bio(dc->bio);
> > + __submit_discard_cmd(dc);
> > atomic_inc(&dcc->submit_discard);
> > }
> > continue;
> > @@ -716,14 +760,6 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> > mutex_unlock(&dcc->cmd_lock);
> > }
> >
> > -static void f2fs_submit_discard_endio(struct bio *bio)
> > -{
> > - struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> > -
> > - complete(&dc->wait);
> > - dc->state = D_DONE;
> > -}
> > -
> > static int issue_discard_thread(void *data)
> > {
> > struct f2fs_sb_info *sbi = data;
> > @@ -742,8 +778,7 @@ static int issue_discard_thread(void *data)
> > mutex_lock(&dcc->cmd_lock);
> > list_for_each_entry_safe(dc, tmp, cmd_list, list) {
> > if (dc->state == D_PREP) {
> > - dc->state = D_SUBMIT;
> > - submit_bio(dc->bio);
> > + __submit_discard_cmd(dc);
> > atomic_inc(&dcc->submit_discard);
> > if (iter++ > DISCARD_ISSUE_RATE)
> > break;
> > @@ -763,36 +798,6 @@ static int issue_discard_thread(void *data)
> > goto repeat;
> > }
> >
> > -
> > -/* this function is copied from blkdev_issue_discard from block/blk-lib.c */
> > -static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
> > - struct block_device *bdev, block_t blkstart, block_t blklen)
> > -{
> > - struct bio *bio = NULL;
> > - block_t lblkstart = blkstart;
> > - int err;
> > -
> > - trace_f2fs_issue_discard(bdev, blkstart, blklen);
> > -
> > - if (sbi->s_ndevs) {
> > - int devi = f2fs_target_device_index(sbi, blkstart);
> > -
> > - blkstart -= FDEV(devi).start_blk;
> > - }
> > - err = __blkdev_issue_discard(bdev,
> > - SECTOR_FROM_BLOCK(blkstart),
> > - SECTOR_FROM_BLOCK(blklen),
> > - GFP_NOFS, 0, &bio);
> > - if (!err && bio) {
> > - bio->bi_end_io = f2fs_submit_discard_endio;
> > - bio->bi_opf |= REQ_SYNC;
> > -
> > - __add_discard_cmd(sbi, bio, lblkstart, blklen);
> > - wake_up(&SM_I(sbi)->dcc_info->discard_wait_queue);
> > - }
> > - return err;
> > -}
> > -
> > #ifdef CONFIG_BLK_DEV_ZONED
> > static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
> > struct block_device *bdev, block_t blkstart, block_t blklen)
> > @@ -816,7 +821,7 @@ static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
> > case BLK_ZONE_TYPE_CONVENTIONAL:
> > if (!blk_queue_discard(bdev_get_queue(bdev)))
> > return 0;
> > - return __f2fs_issue_discard_async(sbi, bdev, lblkstart, blklen);
> > + return __queue_discard_cmd(sbi, bdev, lblkstart, blklen);
> > case BLK_ZONE_TYPE_SEQWRITE_REQ:
> > case BLK_ZONE_TYPE_SEQWRITE_PREF:
> > sector = SECTOR_FROM_BLOCK(blkstart);
> > @@ -848,7 +853,7 @@ static int __issue_discard_async(struct f2fs_sb_info *sbi,
> > bdev_zoned_model(bdev) != BLK_ZONED_NONE)
> > return __f2fs_issue_discard_zone(sbi, bdev, blkstart, blklen);
> > #endif
> > - return __f2fs_issue_discard_async(sbi, bdev, blkstart, blklen);
> > + return __queue_discard_cmd(sbi, bdev, blkstart, blklen);
> > }
> >
> > static int f2fs_issue_discard(struct f2fs_sb_info *sbi,
> > --
> > 2.11.0
> >
> ---end quoted text---
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: allocate a bio for discarding when actually issuing it
2017-03-08 2:33 [PATCH] f2fs: allocate a bio for discarding when actually issuing it Jaegeuk Kim
2017-03-08 15:03 ` Christoph Hellwig
@ 2017-03-09 6:35 ` Chao Yu
2017-03-09 22:49 ` [PATCH v2] " Jaegeuk Kim
2 siblings, 0 replies; 5+ messages in thread
From: Chao Yu @ 2017-03-09 6:35 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel, linux-fsdevel, linux-f2fs-devel
Hi Jaegeuk,
On 2017/3/8 10:33, Jaegeuk Kim wrote:
> Let's allocate a bio when issuing discard commands later.
>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> fs/f2fs/f2fs.h | 4 +-
> fs/f2fs/segment.c | 113 ++++++++++++++++++++++++++++--------------------------
> 2 files changed, 62 insertions(+), 55 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index a58c2e43bd2a..870bb4d9bc65 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -197,10 +197,12 @@ enum {
> struct discard_cmd {
> struct list_head list; /* command list */
> struct completion wait; /* compleation */
> + struct block_device *bdev; /* bdev */
> block_t lstart; /* logical start address */
> + block_t start; /* actual start address in dev */
> block_t len; /* length */
> - struct bio *bio; /* bio */
> int state; /* state */
> + int error; /* bio error */
> };
>
> struct discard_cmd_control {
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 50c65cc4645a..d8f9e6c895cd 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -636,7 +636,8 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
> }
>
> static void __add_discard_cmd(struct f2fs_sb_info *sbi,
> - struct bio *bio, block_t lstart, block_t len)
> + struct block_device *bdev, block_t lstart,
> + block_t start, block_t len)
> {
> struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
> struct list_head *cmd_list = &(dcc->discard_cmd_list);
> @@ -644,9 +645,9 @@ static void __add_discard_cmd(struct f2fs_sb_info *sbi,
>
> dc = f2fs_kmem_cache_alloc(discard_cmd_slab, GFP_NOFS);
> INIT_LIST_HEAD(&dc->list);
> - dc->bio = bio;
> - bio->bi_private = dc;
> + dc->bdev = bdev;
> dc->lstart = lstart;
> + dc->start = start;
> dc->len = len;
> dc->state = D_PREP;
> init_completion(&dc->wait);
> @@ -658,22 +659,66 @@ static void __add_discard_cmd(struct f2fs_sb_info *sbi,
>
> static void __remove_discard_cmd(struct f2fs_sb_info *sbi, struct discard_cmd *dc)
> {
> - int err = dc->bio->bi_error;
> -
> if (dc->state == D_DONE)
> atomic_dec(&(SM_I(sbi)->dcc_info->submit_discard));
>
> - if (err == -EOPNOTSUPP)
> - err = 0;
> + if (dc->error == -EOPNOTSUPP)
> + dc->error = 0;
>
> - if (err)
> + if (dc->error)
> f2fs_msg(sbi->sb, KERN_INFO,
> - "Issue discard failed, ret: %d", err);
> - bio_put(dc->bio);
> + "Issue discard failed, ret: %d", dc->error);
> list_del(&dc->list);
> kmem_cache_free(discard_cmd_slab, dc);
> }
>
> +static void f2fs_submit_discard_endio(struct bio *bio)
> +{
> + struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> +
> + complete(&dc->wait);
> + dc->error = bio->bi_error;
> + dc->state = D_DONE;
> + bio_put(bio);
> +}
> +
> +/* this function is copied from blkdev_issue_discard from block/blk-lib.c */
> +static int __submit_discard_cmd(struct discard_cmd *dc)
> +{
> + struct bio *bio = NULL;
> + int err;
> +
> + err = __blkdev_issue_discard(dc->bdev,
> + SECTOR_FROM_BLOCK(dc->start),
> + SECTOR_FROM_BLOCK(dc->len),
> + GFP_NOFS, 0, &bio);
> + if (!err && bio) {
> + bio->bi_private = dc;
> + bio->bi_end_io = f2fs_submit_discard_endio;
> + bio->bi_opf |= REQ_SYNC;
> + submit_bio(bio);
Should set flag only if __blkdev_issue_discard is successful?
dc->state = D_SUBMIT;
And how about moving atomic_inc(&dcc->submit_discard); here?
Thanks,
> + }
> + dc->state = D_SUBMIT;
> + return err;
> +}
> +
> +static int __queue_discard_cmd(struct f2fs_sb_info *sbi,
> + struct block_device *bdev, block_t blkstart, block_t blklen)
> +{
> + block_t lblkstart = blkstart;
> +
> + trace_f2fs_issue_discard(bdev, blkstart, blklen);
> +
> + if (sbi->s_ndevs) {
> + int devi = f2fs_target_device_index(sbi, blkstart);
> +
> + blkstart -= FDEV(devi).start_blk;
> + }
> + __add_discard_cmd(sbi, bdev, lblkstart, blkstart, blklen);
> + wake_up(&SM_I(sbi)->dcc_info->discard_wait_queue);
> + return 0;
> +}
> +
> /* This should be covered by global mutex, &sit_i->sentry_lock */
> void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> {
> @@ -690,8 +735,7 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
>
> if (blkaddr == NULL_ADDR) {
> if (dc->state == D_PREP) {
> - dc->state = D_SUBMIT;
> - submit_bio(dc->bio);
> + __submit_discard_cmd(dc);
> atomic_inc(&dcc->submit_discard);
> }
> continue;
> @@ -716,14 +760,6 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
> mutex_unlock(&dcc->cmd_lock);
> }
>
> -static void f2fs_submit_discard_endio(struct bio *bio)
> -{
> - struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
> -
> - complete(&dc->wait);
> - dc->state = D_DONE;
> -}
> -
> static int issue_discard_thread(void *data)
> {
> struct f2fs_sb_info *sbi = data;
> @@ -742,8 +778,7 @@ static int issue_discard_thread(void *data)
> mutex_lock(&dcc->cmd_lock);
> list_for_each_entry_safe(dc, tmp, cmd_list, list) {
> if (dc->state == D_PREP) {
> - dc->state = D_SUBMIT;
> - submit_bio(dc->bio);
> + __submit_discard_cmd(dc);
> atomic_inc(&dcc->submit_discard);
> if (iter++ > DISCARD_ISSUE_RATE)
> break;
> @@ -763,36 +798,6 @@ static int issue_discard_thread(void *data)
> goto repeat;
> }
>
> -
> -/* this function is copied from blkdev_issue_discard from block/blk-lib.c */
> -static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
> - struct block_device *bdev, block_t blkstart, block_t blklen)
> -{
> - struct bio *bio = NULL;
> - block_t lblkstart = blkstart;
> - int err;
> -
> - trace_f2fs_issue_discard(bdev, blkstart, blklen);
> -
> - if (sbi->s_ndevs) {
> - int devi = f2fs_target_device_index(sbi, blkstart);
> -
> - blkstart -= FDEV(devi).start_blk;
> - }
> - err = __blkdev_issue_discard(bdev,
> - SECTOR_FROM_BLOCK(blkstart),
> - SECTOR_FROM_BLOCK(blklen),
> - GFP_NOFS, 0, &bio);
> - if (!err && bio) {
> - bio->bi_end_io = f2fs_submit_discard_endio;
> - bio->bi_opf |= REQ_SYNC;
> -
> - __add_discard_cmd(sbi, bio, lblkstart, blklen);
> - wake_up(&SM_I(sbi)->dcc_info->discard_wait_queue);
> - }
> - return err;
> -}
> -
> #ifdef CONFIG_BLK_DEV_ZONED
> static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
> struct block_device *bdev, block_t blkstart, block_t blklen)
> @@ -816,7 +821,7 @@ static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
> case BLK_ZONE_TYPE_CONVENTIONAL:
> if (!blk_queue_discard(bdev_get_queue(bdev)))
> return 0;
> - return __f2fs_issue_discard_async(sbi, bdev, lblkstart, blklen);
> + return __queue_discard_cmd(sbi, bdev, lblkstart, blklen);
> case BLK_ZONE_TYPE_SEQWRITE_REQ:
> case BLK_ZONE_TYPE_SEQWRITE_PREF:
> sector = SECTOR_FROM_BLOCK(blkstart);
> @@ -848,7 +853,7 @@ static int __issue_discard_async(struct f2fs_sb_info *sbi,
> bdev_zoned_model(bdev) != BLK_ZONED_NONE)
> return __f2fs_issue_discard_zone(sbi, bdev, blkstart, blklen);
> #endif
> - return __f2fs_issue_discard_async(sbi, bdev, blkstart, blklen);
> + return __queue_discard_cmd(sbi, bdev, blkstart, blklen);
> }
>
> static int f2fs_issue_discard(struct f2fs_sb_info *sbi,
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] f2fs: allocate a bio for discarding when actually issuing it
2017-03-08 2:33 [PATCH] f2fs: allocate a bio for discarding when actually issuing it Jaegeuk Kim
2017-03-08 15:03 ` Christoph Hellwig
2017-03-09 6:35 ` [f2fs-dev] " Chao Yu
@ 2017-03-09 22:49 ` Jaegeuk Kim
2 siblings, 0 replies; 5+ messages in thread
From: Jaegeuk Kim @ 2017-03-09 22:49 UTC (permalink / raw)
To: linux-kernel, linux-fsdevel, linux-f2fs-devel
Change log from v1:
o change error handling flows
>From 58bb056dfa207a0dc9b9432dcf78758a7784144a Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Tue, 7 Mar 2017 18:02:02 -0800
Subject: [PATCH] f2fs: allocate a bio for discarding when actually issuing it
Let's allocate a bio when issuing discard commands later.
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/f2fs.h | 4 +-
fs/f2fs/segment.c | 136 +++++++++++++++++++++++++++++-------------------------
2 files changed, 76 insertions(+), 64 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index a58c2e43bd2a..870bb4d9bc65 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -197,10 +197,12 @@ enum {
struct discard_cmd {
struct list_head list; /* command list */
struct completion wait; /* compleation */
+ struct block_device *bdev; /* bdev */
block_t lstart; /* logical start address */
+ block_t start; /* actual start address in dev */
block_t len; /* length */
- struct bio *bio; /* bio */
int state; /* state */
+ int error; /* bio error */
};
struct discard_cmd_control {
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 50c65cc4645a..2d99fb0d44c9 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -636,7 +636,8 @@ static void locate_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno)
}
static void __add_discard_cmd(struct f2fs_sb_info *sbi,
- struct bio *bio, block_t lstart, block_t len)
+ struct block_device *bdev, block_t lstart,
+ block_t start, block_t len)
{
struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
struct list_head *cmd_list = &(dcc->discard_cmd_list);
@@ -644,11 +645,12 @@ static void __add_discard_cmd(struct f2fs_sb_info *sbi,
dc = f2fs_kmem_cache_alloc(discard_cmd_slab, GFP_NOFS);
INIT_LIST_HEAD(&dc->list);
- dc->bio = bio;
- bio->bi_private = dc;
+ dc->bdev = bdev;
dc->lstart = lstart;
+ dc->start = start;
dc->len = len;
dc->state = D_PREP;
+ dc->error = 0;
init_completion(&dc->wait);
mutex_lock(&dcc->cmd_lock);
@@ -658,22 +660,74 @@ static void __add_discard_cmd(struct f2fs_sb_info *sbi,
static void __remove_discard_cmd(struct f2fs_sb_info *sbi, struct discard_cmd *dc)
{
- int err = dc->bio->bi_error;
-
if (dc->state == D_DONE)
atomic_dec(&(SM_I(sbi)->dcc_info->submit_discard));
- if (err == -EOPNOTSUPP)
- err = 0;
+ if (dc->error == -EOPNOTSUPP)
+ dc->error = 0;
- if (err)
+ if (dc->error)
f2fs_msg(sbi->sb, KERN_INFO,
- "Issue discard failed, ret: %d", err);
- bio_put(dc->bio);
+ "Issue discard failed, ret: %d", dc->error);
list_del(&dc->list);
kmem_cache_free(discard_cmd_slab, dc);
}
+static void f2fs_submit_discard_endio(struct bio *bio)
+{
+ struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
+
+ complete(&dc->wait);
+ dc->error = bio->bi_error;
+ dc->state = D_DONE;
+ bio_put(bio);
+}
+
+/* this function is copied from blkdev_issue_discard from block/blk-lib.c */
+static void __submit_discard_cmd(struct f2fs_sb_info *sbi,
+ struct discard_cmd *dc)
+{
+ struct discard_cmd_control *dcc = SM_I(sbi)->dcc_info;
+ struct bio *bio = NULL;
+
+ if (dc->state != D_PREP)
+ return;
+
+ dc->error = __blkdev_issue_discard(dc->bdev,
+ SECTOR_FROM_BLOCK(dc->start),
+ SECTOR_FROM_BLOCK(dc->len),
+ GFP_NOFS, 0, &bio);
+ if (!dc->error) {
+ if (bio) {
+ bio->bi_private = dc;
+ bio->bi_end_io = f2fs_submit_discard_endio;
+ bio->bi_opf |= REQ_SYNC;
+ submit_bio(bio);
+ }
+ dc->state = D_SUBMIT;
+ atomic_inc(&dcc->submit_discard);
+ } else {
+ __remove_discard_cmd(sbi, dc);
+ }
+}
+
+static int __queue_discard_cmd(struct f2fs_sb_info *sbi,
+ struct block_device *bdev, block_t blkstart, block_t blklen)
+{
+ block_t lblkstart = blkstart;
+
+ trace_f2fs_issue_discard(bdev, blkstart, blklen);
+
+ if (sbi->s_ndevs) {
+ int devi = f2fs_target_device_index(sbi, blkstart);
+
+ blkstart -= FDEV(devi).start_blk;
+ }
+ __add_discard_cmd(sbi, bdev, lblkstart, blkstart, blklen);
+ wake_up(&SM_I(sbi)->dcc_info->discard_wait_queue);
+ return 0;
+}
+
/* This should be covered by global mutex, &sit_i->sentry_lock */
void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
{
@@ -689,11 +743,7 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
list_for_each_entry_safe(dc, tmp, wait_list, list) {
if (blkaddr == NULL_ADDR) {
- if (dc->state == D_PREP) {
- dc->state = D_SUBMIT;
- submit_bio(dc->bio);
- atomic_inc(&dcc->submit_discard);
- }
+ __submit_discard_cmd(sbi, dc);
continue;
}
@@ -716,14 +766,6 @@ void f2fs_wait_discard_bio(struct f2fs_sb_info *sbi, block_t blkaddr)
mutex_unlock(&dcc->cmd_lock);
}
-static void f2fs_submit_discard_endio(struct bio *bio)
-{
- struct discard_cmd *dc = (struct discard_cmd *)bio->bi_private;
-
- complete(&dc->wait);
- dc->state = D_DONE;
-}
-
static int issue_discard_thread(void *data)
{
struct f2fs_sb_info *sbi = data;
@@ -741,15 +783,13 @@ static int issue_discard_thread(void *data)
mutex_lock(&dcc->cmd_lock);
list_for_each_entry_safe(dc, tmp, cmd_list, list) {
- if (dc->state == D_PREP) {
- dc->state = D_SUBMIT;
- submit_bio(dc->bio);
- atomic_inc(&dcc->submit_discard);
- if (iter++ > DISCARD_ISSUE_RATE)
- break;
- } else if (dc->state == D_DONE) {
+
+ __submit_discard_cmd(sbi, dc);
+
+ if (dc->state == D_PREP && iter++ > DISCARD_ISSUE_RATE)
+ break;
+ if (dc->state == D_DONE)
__remove_discard_cmd(sbi, dc);
- }
}
mutex_unlock(&dcc->cmd_lock);
@@ -763,36 +803,6 @@ static int issue_discard_thread(void *data)
goto repeat;
}
-
-/* this function is copied from blkdev_issue_discard from block/blk-lib.c */
-static int __f2fs_issue_discard_async(struct f2fs_sb_info *sbi,
- struct block_device *bdev, block_t blkstart, block_t blklen)
-{
- struct bio *bio = NULL;
- block_t lblkstart = blkstart;
- int err;
-
- trace_f2fs_issue_discard(bdev, blkstart, blklen);
-
- if (sbi->s_ndevs) {
- int devi = f2fs_target_device_index(sbi, blkstart);
-
- blkstart -= FDEV(devi).start_blk;
- }
- err = __blkdev_issue_discard(bdev,
- SECTOR_FROM_BLOCK(blkstart),
- SECTOR_FROM_BLOCK(blklen),
- GFP_NOFS, 0, &bio);
- if (!err && bio) {
- bio->bi_end_io = f2fs_submit_discard_endio;
- bio->bi_opf |= REQ_SYNC;
-
- __add_discard_cmd(sbi, bio, lblkstart, blklen);
- wake_up(&SM_I(sbi)->dcc_info->discard_wait_queue);
- }
- return err;
-}
-
#ifdef CONFIG_BLK_DEV_ZONED
static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
struct block_device *bdev, block_t blkstart, block_t blklen)
@@ -816,7 +826,7 @@ static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
case BLK_ZONE_TYPE_CONVENTIONAL:
if (!blk_queue_discard(bdev_get_queue(bdev)))
return 0;
- return __f2fs_issue_discard_async(sbi, bdev, lblkstart, blklen);
+ return __queue_discard_cmd(sbi, bdev, lblkstart, blklen);
case BLK_ZONE_TYPE_SEQWRITE_REQ:
case BLK_ZONE_TYPE_SEQWRITE_PREF:
sector = SECTOR_FROM_BLOCK(blkstart);
@@ -848,7 +858,7 @@ static int __issue_discard_async(struct f2fs_sb_info *sbi,
bdev_zoned_model(bdev) != BLK_ZONED_NONE)
return __f2fs_issue_discard_zone(sbi, bdev, blkstart, blklen);
#endif
- return __f2fs_issue_discard_async(sbi, bdev, blkstart, blklen);
+ return __queue_discard_cmd(sbi, bdev, blkstart, blklen);
}
static int f2fs_issue_discard(struct f2fs_sb_info *sbi,
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread