Re: [PATCH] f2fs: allocate a bio for discarding when actually issuing it

[Jaegeuk Kim]

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 
> > ---
> >  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(>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(>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(>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(>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, );
> > +   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(_I(sbi)->dcc_info->discard_wait_queue);
> > +   return 0;
> > +}
> > +
> >  /* This should be covered by global mutex, _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)
> >  
> >   

Re: [PATCH] f2fs: allocate a bio for discarding when actually issuing it

[Jaegeuk Kim]

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 
> > ---
> >  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(>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(>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(>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(>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, );
> > +   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(_I(sbi)->dcc_info->discard_wait_queue);
> > +   return 0;
> > +}
> > +
> >  /* This should be covered by global mutex, _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 

Re: [PATCH] f2fs: allocate a bio for discarding when actually issuing it

[Christoph Hellwig]

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 
> ---
>  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(>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(>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(>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(>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, );
> + 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(_I(sbi)->dcc_info->discard_wait_queue);
> + return 0;
> +}
> +
>  /* This should be covered by global mutex, _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);
>   

Re: [PATCH] f2fs: allocate a bio for discarding when actually issuing it

[Christoph Hellwig]

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 
> ---
>  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(>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(>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(>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(>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, );
> + 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(_I(sbi)->dcc_info->discard_wait_queue);
> + return 0;
> +}
> +
>  /* This should be covered by global mutex, _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);
>