* [PATCH] f2fs: bio_alloc should never fail @ 2019-10-30 3:55 Gao Xiang 2019-10-30 8:56 ` [f2fs-dev] " Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Gao Xiang @ 2019-10-30 3:55 UTC (permalink / raw) To: Jaegeuk Kim, Chao Yu Cc: Jonathan Corbet, linux-f2fs-devel, linux-doc, linux-kernel, Gao Xiang remove such useless code and related fault injection. Signed-off-by: Gao Xiang <gaoxiang25@huawei.com> --- Documentation/filesystems/f2fs.txt | 1 - fs/f2fs/data.c | 6 ++---- fs/f2fs/f2fs.h | 21 --------------------- fs/f2fs/segment.c | 5 +---- fs/f2fs/super.c | 1 - 5 files changed, 3 insertions(+), 31 deletions(-) diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt index 7e1991328473..3477c3e4c08b 100644 --- a/Documentation/filesystems/f2fs.txt +++ b/Documentation/filesystems/f2fs.txt @@ -172,7 +172,6 @@ fault_type=%d Support configuring fault injection type, should be FAULT_KVMALLOC 0x000000002 FAULT_PAGE_ALLOC 0x000000004 FAULT_PAGE_GET 0x000000008 - FAULT_ALLOC_BIO 0x000000010 FAULT_ALLOC_NID 0x000000020 FAULT_ORPHAN 0x000000040 FAULT_BLOCK 0x000000080 diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 5755e897a5f0..3b88dcb15de6 100644 --- a/fs/f2fs/data.c +++ b/fs/f2fs/data.c @@ -288,7 +288,7 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages) struct f2fs_sb_info *sbi = fio->sbi; struct bio *bio; - bio = f2fs_bio_alloc(sbi, npages, true); + bio = bio_alloc(GFP_NOIO, npages); f2fs_target_device(sbi, fio->new_blkaddr, bio); if (is_read_io(fio->op)) { @@ -682,9 +682,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, struct bio_post_read_ctx *ctx; unsigned int post_read_steps = 0; - bio = f2fs_bio_alloc(sbi, min_t(int, nr_pages, BIO_MAX_PAGES), false); - if (!bio) - return ERR_PTR(-ENOMEM); + bio = bio_alloc(GFP_KERNEL, min_t(int, nr_pages, BIO_MAX_PAGES)); f2fs_target_device(sbi, blkaddr, bio); bio->bi_end_io = f2fs_read_end_io; bio_set_op_attrs(bio, REQ_OP_READ, op_flag); diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 4024790028aa..40012f874be0 100644 --- a/fs/f2fs/f2fs.h +++ b/fs/f2fs/f2fs.h @@ -44,7 +44,6 @@ enum { FAULT_KVMALLOC, FAULT_PAGE_ALLOC, FAULT_PAGE_GET, - FAULT_ALLOC_BIO, FAULT_ALLOC_NID, FAULT_ORPHAN, FAULT_BLOCK, @@ -2210,26 +2209,6 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep, return entry; } -static inline struct bio *f2fs_bio_alloc(struct f2fs_sb_info *sbi, - int npages, bool no_fail) -{ - struct bio *bio; - - if (no_fail) { - /* No failure on bio allocation */ - bio = bio_alloc(GFP_NOIO, npages); - if (!bio) - bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL, npages); - return bio; - } - if (time_to_inject(sbi, FAULT_ALLOC_BIO)) { - f2fs_show_injection_info(FAULT_ALLOC_BIO); - return NULL; - } - - return bio_alloc(GFP_KERNEL, npages); -} - static inline bool is_idle(struct f2fs_sb_info *sbi, int type) { if (sbi->gc_mode == GC_URGENT) diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 808709581481..28457c878d0d 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -552,10 +552,7 @@ static int __submit_flush_wait(struct f2fs_sb_info *sbi, struct bio *bio; int ret; - bio = f2fs_bio_alloc(sbi, 0, false); - if (!bio) - return -ENOMEM; - + bio = bio_alloc(GFP_KERNEL, 0); bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH; bio_set_dev(bio, bdev); ret = submit_bio_wait(bio); diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index 1443cee15863..51945dd27f00 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -44,7 +44,6 @@ const char *f2fs_fault_name[FAULT_MAX] = { [FAULT_KVMALLOC] = "kvmalloc", [FAULT_PAGE_ALLOC] = "page alloc", [FAULT_PAGE_GET] = "page get", - [FAULT_ALLOC_BIO] = "alloc bio", [FAULT_ALLOC_NID] = "alloc nid", [FAULT_ORPHAN] = "orphan", [FAULT_BLOCK] = "no more block", -- 2.17.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 3:55 [PATCH] f2fs: bio_alloc should never fail Gao Xiang @ 2019-10-30 8:56 ` Chao Yu 2019-10-30 9:15 ` Gao Xiang 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2019-10-30 8:56 UTC (permalink / raw) To: Gao Xiang, Jaegeuk Kim, Chao Yu Cc: linux-f2fs-devel, linux-doc, linux-kernel, Jonathan Corbet On 2019/10/30 11:55, Gao Xiang wrote: > remove such useless code and related fault injection. Hi Xiang, Although, there is so many 'nofail' allocation in f2fs, I think we'd better avoid such allocation as much as possible (now for read path, we may allow to fail to allocate bio), I suggest to keep the failure path and bio allocation injection. It looks bio_alloc() will use its own mempool, which may suffer deadlock potentially. So how about changing to use bio_alloc_bioset(, , NULL) instead of bio_alloc()? Thanks, > > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com> > --- > Documentation/filesystems/f2fs.txt | 1 - > fs/f2fs/data.c | 6 ++---- > fs/f2fs/f2fs.h | 21 --------------------- > fs/f2fs/segment.c | 5 +---- > fs/f2fs/super.c | 1 - > 5 files changed, 3 insertions(+), 31 deletions(-) > > diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt > index 7e1991328473..3477c3e4c08b 100644 > --- a/Documentation/filesystems/f2fs.txt > +++ b/Documentation/filesystems/f2fs.txt > @@ -172,7 +172,6 @@ fault_type=%d Support configuring fault injection type, should be > FAULT_KVMALLOC 0x000000002 > FAULT_PAGE_ALLOC 0x000000004 > FAULT_PAGE_GET 0x000000008 > - FAULT_ALLOC_BIO 0x000000010 > FAULT_ALLOC_NID 0x000000020 > FAULT_ORPHAN 0x000000040 > FAULT_BLOCK 0x000000080 > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > index 5755e897a5f0..3b88dcb15de6 100644 > --- a/fs/f2fs/data.c > +++ b/fs/f2fs/data.c > @@ -288,7 +288,7 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages) > struct f2fs_sb_info *sbi = fio->sbi; > struct bio *bio; > > - bio = f2fs_bio_alloc(sbi, npages, true); > + bio = bio_alloc(GFP_NOIO, npages); > > f2fs_target_device(sbi, fio->new_blkaddr, bio); > if (is_read_io(fio->op)) { > @@ -682,9 +682,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, > struct bio_post_read_ctx *ctx; > unsigned int post_read_steps = 0; > > - bio = f2fs_bio_alloc(sbi, min_t(int, nr_pages, BIO_MAX_PAGES), false); > - if (!bio) > - return ERR_PTR(-ENOMEM); > + bio = bio_alloc(GFP_KERNEL, min_t(int, nr_pages, BIO_MAX_PAGES)); > f2fs_target_device(sbi, blkaddr, bio); > bio->bi_end_io = f2fs_read_end_io; > bio_set_op_attrs(bio, REQ_OP_READ, op_flag); > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > index 4024790028aa..40012f874be0 100644 > --- a/fs/f2fs/f2fs.h > +++ b/fs/f2fs/f2fs.h > @@ -44,7 +44,6 @@ enum { > FAULT_KVMALLOC, > FAULT_PAGE_ALLOC, > FAULT_PAGE_GET, > - FAULT_ALLOC_BIO, > FAULT_ALLOC_NID, > FAULT_ORPHAN, > FAULT_BLOCK, > @@ -2210,26 +2209,6 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep, > return entry; > } > > -static inline struct bio *f2fs_bio_alloc(struct f2fs_sb_info *sbi, > - int npages, bool no_fail) > -{ > - struct bio *bio; > - > - if (no_fail) { > - /* No failure on bio allocation */ > - bio = bio_alloc(GFP_NOIO, npages); > - if (!bio) > - bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL, npages); > - return bio; > - } > - if (time_to_inject(sbi, FAULT_ALLOC_BIO)) { > - f2fs_show_injection_info(FAULT_ALLOC_BIO); > - return NULL; > - } > - > - return bio_alloc(GFP_KERNEL, npages); > -} > - > static inline bool is_idle(struct f2fs_sb_info *sbi, int type) > { > if (sbi->gc_mode == GC_URGENT) > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 808709581481..28457c878d0d 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -552,10 +552,7 @@ static int __submit_flush_wait(struct f2fs_sb_info *sbi, > struct bio *bio; > int ret; > > - bio = f2fs_bio_alloc(sbi, 0, false); > - if (!bio) > - return -ENOMEM; > - > + bio = bio_alloc(GFP_KERNEL, 0); > bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH; > bio_set_dev(bio, bdev); > ret = submit_bio_wait(bio); > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > index 1443cee15863..51945dd27f00 100644 > --- a/fs/f2fs/super.c > +++ b/fs/f2fs/super.c > @@ -44,7 +44,6 @@ const char *f2fs_fault_name[FAULT_MAX] = { > [FAULT_KVMALLOC] = "kvmalloc", > [FAULT_PAGE_ALLOC] = "page alloc", > [FAULT_PAGE_GET] = "page get", > - [FAULT_ALLOC_BIO] = "alloc bio", > [FAULT_ALLOC_NID] = "alloc nid", > [FAULT_ORPHAN] = "orphan", > [FAULT_BLOCK] = "no more block", > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 8:56 ` [f2fs-dev] " Chao Yu @ 2019-10-30 9:15 ` Gao Xiang 2019-10-30 9:27 ` Chao Yu 0 siblings, 1 reply; 12+ messages in thread From: Gao Xiang @ 2019-10-30 9:15 UTC (permalink / raw) To: Chao Yu Cc: Jaegeuk Kim, Chao Yu, linux-f2fs-devel, linux-doc, linux-kernel, Jonathan Corbet Hi Chao, On Wed, Oct 30, 2019 at 04:56:17PM +0800, Chao Yu wrote: > On 2019/10/30 11:55, Gao Xiang wrote: > > remove such useless code and related fault injection. > > Hi Xiang, > > Although, there is so many 'nofail' allocation in f2fs, I think we'd better > avoid such allocation as much as possible (now for read path, we may allow to > fail to allocate bio), I suggest to keep the failure path and bio allocation > injection. > > It looks bio_alloc() will use its own mempool, which may suffer deadlock > potentially. So how about changing to use bio_alloc_bioset(, , NULL) instead of > bio_alloc()? Yes, I noticed the original commit 740432f83560 ("f2fs: handle failed bio allocation"), yet I don't find any real call trace clue what happened before. As my understanding, if we allocate bios without submit_bio (I mean write path) with default bs and gfp_flags GFP_NOIO or GFP_KERNEL, I think it will be slept inside mempool rather than return NULL to its caller... Please correct me if I'm wrong... I could send another patch with bio_alloc_bioset(, , NULL), I am curious to know the original issue and how it solved though... For read or flush path, since it will submit_bio and bio_alloc one by one, I think mempool will get a page quicker (memory failure path could be longer). But I can send a patch just by using bio_alloc_bioset(, , NULL) instead as you suggested later. Thanks, Gao Xiang > > Thanks, > > > > > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com> > > --- > > Documentation/filesystems/f2fs.txt | 1 - > > fs/f2fs/data.c | 6 ++---- > > fs/f2fs/f2fs.h | 21 --------------------- > > fs/f2fs/segment.c | 5 +---- > > fs/f2fs/super.c | 1 - > > 5 files changed, 3 insertions(+), 31 deletions(-) > > > > diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt > > index 7e1991328473..3477c3e4c08b 100644 > > --- a/Documentation/filesystems/f2fs.txt > > +++ b/Documentation/filesystems/f2fs.txt > > @@ -172,7 +172,6 @@ fault_type=%d Support configuring fault injection type, should be > > FAULT_KVMALLOC 0x000000002 > > FAULT_PAGE_ALLOC 0x000000004 > > FAULT_PAGE_GET 0x000000008 > > - FAULT_ALLOC_BIO 0x000000010 > > FAULT_ALLOC_NID 0x000000020 > > FAULT_ORPHAN 0x000000040 > > FAULT_BLOCK 0x000000080 > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > > index 5755e897a5f0..3b88dcb15de6 100644 > > --- a/fs/f2fs/data.c > > +++ b/fs/f2fs/data.c > > @@ -288,7 +288,7 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages) > > struct f2fs_sb_info *sbi = fio->sbi; > > struct bio *bio; > > > > - bio = f2fs_bio_alloc(sbi, npages, true); > > + bio = bio_alloc(GFP_NOIO, npages); > > > > f2fs_target_device(sbi, fio->new_blkaddr, bio); > > if (is_read_io(fio->op)) { > > @@ -682,9 +682,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, > > struct bio_post_read_ctx *ctx; > > unsigned int post_read_steps = 0; > > > > - bio = f2fs_bio_alloc(sbi, min_t(int, nr_pages, BIO_MAX_PAGES), false); > > - if (!bio) > > - return ERR_PTR(-ENOMEM); > > + bio = bio_alloc(GFP_KERNEL, min_t(int, nr_pages, BIO_MAX_PAGES)); > > f2fs_target_device(sbi, blkaddr, bio); > > bio->bi_end_io = f2fs_read_end_io; > > bio_set_op_attrs(bio, REQ_OP_READ, op_flag); > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > > index 4024790028aa..40012f874be0 100644 > > --- a/fs/f2fs/f2fs.h > > +++ b/fs/f2fs/f2fs.h > > @@ -44,7 +44,6 @@ enum { > > FAULT_KVMALLOC, > > FAULT_PAGE_ALLOC, > > FAULT_PAGE_GET, > > - FAULT_ALLOC_BIO, > > FAULT_ALLOC_NID, > > FAULT_ORPHAN, > > FAULT_BLOCK, > > @@ -2210,26 +2209,6 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep, > > return entry; > > } > > > > -static inline struct bio *f2fs_bio_alloc(struct f2fs_sb_info *sbi, > > - int npages, bool no_fail) > > -{ > > - struct bio *bio; > > - > > - if (no_fail) { > > - /* No failure on bio allocation */ > > - bio = bio_alloc(GFP_NOIO, npages); > > - if (!bio) > > - bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL, npages); > > - return bio; > > - } > > - if (time_to_inject(sbi, FAULT_ALLOC_BIO)) { > > - f2fs_show_injection_info(FAULT_ALLOC_BIO); > > - return NULL; > > - } > > - > > - return bio_alloc(GFP_KERNEL, npages); > > -} > > - > > static inline bool is_idle(struct f2fs_sb_info *sbi, int type) > > { > > if (sbi->gc_mode == GC_URGENT) > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > > index 808709581481..28457c878d0d 100644 > > --- a/fs/f2fs/segment.c > > +++ b/fs/f2fs/segment.c > > @@ -552,10 +552,7 @@ static int __submit_flush_wait(struct f2fs_sb_info *sbi, > > struct bio *bio; > > int ret; > > > > - bio = f2fs_bio_alloc(sbi, 0, false); > > - if (!bio) > > - return -ENOMEM; > > - > > + bio = bio_alloc(GFP_KERNEL, 0); > > bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH; > > bio_set_dev(bio, bdev); > > ret = submit_bio_wait(bio); > > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > > index 1443cee15863..51945dd27f00 100644 > > --- a/fs/f2fs/super.c > > +++ b/fs/f2fs/super.c > > @@ -44,7 +44,6 @@ const char *f2fs_fault_name[FAULT_MAX] = { > > [FAULT_KVMALLOC] = "kvmalloc", > > [FAULT_PAGE_ALLOC] = "page alloc", > > [FAULT_PAGE_GET] = "page get", > > - [FAULT_ALLOC_BIO] = "alloc bio", > > [FAULT_ALLOC_NID] = "alloc nid", > > [FAULT_ORPHAN] = "orphan", > > [FAULT_BLOCK] = "no more block", > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 9:15 ` Gao Xiang @ 2019-10-30 9:27 ` Chao Yu 2019-10-30 10:43 ` Gao Xiang 0 siblings, 1 reply; 12+ messages in thread From: Chao Yu @ 2019-10-30 9:27 UTC (permalink / raw) To: Gao Xiang Cc: Jaegeuk Kim, Chao Yu, linux-f2fs-devel, linux-doc, linux-kernel, Jonathan Corbet Hi Xiang, On 2019/10/30 17:15, Gao Xiang wrote: > Hi Chao, > > On Wed, Oct 30, 2019 at 04:56:17PM +0800, Chao Yu wrote: >> On 2019/10/30 11:55, Gao Xiang wrote: >>> remove such useless code and related fault injection. >> >> Hi Xiang, >> >> Although, there is so many 'nofail' allocation in f2fs, I think we'd better >> avoid such allocation as much as possible (now for read path, we may allow to >> fail to allocate bio), I suggest to keep the failure path and bio allocation >> injection. >> >> It looks bio_alloc() will use its own mempool, which may suffer deadlock >> potentially. So how about changing to use bio_alloc_bioset(, , NULL) instead of >> bio_alloc()? > > Yes, I noticed the original commit 740432f83560 ("f2fs: handle failed bio allocation"), > yet I don't find any real call trace clue what happened before. > > As my understanding, if we allocate bios without submit_bio (I mean write path) with > default bs and gfp_flags GFP_NOIO or GFP_KERNEL, I think it will be slept inside > mempool rather than return NULL to its caller... Please correct me if I'm wrong... I'm curious too... Jaegeuk may know the details. > > I could send another patch with bio_alloc_bioset(, , NULL), I am curious to know the > original issue and how it solved though... > > For read or flush path, since it will submit_bio and bio_alloc one by one, I think > mempool will get a page quicker (memory failure path could be longer). But I can > send a patch just by using bio_alloc_bioset(, , NULL) instead as you suggested later. You're right, in low memory scenario, allocation with bioset will be faster, as you mentioned offline, maybe we can add/use a priviate bioset like btrfs did rather than using global one, however, we'd better check how deadlock happen with a bioset mempool first ... Thanks, > > Thanks, > Gao Xiang > >> >> Thanks, >> >>> >>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com> >>> --- >>> Documentation/filesystems/f2fs.txt | 1 - >>> fs/f2fs/data.c | 6 ++---- >>> fs/f2fs/f2fs.h | 21 --------------------- >>> fs/f2fs/segment.c | 5 +---- >>> fs/f2fs/super.c | 1 - >>> 5 files changed, 3 insertions(+), 31 deletions(-) >>> >>> diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt >>> index 7e1991328473..3477c3e4c08b 100644 >>> --- a/Documentation/filesystems/f2fs.txt >>> +++ b/Documentation/filesystems/f2fs.txt >>> @@ -172,7 +172,6 @@ fault_type=%d Support configuring fault injection type, should be >>> FAULT_KVMALLOC 0x000000002 >>> FAULT_PAGE_ALLOC 0x000000004 >>> FAULT_PAGE_GET 0x000000008 >>> - FAULT_ALLOC_BIO 0x000000010 >>> FAULT_ALLOC_NID 0x000000020 >>> FAULT_ORPHAN 0x000000040 >>> FAULT_BLOCK 0x000000080 >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c >>> index 5755e897a5f0..3b88dcb15de6 100644 >>> --- a/fs/f2fs/data.c >>> +++ b/fs/f2fs/data.c >>> @@ -288,7 +288,7 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages) >>> struct f2fs_sb_info *sbi = fio->sbi; >>> struct bio *bio; >>> >>> - bio = f2fs_bio_alloc(sbi, npages, true); >>> + bio = bio_alloc(GFP_NOIO, npages); >>> >>> f2fs_target_device(sbi, fio->new_blkaddr, bio); >>> if (is_read_io(fio->op)) { >>> @@ -682,9 +682,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, >>> struct bio_post_read_ctx *ctx; >>> unsigned int post_read_steps = 0; >>> >>> - bio = f2fs_bio_alloc(sbi, min_t(int, nr_pages, BIO_MAX_PAGES), false); >>> - if (!bio) >>> - return ERR_PTR(-ENOMEM); >>> + bio = bio_alloc(GFP_KERNEL, min_t(int, nr_pages, BIO_MAX_PAGES)); >>> f2fs_target_device(sbi, blkaddr, bio); >>> bio->bi_end_io = f2fs_read_end_io; >>> bio_set_op_attrs(bio, REQ_OP_READ, op_flag); >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 4024790028aa..40012f874be0 100644 >>> --- a/fs/f2fs/f2fs.h >>> +++ b/fs/f2fs/f2fs.h >>> @@ -44,7 +44,6 @@ enum { >>> FAULT_KVMALLOC, >>> FAULT_PAGE_ALLOC, >>> FAULT_PAGE_GET, >>> - FAULT_ALLOC_BIO, >>> FAULT_ALLOC_NID, >>> FAULT_ORPHAN, >>> FAULT_BLOCK, >>> @@ -2210,26 +2209,6 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep, >>> return entry; >>> } >>> >>> -static inline struct bio *f2fs_bio_alloc(struct f2fs_sb_info *sbi, >>> - int npages, bool no_fail) >>> -{ >>> - struct bio *bio; >>> - >>> - if (no_fail) { >>> - /* No failure on bio allocation */ >>> - bio = bio_alloc(GFP_NOIO, npages); >>> - if (!bio) >>> - bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL, npages); >>> - return bio; >>> - } >>> - if (time_to_inject(sbi, FAULT_ALLOC_BIO)) { >>> - f2fs_show_injection_info(FAULT_ALLOC_BIO); >>> - return NULL; >>> - } >>> - >>> - return bio_alloc(GFP_KERNEL, npages); >>> -} >>> - >>> static inline bool is_idle(struct f2fs_sb_info *sbi, int type) >>> { >>> if (sbi->gc_mode == GC_URGENT) >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c >>> index 808709581481..28457c878d0d 100644 >>> --- a/fs/f2fs/segment.c >>> +++ b/fs/f2fs/segment.c >>> @@ -552,10 +552,7 @@ static int __submit_flush_wait(struct f2fs_sb_info *sbi, >>> struct bio *bio; >>> int ret; >>> >>> - bio = f2fs_bio_alloc(sbi, 0, false); >>> - if (!bio) >>> - return -ENOMEM; >>> - >>> + bio = bio_alloc(GFP_KERNEL, 0); >>> bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH; >>> bio_set_dev(bio, bdev); >>> ret = submit_bio_wait(bio); >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index 1443cee15863..51945dd27f00 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -44,7 +44,6 @@ const char *f2fs_fault_name[FAULT_MAX] = { >>> [FAULT_KVMALLOC] = "kvmalloc", >>> [FAULT_PAGE_ALLOC] = "page alloc", >>> [FAULT_PAGE_GET] = "page get", >>> - [FAULT_ALLOC_BIO] = "alloc bio", >>> [FAULT_ALLOC_NID] = "alloc nid", >>> [FAULT_ORPHAN] = "orphan", >>> [FAULT_BLOCK] = "no more block", >>> > . > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 9:27 ` Chao Yu @ 2019-10-30 10:43 ` Gao Xiang 2019-10-30 15:14 ` Theodore Y. Ts'o 0 siblings, 1 reply; 12+ messages in thread From: Gao Xiang @ 2019-10-30 10:43 UTC (permalink / raw) To: Chao Yu Cc: Jaegeuk Kim, Chao Yu, linux-f2fs-devel, linux-doc, linux-kernel, Jonathan Corbet On Wed, Oct 30, 2019 at 05:27:54PM +0800, Chao Yu wrote: > Hi Xiang, > > On 2019/10/30 17:15, Gao Xiang wrote: > > Hi Chao, > > > > On Wed, Oct 30, 2019 at 04:56:17PM +0800, Chao Yu wrote: > >> On 2019/10/30 11:55, Gao Xiang wrote: > >>> remove such useless code and related fault injection. > >> > >> Hi Xiang, > >> > >> Although, there is so many 'nofail' allocation in f2fs, I think we'd better > >> avoid such allocation as much as possible (now for read path, we may allow to > >> fail to allocate bio), I suggest to keep the failure path and bio allocation > >> injection. > >> > >> It looks bio_alloc() will use its own mempool, which may suffer deadlock > >> potentially. So how about changing to use bio_alloc_bioset(, , NULL) instead of > >> bio_alloc()? > > > > Yes, I noticed the original commit 740432f83560 ("f2fs: handle failed bio allocation"), > > yet I don't find any real call trace clue what happened before. > > > > As my understanding, if we allocate bios without submit_bio (I mean write path) with > > default bs and gfp_flags GFP_NOIO or GFP_KERNEL, I think it will be slept inside > > mempool rather than return NULL to its caller... Please correct me if I'm wrong... > > I'm curious too... > > Jaegeuk may know the details. > > > > > I could send another patch with bio_alloc_bioset(, , NULL), I am curious to know the > > original issue and how it solved though... > > > > For read or flush path, since it will submit_bio and bio_alloc one by one, I think > > mempool will get a page quicker (memory failure path could be longer). But I can > > send a patch just by using bio_alloc_bioset(, , NULL) instead as you suggested later. > > You're right, in low memory scenario, allocation with bioset will be faster, as > you mentioned offline, maybe we can add/use a priviate bioset like btrfs did > rather than using global one, however, we'd better check how deadlock happen > with a bioset mempool first ... Okay, hope to get hints from Jaegeuk and redo this patch then... Thanks, Gao Xiang > > Thanks, > > > > > Thanks, > > Gao Xiang > > > >> > >> Thanks, > >> > >>> > >>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com> > >>> --- > >>> Documentation/filesystems/f2fs.txt | 1 - > >>> fs/f2fs/data.c | 6 ++---- > >>> fs/f2fs/f2fs.h | 21 --------------------- > >>> fs/f2fs/segment.c | 5 +---- > >>> fs/f2fs/super.c | 1 - > >>> 5 files changed, 3 insertions(+), 31 deletions(-) > >>> > >>> diff --git a/Documentation/filesystems/f2fs.txt b/Documentation/filesystems/f2fs.txt > >>> index 7e1991328473..3477c3e4c08b 100644 > >>> --- a/Documentation/filesystems/f2fs.txt > >>> +++ b/Documentation/filesystems/f2fs.txt > >>> @@ -172,7 +172,6 @@ fault_type=%d Support configuring fault injection type, should be > >>> FAULT_KVMALLOC 0x000000002 > >>> FAULT_PAGE_ALLOC 0x000000004 > >>> FAULT_PAGE_GET 0x000000008 > >>> - FAULT_ALLOC_BIO 0x000000010 > >>> FAULT_ALLOC_NID 0x000000020 > >>> FAULT_ORPHAN 0x000000040 > >>> FAULT_BLOCK 0x000000080 > >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c > >>> index 5755e897a5f0..3b88dcb15de6 100644 > >>> --- a/fs/f2fs/data.c > >>> +++ b/fs/f2fs/data.c > >>> @@ -288,7 +288,7 @@ static struct bio *__bio_alloc(struct f2fs_io_info *fio, int npages) > >>> struct f2fs_sb_info *sbi = fio->sbi; > >>> struct bio *bio; > >>> > >>> - bio = f2fs_bio_alloc(sbi, npages, true); > >>> + bio = bio_alloc(GFP_NOIO, npages); > >>> > >>> f2fs_target_device(sbi, fio->new_blkaddr, bio); > >>> if (is_read_io(fio->op)) { > >>> @@ -682,9 +682,7 @@ static struct bio *f2fs_grab_read_bio(struct inode *inode, block_t blkaddr, > >>> struct bio_post_read_ctx *ctx; > >>> unsigned int post_read_steps = 0; > >>> > >>> - bio = f2fs_bio_alloc(sbi, min_t(int, nr_pages, BIO_MAX_PAGES), false); > >>> - if (!bio) > >>> - return ERR_PTR(-ENOMEM); > >>> + bio = bio_alloc(GFP_KERNEL, min_t(int, nr_pages, BIO_MAX_PAGES)); > >>> f2fs_target_device(sbi, blkaddr, bio); > >>> bio->bi_end_io = f2fs_read_end_io; > >>> bio_set_op_attrs(bio, REQ_OP_READ, op_flag); > >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h > >>> index 4024790028aa..40012f874be0 100644 > >>> --- a/fs/f2fs/f2fs.h > >>> +++ b/fs/f2fs/f2fs.h > >>> @@ -44,7 +44,6 @@ enum { > >>> FAULT_KVMALLOC, > >>> FAULT_PAGE_ALLOC, > >>> FAULT_PAGE_GET, > >>> - FAULT_ALLOC_BIO, > >>> FAULT_ALLOC_NID, > >>> FAULT_ORPHAN, > >>> FAULT_BLOCK, > >>> @@ -2210,26 +2209,6 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep, > >>> return entry; > >>> } > >>> > >>> -static inline struct bio *f2fs_bio_alloc(struct f2fs_sb_info *sbi, > >>> - int npages, bool no_fail) > >>> -{ > >>> - struct bio *bio; > >>> - > >>> - if (no_fail) { > >>> - /* No failure on bio allocation */ > >>> - bio = bio_alloc(GFP_NOIO, npages); > >>> - if (!bio) > >>> - bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL, npages); > >>> - return bio; > >>> - } > >>> - if (time_to_inject(sbi, FAULT_ALLOC_BIO)) { > >>> - f2fs_show_injection_info(FAULT_ALLOC_BIO); > >>> - return NULL; > >>> - } > >>> - > >>> - return bio_alloc(GFP_KERNEL, npages); > >>> -} > >>> - > >>> static inline bool is_idle(struct f2fs_sb_info *sbi, int type) > >>> { > >>> if (sbi->gc_mode == GC_URGENT) > >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > >>> index 808709581481..28457c878d0d 100644 > >>> --- a/fs/f2fs/segment.c > >>> +++ b/fs/f2fs/segment.c > >>> @@ -552,10 +552,7 @@ static int __submit_flush_wait(struct f2fs_sb_info *sbi, > >>> struct bio *bio; > >>> int ret; > >>> > >>> - bio = f2fs_bio_alloc(sbi, 0, false); > >>> - if (!bio) > >>> - return -ENOMEM; > >>> - > >>> + bio = bio_alloc(GFP_KERNEL, 0); > >>> bio->bi_opf = REQ_OP_WRITE | REQ_SYNC | REQ_PREFLUSH; > >>> bio_set_dev(bio, bdev); > >>> ret = submit_bio_wait(bio); > >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c > >>> index 1443cee15863..51945dd27f00 100644 > >>> --- a/fs/f2fs/super.c > >>> +++ b/fs/f2fs/super.c > >>> @@ -44,7 +44,6 @@ const char *f2fs_fault_name[FAULT_MAX] = { > >>> [FAULT_KVMALLOC] = "kvmalloc", > >>> [FAULT_PAGE_ALLOC] = "page alloc", > >>> [FAULT_PAGE_GET] = "page get", > >>> - [FAULT_ALLOC_BIO] = "alloc bio", > >>> [FAULT_ALLOC_NID] = "alloc nid", > >>> [FAULT_ORPHAN] = "orphan", > >>> [FAULT_BLOCK] = "no more block", > >>> > > . > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 10:43 ` Gao Xiang @ 2019-10-30 15:14 ` Theodore Y. Ts'o 2019-10-30 15:50 ` Gao Xiang 2019-10-31 2:03 ` Chao Yu 0 siblings, 2 replies; 12+ messages in thread From: Theodore Y. Ts'o @ 2019-10-30 15:14 UTC (permalink / raw) To: Gao Xiang Cc: Chao Yu, Jaegeuk Kim, Chao Yu, linux-f2fs-devel, linux-doc, linux-kernel, Jonathan Corbet On Wed, Oct 30, 2019 at 06:43:45PM +0800, Gao Xiang wrote: > > You're right, in low memory scenario, allocation with bioset will be faster, as > > you mentioned offline, maybe we can add/use a priviate bioset like btrfs did > > rather than using global one, however, we'd better check how deadlock happen > > with a bioset mempool first ... > > Okay, hope to get hints from Jaegeuk and redo this patch then... It's not at all clear to me that using a private bioset is a good idea for f2fs. That just means you're allocating a separate chunk of memory just for f2fs, as opposed to using the global pool. That's an additional chunk of non-swapable kernel memory that's not going to be available, in *addition* to the global mempool. Also, who else would you be contending for space with the global mempool? It's not like an mobile handset is going to have other users of the global bio mempool. On a low-end mobile handset, memory is at a premium, so wasting memory to no good effect isn't going to be a great idea. Regards, - Ted ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 15:14 ` Theodore Y. Ts'o @ 2019-10-30 15:50 ` Gao Xiang 2019-10-30 16:22 ` Theodore Y. Ts'o 2019-10-31 2:03 ` Chao Yu 1 sibling, 1 reply; 12+ messages in thread From: Gao Xiang @ 2019-10-30 15:50 UTC (permalink / raw) To: Theodore Y. Ts'o Cc: Gao Xiang, Jonathan Corbet, linux-doc, linux-kernel, linux-f2fs-devel, Jaegeuk Kim Hi Ted, On Wed, Oct 30, 2019 at 11:14:45AM -0400, Theodore Y. Ts'o wrote: > On Wed, Oct 30, 2019 at 06:43:45PM +0800, Gao Xiang wrote: > > > You're right, in low memory scenario, allocation with bioset will be faster, as > > > you mentioned offline, maybe we can add/use a priviate bioset like btrfs did > > > rather than using global one, however, we'd better check how deadlock happen > > > with a bioset mempool first ... > > > > Okay, hope to get hints from Jaegeuk and redo this patch then... > > It's not at all clear to me that using a private bioset is a good idea > for f2fs. That just means you're allocating a separate chunk of > memory just for f2fs, as opposed to using the global pool. That's an > additional chunk of non-swapable kernel memory that's not going to be > available, in *addition* to the global mempool. > > Also, who else would you be contending for space with the global > mempool? It's not like an mobile handset is going to have other users > of the global bio mempool. > > On a low-end mobile handset, memory is at a premium, so wasting memory > to no good effect isn't going to be a great idea. Thanks for your reply. I agree with your idea. Actually I think after this version patch is applied, all are the same as the previous status (whether some deadlock or not). So I'm curious about the original issue in commit 740432f83560 ("f2fs: handle failed bio allocation"). Since f2fs manages multiple write bios with its internal fio but it seems the commit is not helpful to resolve potential mempool deadlock (I'm confused since no calltrace, maybe I'm wrong)... I think it should be gotten clear first and think how to do next.. (I tend not to add another private bioset since it's unshareable as you said as well...) Thanks, Gao Xiang > > Regards, > > - Ted > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 15:50 ` Gao Xiang @ 2019-10-30 16:22 ` Theodore Y. Ts'o 2019-10-30 16:33 ` Jaegeuk Kim 0 siblings, 1 reply; 12+ messages in thread From: Theodore Y. Ts'o @ 2019-10-30 16:22 UTC (permalink / raw) To: Gao Xiang Cc: Gao Xiang, Jonathan Corbet, linux-doc, linux-kernel, linux-f2fs-devel, Jaegeuk Kim On Wed, Oct 30, 2019 at 11:50:37PM +0800, Gao Xiang wrote: > > So I'm curious about the original issue in commit 740432f83560 > ("f2fs: handle failed bio allocation"). Since f2fs manages multiple write > bios with its internal fio but it seems the commit is not helpful to > resolve potential mempool deadlock (I'm confused since no calltrace, > maybe I'm wrong)... Two possibilities come to mind. (a) It may be that on older kernels (when f2fs is backported to older Board Support Package kernels from the SOC vendors) didn't have the bio_alloc() guarantee, so it was necessary on older kernels, but not on upstream, or (b) it wasn't *actually* possible for bio_alloc() to fail and someone added the error handling in 740432f83560 out of paranoia. (Hence my suggestion that in the ext4 version of the patch, we add a code comment justifying why there was no error checking, to make it clear that this was a deliberate choice. :-) - Ted ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 16:22 ` Theodore Y. Ts'o @ 2019-10-30 16:33 ` Jaegeuk Kim 2019-10-30 16:52 ` Gao Xiang 2019-10-31 6:55 ` Chao Yu 0 siblings, 2 replies; 12+ messages in thread From: Jaegeuk Kim @ 2019-10-30 16:33 UTC (permalink / raw) To: Theodore Y. Ts'o Cc: Gao Xiang, Gao Xiang, Jonathan Corbet, linux-doc, linux-kernel, linux-f2fs-devel On 10/30, Theodore Y. Ts'o wrote: > On Wed, Oct 30, 2019 at 11:50:37PM +0800, Gao Xiang wrote: > > > > So I'm curious about the original issue in commit 740432f83560 > > ("f2fs: handle failed bio allocation"). Since f2fs manages multiple write > > bios with its internal fio but it seems the commit is not helpful to > > resolve potential mempool deadlock (I'm confused since no calltrace, > > maybe I'm wrong)... > > Two possibilities come to mind. (a) It may be that on older kernels > (when f2fs is backported to older Board Support Package kernels from > the SOC vendors) didn't have the bio_alloc() guarantee, so it was > necessary on older kernels, but not on upstream, or (b) it wasn't > *actually* possible for bio_alloc() to fail and someone added the > error handling in 740432f83560 out of paranoia. Yup, I was checking old device kernels but just stopped digging it out. Instead, I hesitate to apply this patch since I can't get why we need to get rid of this code for clean-up purpose. This may be able to bring some hassles when backporting to android/device kernels. > > (Hence my suggestion that in the ext4 version of the patch, we add a > code comment justifying why there was no error checking, to make it > clear that this was a deliberate choice. :-) > > - Ted ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 16:33 ` Jaegeuk Kim @ 2019-10-30 16:52 ` Gao Xiang 2019-10-31 6:55 ` Chao Yu 1 sibling, 0 replies; 12+ messages in thread From: Gao Xiang @ 2019-10-30 16:52 UTC (permalink / raw) To: Jaegeuk Kim Cc: Theodore Y. Ts'o, Gao Xiang, Jonathan Corbet, linux-doc, linux-kernel, linux-f2fs-devel On Wed, Oct 30, 2019 at 09:33:13AM -0700, Jaegeuk Kim wrote: > On 10/30, Theodore Y. Ts'o wrote: > > On Wed, Oct 30, 2019 at 11:50:37PM +0800, Gao Xiang wrote: > > > > > > So I'm curious about the original issue in commit 740432f83560 > > > ("f2fs: handle failed bio allocation"). Since f2fs manages multiple write > > > bios with its internal fio but it seems the commit is not helpful to > > > resolve potential mempool deadlock (I'm confused since no calltrace, > > > maybe I'm wrong)... > > > > Two possibilities come to mind. (a) It may be that on older kernels > > (when f2fs is backported to older Board Support Package kernels from > > the SOC vendors) didn't have the bio_alloc() guarantee, so it was > > necessary on older kernels, but not on upstream, or (b) it wasn't > > *actually* possible for bio_alloc() to fail and someone added the > > error handling in 740432f83560 out of paranoia. > > Yup, I was checking old device kernels but just stopped digging it out. > Instead, I hesitate to apply this patch since I can't get why we need to > get rid of this code for clean-up purpose. This may be able to bring > some hassles when backporting to android/device kernels. Yes, got you concern. As I said in other patches for many times, since you're the maintainer of f2fs, it's all up to you (I'm not paranoia). However, I think there are 2 valid reasons: 1) As a newbie of Linux filesystem. When I study or work on f2fs, and I saw these misleading code, I think I will produce similar code in the future (not everyone refers comments above bio_alloc), so such usage will spread (since one could refer some sample code from exist code); 2) Since it's upstream, I personally think appropriate cleanup is ok (anyway it kills net 20+ line dead code), and this patch I think isn't so harmful for backporting. Thanks, Gao Xiang > > > > > (Hence my suggestion that in the ext4 version of the patch, we add a > > code comment justifying why there was no error checking, to make it > > clear that this was a deliberate choice. :-) > > > > - Ted ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 16:33 ` Jaegeuk Kim 2019-10-30 16:52 ` Gao Xiang @ 2019-10-31 6:55 ` Chao Yu 1 sibling, 0 replies; 12+ messages in thread From: Chao Yu @ 2019-10-31 6:55 UTC (permalink / raw) To: Jaegeuk Kim, Theodore Y. Ts'o Cc: Jonathan Corbet, linux-doc, Gao Xiang, linux-kernel, linux-f2fs-devel On 2019/10/31 0:33, Jaegeuk Kim wrote: > On 10/30, Theodore Y. Ts'o wrote: >> On Wed, Oct 30, 2019 at 11:50:37PM +0800, Gao Xiang wrote: >>> >>> So I'm curious about the original issue in commit 740432f83560 >>> ("f2fs: handle failed bio allocation"). Since f2fs manages multiple write >>> bios with its internal fio but it seems the commit is not helpful to >>> resolve potential mempool deadlock (I'm confused since no calltrace, >>> maybe I'm wrong)... >> >> Two possibilities come to mind. (a) It may be that on older kernels >> (when f2fs is backported to older Board Support Package kernels from >> the SOC vendors) didn't have the bio_alloc() guarantee, so it was >> necessary on older kernels, but not on upstream, or (b) it wasn't >> *actually* possible for bio_alloc() to fail and someone added the >> error handling in 740432f83560 out of paranoia. > > Yup, I was checking old device kernels but just stopped digging it out. > Instead, I hesitate to apply this patch since I can't get why we need to > get rid of this code for clean-up purpose. This may be able to bring > some hassles when backporting to android/device kernels. Jaegeuk, IIUC, as getting hint from commit 740432f83560, are we trying to fix potential deadlock like this? In low memory scenario: - f2fs_write_checkpoint() - block_operations() - f2fs_sync_node_pages() step 1) flush cold nodes, allocate new bio from mempool - bio_alloc() - mempool_alloc() step 2) flush hot nodes, allocate a bio from mempool - bio_alloc() - mempool_alloc() step 3) flush warm nodes, be stuck in below call path - bio_alloc() - mempool_alloc() - loop to wait mempool element release, as we only reserved memory for two bio allocation, however above allocated two bios never getting submitted. #define BIO_POOL_SIZE 2 If so, we need avoid using bioset, or introducing private bioset, at least enlarging mempool size to three (adapt to total log headers' number)... Thanks, > >> >> (Hence my suggestion that in the ext4 version of the patch, we add a >> code comment justifying why there was no error checking, to make it >> clear that this was a deliberate choice. :-) >> >> - Ted > > > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel > . > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail 2019-10-30 15:14 ` Theodore Y. Ts'o 2019-10-30 15:50 ` Gao Xiang @ 2019-10-31 2:03 ` Chao Yu 1 sibling, 0 replies; 12+ messages in thread From: Chao Yu @ 2019-10-31 2:03 UTC (permalink / raw) To: Theodore Y. Ts'o, Gao Xiang Cc: Jaegeuk Kim, Chao Yu, linux-f2fs-devel, linux-doc, linux-kernel, Jonathan Corbet On 2019/10/30 23:14, Theodore Y. Ts'o wrote: > On Wed, Oct 30, 2019 at 06:43:45PM +0800, Gao Xiang wrote: >>> You're right, in low memory scenario, allocation with bioset will be faster, as >>> you mentioned offline, maybe we can add/use a priviate bioset like btrfs did >>> rather than using global one, however, we'd better check how deadlock happen >>> with a bioset mempool first ... >> >> Okay, hope to get hints from Jaegeuk and redo this patch then... > > It's not at all clear to me that using a private bioset is a good idea > for f2fs. That just means you're allocating a separate chunk of > memory just for f2fs, as opposed to using the global pool. That's an > additional chunk of non-swapable kernel memory that's not going to be > available, in *addition* to the global mempool. > > Also, who else would you be contending for space with the global > mempool? It's not like an mobile handset is going to have other users > of the global bio mempool. > > On a low-end mobile handset, memory is at a premium, so wasting memory > to no good effect isn't going to be a great idea. You're right, it looks that the purpose that btrfs added private bioset is to avoid abusing bio internal fields (via commit 9be3395bcd4a), f2fs has no such reason to do that now. Thanks, > > Regards, > > - Ted > . > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-10-31 6:55 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-30 3:55 [PATCH] f2fs: bio_alloc should never fail Gao Xiang 2019-10-30 8:56 ` [f2fs-dev] " Chao Yu 2019-10-30 9:15 ` Gao Xiang 2019-10-30 9:27 ` Chao Yu 2019-10-30 10:43 ` Gao Xiang 2019-10-30 15:14 ` Theodore Y. Ts'o 2019-10-30 15:50 ` Gao Xiang 2019-10-30 16:22 ` Theodore Y. Ts'o 2019-10-30 16:33 ` Jaegeuk Kim 2019-10-30 16:52 ` Gao Xiang 2019-10-31 6:55 ` Chao Yu 2019-10-31 2:03 ` Chao Yu
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).