As the below comment of bio_alloc_bioset, f2fs can allocate multiple bios at the
same time. So, we can't guarantee that bio is allocated all the time.
"
* When @bs is not NULL, if %__GFP_WAIT is set then bio_alloc will always be
* able to allocate a bio. This is due to the mempool guarantees. To make this
* work, callers must never allocate more than 1 bio at a time from this pool.
* Callers that need to allocate more than 1 bio must always submit the
* previously allocated bio for IO before attempting to allocate a new one.
* Failure to do so can cause deadlocks under memory pressure.
"
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/data.c | 3 +--
fs/f2fs/f2fs.h | 15 +++++++++++++++
fs/f2fs/segment.c | 14 +++++++++++---
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index cad9ebe..726e58b 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -90,8 +90,7 @@ static struct bio *__bio_alloc(struct f2fs_sb_info *sbi, block_t blk_addr,
{
struct bio *bio;
- /* No failure on bio allocation */
- bio = bio_alloc(GFP_NOIO, npages);
+ bio = f2fs_bio_alloc(npages);
bio->bi_bdev = sbi->sb->s_bdev;
bio->bi_iter.bi_sector = SECTOR_FROM_BLOCK(blk_addr);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 23bfc0c..a55169a 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -19,6 +19,7 @@
#include <linux/magic.h>
#include <linux/kobject.h>
#include <linux/sched.h>
+#include <linux/bio.h>
#ifdef CONFIG_F2FS_CHECK_FS
#define f2fs_bug_on(sbi, condition) BUG_ON(condition)
@@ -1253,6 +1254,20 @@ retry:
return entry;
}
+static inline struct bio *f2fs_bio_alloc(int npages)
+{
+ struct bio *bio;
+
+ /* No failure on bio allocation */
+retry:
+ bio = bio_alloc(GFP_NOIO, npages);
+ if (!bio) {
+ cond_resched();
+ goto retry;
+ }
+ return bio;
+}
+
static inline void f2fs_radix_tree_insert(struct radix_tree_root *root,
unsigned long index, void *item)
{
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 1b42656..6d919fe 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -330,10 +330,12 @@ repeat:
return 0;
if (!llist_empty(&fcc->issue_list)) {
- struct bio *bio = bio_alloc(GFP_NOIO, 0);
+ struct bio *bio;
struct flush_cmd *cmd, *next;
int ret;
+ bio = f2fs_bio_alloc(0);
+
fcc->dispatch_list = llist_del_all(&fcc->issue_list);
fcc->dispatch_list = llist_reverse_order(fcc->dispatch_list);
@@ -365,8 +367,14 @@ int f2fs_issue_flush(struct f2fs_sb_info *sbi)
if (test_opt(sbi, NOBARRIER))
return 0;
- if (!test_opt(sbi, FLUSH_MERGE))
- return blkdev_issue_flush(sbi->sb->s_bdev, GFP_KERNEL, NULL);
+ if (!test_opt(sbi, FLUSH_MERGE)) {
+ struct bio *bio = f2fs_bio_alloc(0);
+ int ret;
+
+ ret = submit_bio_wait(WRITE_FLUSH, bio);
+ bio_put(bio);
+ return ret;
+ }
init_completion(&cmd.wait);
--
2.1.1
If node blocks were already moved, we don't need to move them again.
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/gc.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 81de28d8..0a5d573 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -396,14 +396,18 @@ static void gc_node_segment(struct f2fs_sb_info *sbi,
{
bool initial = true;
struct f2fs_summary *entry;
+ block_t start_addr;
int off;
+ start_addr = START_BLOCK(sbi, segno);
+
next_step:
entry = sum;
for (off = 0; off < sbi->blocks_per_seg; off++, entry++) {
nid_t nid = le32_to_cpu(entry->nid);
struct page *node_page;
+ struct node_info ni;
/* stop BG_GC if there is not enough free sections. */
if (gc_type == BG_GC && has_not_enough_free_secs(sbi, 0))
@@ -426,6 +430,12 @@ next_step:
continue;
}
+ get_node_info(sbi, nid, &ni);
+ if (ni.blk_addr != start_addr + off) {
+ f2fs_put_page(node_page, 1);
+ continue;
+ }
+
/* set page dirty and write it */
if (gc_type == FG_GC) {
f2fs_wait_on_page_writeback(node_page, NODE);
--
2.1.1
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Saturday, August 15, 2015 7:09 AM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 2/2] f2fs: avoid garbage collecting already moved node blocks
>
> If node blocks were already moved, we don't need to move them again.
>
> Signed-off-by: Jaegeuk Kim <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Saturday, August 15, 2015 7:09 AM
> To: [email protected]; [email protected];
> [email protected]
> Cc: Jaegeuk Kim
> Subject: [f2fs-dev] [PATCH 1/2] f2fs: handle failed bio allocation
>
> As the below comment of bio_alloc_bioset, f2fs can allocate multiple bios at the
> same time. So, we can't guarantee that bio is allocated all the time.
>
> "
> * When @bs is not NULL, if %__GFP_WAIT is set then bio_alloc will always be
> * able to allocate a bio. This is due to the mempool guarantees. To make this
> * work, callers must never allocate more than 1 bio at a time from this pool.
> * Callers that need to allocate more than 1 bio must always submit the
> * previously allocated bio for IO before attempting to allocate a new one.
> * Failure to do so can cause deadlocks under memory pressure.
> "
>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/data.c | 3 +--
> fs/f2fs/f2fs.h | 15 +++++++++++++++
> fs/f2fs/segment.c | 14 +++++++++++---
> 3 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index cad9ebe..726e58b 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -90,8 +90,7 @@ static struct bio *__bio_alloc(struct f2fs_sb_info *sbi, block_t blk_addr,
> {
> struct bio *bio;
>
> - /* No failure on bio allocation */
> - bio = bio_alloc(GFP_NOIO, npages);
How about using __GFP_NOFAIL flag to avoid failing in bio_alloc instead
of adding opencode endless loop in code?
We can see the reason in this commit 647757197cd3
("mm: clarify __GFP_NOFAIL deprecation status ")
"__GFP_NOFAIL is documented as a deprecated flag since commit
478352e789f5 ("mm: add comment about deprecation of __GFP_NOFAIL").
This has discouraged people from using it but in some cases an opencoded
endless loop around allocator has been used instead. So the allocator
is not aware of the de facto __GFP_NOFAIL allocation because this
information was not communicated properly.
Let's make clear that if the allocation context really cannot afford
failure because there is no good failure policy then using __GFP_NOFAIL
is preferable to opencoding the loop outside of the allocator."
BTW, I found that f2fs_kmem_cache_alloc also could be replaced, we could
fix them together.
Thanks,
On Thu, Aug 20, 2015 at 05:08:24PM +0800, Chao Yu wrote:
> Hi Jaegeuk,
>
> > -----Original Message-----
> > From: Jaegeuk Kim [mailto:[email protected]]
> > Sent: Saturday, August 15, 2015 7:09 AM
> > To: [email protected]; [email protected];
> > [email protected]
> > Cc: Jaegeuk Kim
> > Subject: [f2fs-dev] [PATCH 1/2] f2fs: handle failed bio allocation
> >
> > As the below comment of bio_alloc_bioset, f2fs can allocate multiple bios at the
> > same time. So, we can't guarantee that bio is allocated all the time.
> >
> > "
> > * When @bs is not NULL, if %__GFP_WAIT is set then bio_alloc will always be
> > * able to allocate a bio. This is due to the mempool guarantees. To make this
> > * work, callers must never allocate more than 1 bio at a time from this pool.
> > * Callers that need to allocate more than 1 bio must always submit the
> > * previously allocated bio for IO before attempting to allocate a new one.
> > * Failure to do so can cause deadlocks under memory pressure.
> > "
> >
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> > fs/f2fs/data.c | 3 +--
> > fs/f2fs/f2fs.h | 15 +++++++++++++++
> > fs/f2fs/segment.c | 14 +++++++++++---
> > 3 files changed, 27 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index cad9ebe..726e58b 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -90,8 +90,7 @@ static struct bio *__bio_alloc(struct f2fs_sb_info *sbi, block_t blk_addr,
> > {
> > struct bio *bio;
> >
> > - /* No failure on bio allocation */
> > - bio = bio_alloc(GFP_NOIO, npages);
>
> How about using __GFP_NOFAIL flag to avoid failing in bio_alloc instead
> of adding opencode endless loop in code?
>
> We can see the reason in this commit 647757197cd3
> ("mm: clarify __GFP_NOFAIL deprecation status ")
>
> "__GFP_NOFAIL is documented as a deprecated flag since commit
> 478352e789f5 ("mm: add comment about deprecation of __GFP_NOFAIL").
>
> This has discouraged people from using it but in some cases an opencoded
> endless loop around allocator has been used instead. So the allocator
> is not aware of the de facto __GFP_NOFAIL allocation because this
> information was not communicated properly.
>
> Let's make clear that if the allocation context really cannot afford
> failure because there is no good failure policy then using __GFP_NOFAIL
> is preferable to opencoding the loop outside of the allocator."
>
> BTW, I found that f2fs_kmem_cache_alloc also could be replaced, we could
> fix them together.
Agreed. I think that can be another patch like this.
>From 1579e0d1ada96994c4ec6619fb5b5d9386e77ab3 Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Thu, 20 Aug 2015 08:51:56 -0700
Subject: [PATCH] f2fs: use __GFP_NOFAIL to avoid infinite loop
__GFP_NOFAIL can avoid retrying the whole path of kmem_cache_alloc and
bio_alloc.
Suggested-by: Chao Yu <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/f2fs.h | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 00591f7..c78b599 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1244,13 +1244,10 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep,
gfp_t flags)
{
void *entry;
-retry:
- entry = kmem_cache_alloc(cachep, flags);
- if (!entry) {
- cond_resched();
- goto retry;
- }
+ entry = kmem_cache_alloc(cachep, flags);
+ if (!entry)
+ entry = kmem_cache_alloc(cachep, flags | __GFP_NOFAIL);
return entry;
}
@@ -1259,12 +1256,9 @@ static inline struct bio *f2fs_bio_alloc(int npages)
struct bio *bio;
/* No failure on bio allocation */
-retry:
bio = bio_alloc(GFP_NOIO, npages);
- if (!bio) {
- cond_resched();
- goto retry;
- }
+ if (!bio)
+ bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL, npages);
return bio;
}
--
2.1.1
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Thursday, August 20, 2015 11:57 PM
> To: Chao Yu
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH 1/2] f2fs: handle failed bio allocation
>
> On Thu, Aug 20, 2015 at 05:08:24PM +0800, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > > -----Original Message-----
> > > From: Jaegeuk Kim [mailto:[email protected]]
> > > Sent: Saturday, August 15, 2015 7:09 AM
> > > To: [email protected]; [email protected];
> > > [email protected]
> > > Cc: Jaegeuk Kim
> > > Subject: [f2fs-dev] [PATCH 1/2] f2fs: handle failed bio allocation
> > >
> > > As the below comment of bio_alloc_bioset, f2fs can allocate multiple bios at the
> > > same time. So, we can't guarantee that bio is allocated all the time.
> > >
> > > "
> > > * When @bs is not NULL, if %__GFP_WAIT is set then bio_alloc will always be
> > > * able to allocate a bio. This is due to the mempool guarantees. To make this
> > > * work, callers must never allocate more than 1 bio at a time from this pool.
> > > * Callers that need to allocate more than 1 bio must always submit the
> > > * previously allocated bio for IO before attempting to allocate a new one.
> > > * Failure to do so can cause deadlocks under memory pressure.
> > > "
> > >
> > > Signed-off-by: Jaegeuk Kim <[email protected]>
> > > ---
> > > fs/f2fs/data.c | 3 +--
> > > fs/f2fs/f2fs.h | 15 +++++++++++++++
> > > fs/f2fs/segment.c | 14 +++++++++++---
> > > 3 files changed, 27 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > > index cad9ebe..726e58b 100644
> > > --- a/fs/f2fs/data.c
> > > +++ b/fs/f2fs/data.c
> > > @@ -90,8 +90,7 @@ static struct bio *__bio_alloc(struct f2fs_sb_info *sbi, block_t blk_addr,
> > > {
> > > struct bio *bio;
> > >
> > > - /* No failure on bio allocation */
> > > - bio = bio_alloc(GFP_NOIO, npages);
> >
> > How about using __GFP_NOFAIL flag to avoid failing in bio_alloc instead
> > of adding opencode endless loop in code?
> >
> > We can see the reason in this commit 647757197cd3
> > ("mm: clarify __GFP_NOFAIL deprecation status ")
> >
> > "__GFP_NOFAIL is documented as a deprecated flag since commit
> > 478352e789f5 ("mm: add comment about deprecation of __GFP_NOFAIL").
> >
> > This has discouraged people from using it but in some cases an opencoded
> > endless loop around allocator has been used instead. So the allocator
> > is not aware of the de facto __GFP_NOFAIL allocation because this
> > information was not communicated properly.
> >
> > Let's make clear that if the allocation context really cannot afford
> > failure because there is no good failure policy then using __GFP_NOFAIL
> > is preferable to opencoding the loop outside of the allocator."
> >
> > BTW, I found that f2fs_kmem_cache_alloc also could be replaced, we could
> > fix them together.
>
> Agreed. I think that can be another patch like this.
>
> From 1579e0d1ada96994c4ec6619fb5b5d9386e77ab3 Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <[email protected]>
> Date: Thu, 20 Aug 2015 08:51:56 -0700
> Subject: [PATCH] f2fs: use __GFP_NOFAIL to avoid infinite loop
>
> __GFP_NOFAIL can avoid retrying the whole path of kmem_cache_alloc and
> bio_alloc.
>
> Suggested-by: Chao Yu <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> fs/f2fs/f2fs.h | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 00591f7..c78b599 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1244,13 +1244,10 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep,
> gfp_t flags)
> {
> void *entry;
> -retry:
> - entry = kmem_cache_alloc(cachep, flags);
> - if (!entry) {
> - cond_resched();
> - goto retry;
> - }
>
> + entry = kmem_cache_alloc(cachep, flags);
> + if (!entry)
> + entry = kmem_cache_alloc(cachep, flags | __GFP_NOFAIL);
The fast + slow path model looks good to me, expect one thing:
In several paths of checkpoint, caller will grab slab cache with GFP_ATOMIC,
so in slow path, our flags will be GFP_ATOMIC | __GFP_NOFAIL, I'm not sure
that the two flags can be used together.
Should we replace GFP_ATOMIC with GFP_NOFS in flags if caller passed
GFP_ATOMIC?
Thanks,
> return entry;
> }
>
> @@ -1259,12 +1256,9 @@ static inline struct bio *f2fs_bio_alloc(int npages)
> struct bio *bio;
>
> /* No failure on bio allocation */
> -retry:
> bio = bio_alloc(GFP_NOIO, npages);
> - if (!bio) {
> - cond_resched();
> - goto retry;
> - }
> + if (!bio)
> + bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL, npages);
> return bio;
> }
>
> --
> 2.1.1
Hi Chao,
[snip]
> > > >
> > > > - /* No failure on bio allocation */
> > > > - bio = bio_alloc(GFP_NOIO, npages);
> > >
> > > How about using __GFP_NOFAIL flag to avoid failing in bio_alloc instead
> > > of adding opencode endless loop in code?
> > >
> > > We can see the reason in this commit 647757197cd3
> > > ("mm: clarify __GFP_NOFAIL deprecation status ")
> > >
> > > "__GFP_NOFAIL is documented as a deprecated flag since commit
> > > 478352e789f5 ("mm: add comment about deprecation of __GFP_NOFAIL").
> > >
> > > This has discouraged people from using it but in some cases an opencoded
> > > endless loop around allocator has been used instead. So the allocator
> > > is not aware of the de facto __GFP_NOFAIL allocation because this
> > > information was not communicated properly.
> > >
> > > Let's make clear that if the allocation context really cannot afford
> > > failure because there is no good failure policy then using __GFP_NOFAIL
> > > is preferable to opencoding the loop outside of the allocator."
> > >
> > > BTW, I found that f2fs_kmem_cache_alloc also could be replaced, we could
> > > fix them together.
> >
> > Agreed. I think that can be another patch like this.
> >
> > From 1579e0d1ada96994c4ec6619fb5b5d9386e77ab3 Mon Sep 17 00:00:00 2001
> > From: Jaegeuk Kim <[email protected]>
> > Date: Thu, 20 Aug 2015 08:51:56 -0700
> > Subject: [PATCH] f2fs: use __GFP_NOFAIL to avoid infinite loop
> >
> > __GFP_NOFAIL can avoid retrying the whole path of kmem_cache_alloc and
> > bio_alloc.
> >
> > Suggested-by: Chao Yu <[email protected]>
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> > fs/f2fs/f2fs.h | 16 +++++-----------
> > 1 file changed, 5 insertions(+), 11 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 00591f7..c78b599 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -1244,13 +1244,10 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep,
> > gfp_t flags)
> > {
> > void *entry;
> > -retry:
> > - entry = kmem_cache_alloc(cachep, flags);
> > - if (!entry) {
> > - cond_resched();
> > - goto retry;
> > - }
> >
> > + entry = kmem_cache_alloc(cachep, flags);
> > + if (!entry)
> > + entry = kmem_cache_alloc(cachep, flags | __GFP_NOFAIL);
>
> The fast + slow path model looks good to me, expect one thing:
> In several paths of checkpoint, caller will grab slab cache with GFP_ATOMIC,
> so in slow path, our flags will be GFP_ATOMIC | __GFP_NOFAIL, I'm not sure
> that the two flags can be used together.
>
> Should we replace GFP_ATOMIC with GFP_NOFS in flags if caller passed
> GFP_ATOMIC?
Indeed, we need to avoid GFP_ATOMIC as much as possible to mitigate memory
pressure at this moment. Too much abused.
I wrote a patch like this.
>From a9209556d024cdce490695586ecee3164efda49c Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
Date: Thu, 20 Aug 2015 08:51:56 -0700
Subject: [PATCH] f2fs: use __GFP_NOFAIL to avoid infinite loop
__GFP_NOFAIL can avoid retrying the whole path of kmem_cache_alloc and
bio_alloc.
And, it also fixes the use cases of GFP_ATOMIC correctly.
Suggested-by: Chao Yu <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/checkpoint.c | 21 ++++++++-------------
fs/f2fs/f2fs.h | 16 +++++-----------
fs/f2fs/node.c | 4 ++--
fs/f2fs/segment.c | 2 +-
4 files changed, 16 insertions(+), 27 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 890e4d4..c5a38e3 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -336,26 +336,18 @@ const struct address_space_operations f2fs_meta_aops = {
static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
{
struct inode_management *im = &sbi->im[type];
- struct ino_entry *e;
+ struct ino_entry *e, *tmp;
+
+ tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
retry:
- if (radix_tree_preload(GFP_NOFS)) {
- cond_resched();
- goto retry;
- }
+ radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
spin_lock(&im->ino_lock);
-
e = radix_tree_lookup(&im->ino_root, ino);
if (!e) {
- e = kmem_cache_alloc(ino_entry_slab, GFP_ATOMIC);
- if (!e) {
- spin_unlock(&im->ino_lock);
- radix_tree_preload_end();
- goto retry;
- }
+ e = tmp;
if (radix_tree_insert(&im->ino_root, ino, e)) {
spin_unlock(&im->ino_lock);
- kmem_cache_free(ino_entry_slab, e);
radix_tree_preload_end();
goto retry;
}
@@ -368,6 +360,9 @@ retry:
}
spin_unlock(&im->ino_lock);
radix_tree_preload_end();
+
+ if (e != tmp)
+ kmem_cache_free(ino_entry_slab, tmp);
}
static void __remove_ino_entry(struct f2fs_sb_info *sbi, nid_t ino, int type)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 6641017..ece5e70 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1252,13 +1252,10 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep,
gfp_t flags)
{
void *entry;
-retry:
- entry = kmem_cache_alloc(cachep, flags);
- if (!entry) {
- cond_resched();
- goto retry;
- }
+ entry = kmem_cache_alloc(cachep, flags);
+ if (!entry)
+ entry = kmem_cache_alloc(cachep, flags | __GFP_NOFAIL);
return entry;
}
@@ -1267,12 +1264,9 @@ static inline struct bio *f2fs_bio_alloc(int npages)
struct bio *bio;
/* No failure on bio allocation */
-retry:
bio = bio_alloc(GFP_NOIO, npages);
- if (!bio) {
- cond_resched();
- goto retry;
- }
+ if (!bio)
+ bio = bio_alloc(GFP_NOIO | __GFP_NOFAIL, npages);
return bio;
}
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 6bef5a2..777066d 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -159,7 +159,7 @@ static void __set_nat_cache_dirty(struct f2fs_nm_info *nm_i,
head = radix_tree_lookup(&nm_i->nat_set_root, set);
if (!head) {
- head = f2fs_kmem_cache_alloc(nat_entry_set_slab, GFP_ATOMIC);
+ head = f2fs_kmem_cache_alloc(nat_entry_set_slab, GFP_NOFS);
INIT_LIST_HEAD(&head->entry_list);
INIT_LIST_HEAD(&head->set_list);
@@ -246,7 +246,7 @@ static struct nat_entry *grab_nat_entry(struct f2fs_nm_info *nm_i, nid_t nid)
{
struct nat_entry *new;
- new = f2fs_kmem_cache_alloc(nat_entry_slab, GFP_ATOMIC);
+ new = f2fs_kmem_cache_alloc(nat_entry_slab, GFP_NOFS);
f2fs_radix_tree_insert(&nm_i->nat_root, nid, new);
memset(new, 0, sizeof(struct nat_entry));
nat_set_nid(new, nid);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 6273e2c..78e6d06 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -1753,7 +1753,7 @@ static struct page *get_next_sit_page(struct f2fs_sb_info *sbi,
static struct sit_entry_set *grab_sit_entry_set(void)
{
struct sit_entry_set *ses =
- f2fs_kmem_cache_alloc(sit_entry_set_slab, GFP_ATOMIC);
+ f2fs_kmem_cache_alloc(sit_entry_set_slab, GFP_NOFS);
ses->entry_cnt = 0;
INIT_LIST_HEAD(&ses->set_list);
--
2.1.1
Hi Jaegeuk,
> -----Original Message-----
> From: Jaegeuk Kim [mailto:[email protected]]
> Sent: Monday, August 24, 2015 12:54 PM
> To: Chao Yu
> Cc: [email protected]; [email protected];
> [email protected]
> Subject: Re: [f2fs-dev] [PATCH 1/2] f2fs: handle failed bio allocation
>
> Hi Chao,
>
> [snip]
>
> > > > >
> > > > > - /* No failure on bio allocation */
> > > > > - bio = bio_alloc(GFP_NOIO, npages);
> > > >
> > > > How about using __GFP_NOFAIL flag to avoid failing in bio_alloc instead
> > > > of adding opencode endless loop in code?
> > > >
> > > > We can see the reason in this commit 647757197cd3
> > > > ("mm: clarify __GFP_NOFAIL deprecation status ")
> > > >
> > > > "__GFP_NOFAIL is documented as a deprecated flag since commit
> > > > 478352e789f5 ("mm: add comment about deprecation of __GFP_NOFAIL").
> > > >
> > > > This has discouraged people from using it but in some cases an opencoded
> > > > endless loop around allocator has been used instead. So the allocator
> > > > is not aware of the de facto __GFP_NOFAIL allocation because this
> > > > information was not communicated properly.
> > > >
> > > > Let's make clear that if the allocation context really cannot afford
> > > > failure because there is no good failure policy then using __GFP_NOFAIL
> > > > is preferable to opencoding the loop outside of the allocator."
> > > >
> > > > BTW, I found that f2fs_kmem_cache_alloc also could be replaced, we could
> > > > fix them together.
> > >
> > > Agreed. I think that can be another patch like this.
> > >
> > > From 1579e0d1ada96994c4ec6619fb5b5d9386e77ab3 Mon Sep 17 00:00:00 2001
> > > From: Jaegeuk Kim <[email protected]>
> > > Date: Thu, 20 Aug 2015 08:51:56 -0700
> > > Subject: [PATCH] f2fs: use __GFP_NOFAIL to avoid infinite loop
> > >
> > > __GFP_NOFAIL can avoid retrying the whole path of kmem_cache_alloc and
> > > bio_alloc.
> > >
> > > Suggested-by: Chao Yu <[email protected]>
> > > Signed-off-by: Jaegeuk Kim <[email protected]>
> > > ---
> > > fs/f2fs/f2fs.h | 16 +++++-----------
> > > 1 file changed, 5 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index 00591f7..c78b599 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -1244,13 +1244,10 @@ static inline void *f2fs_kmem_cache_alloc(struct kmem_cache *cachep,
> > > gfp_t flags)
> > > {
> > > void *entry;
> > > -retry:
> > > - entry = kmem_cache_alloc(cachep, flags);
> > > - if (!entry) {
> > > - cond_resched();
> > > - goto retry;
> > > - }
> > >
> > > + entry = kmem_cache_alloc(cachep, flags);
> > > + if (!entry)
> > > + entry = kmem_cache_alloc(cachep, flags | __GFP_NOFAIL);
> >
> > The fast + slow path model looks good to me, expect one thing:
> > In several paths of checkpoint, caller will grab slab cache with GFP_ATOMIC,
> > so in slow path, our flags will be GFP_ATOMIC | __GFP_NOFAIL, I'm not sure
> > that the two flags can be used together.
> >
> > Should we replace GFP_ATOMIC with GFP_NOFS in flags if caller passed
> > GFP_ATOMIC?
>
> Indeed, we need to avoid GFP_ATOMIC as much as possible to mitigate memory
> pressure at this moment. Too much abused.
>
> I wrote a patch like this.
>
> From a9209556d024cdce490695586ecee3164efda49c Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <[email protected]>
> Date: Thu, 20 Aug 2015 08:51:56 -0700
> Subject: [PATCH] f2fs: use __GFP_NOFAIL to avoid infinite loop
>
> __GFP_NOFAIL can avoid retrying the whole path of kmem_cache_alloc and
> bio_alloc.
> And, it also fixes the use cases of GFP_ATOMIC correctly.
Looks good to me!
>
> Suggested-by: Chao Yu <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,