remove such useless code and related fault injection.
Signed-off-by: Gao Xiang <[email protected]>
---
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
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 <[email protected]>
> ---
> 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",
>
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 <[email protected]>
> > ---
> > 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",
> >
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 <[email protected]>
>>> ---
>>> 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",
>>>
> .
>
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 <[email protected]>
> >>> ---
> >>> 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",
> >>>
> > .
> >
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
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
>
>
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
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
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
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
> .
>
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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
>