[PATCH] f2fs: bio_alloc should never fail

[PATCH] f2fs: bio_alloc should never fail

[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

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Chao Yu]

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",
> 

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Gao Xiang]

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",
> > 

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Chao Yu]

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",
>>>
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Gao Xiang]

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",
> >>>
> > .
> > 

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Theodore Y. Ts'o]

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

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Gao Xiang]

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
> 
>
 

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Theodore Y. Ts'o]

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

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Gao Xiang]

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

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Chao Yu]

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
> .
> 

Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

[Chao Yu]

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
> .
>