Skip to contentSkip to conversation

f2fs: disable preemption when waiting on all pages writeback

PATCH RESEND In review Chao Yu 4 participants 7 messages Apr 27, 2016 – Apr 28, 2016 F2FS file system

lore
Share & tools
lore

Chao Yu

author maintainer
#0 lore raw

PATCH RESEND 2/2 f2fs: disable preemption when waiting on all pages writeback

From: Chao Yu <[email protected]>

The following condition can happen in a preemptible kernel, it may cause checkpointer hunging.

CPU0: CPU1:
- write_checkpoint
- do_checkpoint
- wait_on_all_pages_writeback
- f2fs_write_end_io
- wake_up
this is last writebacked page, but no sleeper in sbi->cp_wait wait queue, wake_up is not been called.
- prepare_to_wait(TASK_UNINTERRUPTIBLE)
Here, current task can been preempted,
but there will be no waker since last
write_end_io has bypassed wake_up. So
current task will sleep forever.
- io_schedule_timeout

Now we use spinlock to create a critical section to guarantee preemption was disabled during racing in between wait_on_all_pages_writeback and f2fs_write_end_io, so that above condition can be avoided.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/checkpoint.c | 14 +++++++++-----
 fs/f2fs/data.c       |  9 +++++++--
 fs/f2fs/f2fs.h       |  3 ++-
 fs/f2fs/super.c      |  1 +
 4 files changed, 19 insertions(+), 8 deletions(-)
fs/f2fs/checkpoint.c +0 -0
index bf040b5..817cda7 100644
@@ -914,15 +914,19 @@ static void wait_on_all_pages_writeback(struct

f2fs_sb_info *sbi)
{
DEFINE_WAIT(wait);

- for (;;) {
+ spin_lock(&sbi->cp_wb_lock);
+
+ while (get_pages(sbi, F2FS_WRITEBACK)) {
prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);

- if (!get_pages(sbi, F2FS_WRITEBACK))
- break;
+ spin_unlock(&sbi->cp_wb_lock);
+ io_schedule();
+ spin_lock(&sbi->cp_wb_lock);

- io_schedule_timeout(5*HZ);
+ finish_wait(&sbi->cp_wait, &wait);
}
- finish_wait(&sbi->cp_wait, &wait);
+
+ spin_unlock(&sbi->cp_wb_lock);
}

static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)

fs/f2fs/data.c +7 -2
index 38ce5d6..8faeada 100644
@@ -59,6 +59,7 @@ static void f2fs_write_end_io(struct bio *bio)
 {
 	struct f2fs_sb_info *sbi = bio->bi_private;
 	struct bio_vec *bvec;
+	unsigned long flags;
 	int i;
 
 	bio_for_each_segment_all(bvec, bio, i) {

@@ -74,8 +75,12 @@ static void f2fs_write_end_io(struct bio *bio)
 		dec_page_count(sbi, F2FS_WRITEBACK);
 	}
 
-	if (!get_pages(sbi, F2FS_WRITEBACK) && wq_has_sleeper(&sbi->cp_wait))
-		wake_up(&sbi->cp_wait);
+	if (!get_pages(sbi, F2FS_WRITEBACK)) {
+		spin_lock_irqsave(&sbi->cp_wb_lock, flags);
+		if (wq_has_sleeper(&sbi->cp_wait))
+			wake_up(&sbi->cp_wait);
+		spin_unlock_irqrestore(&sbi->cp_wb_lock, flags);
+	}
 
 	bio_put(bio);
 }
fs/f2fs/f2fs.h +2 -1
index 0786a45..cf646b3 100644
@@ -725,7 +725,8 @@ struct f2fs_sb_info {
 	struct rw_semaphore cp_rwsem;		/* blocking FS operations */
 	struct rw_semaphore node_write;		/* locking node writes */
 	struct mutex writepages;		/* mutex for writepages() */
-	wait_queue_head_t cp_wait;
+	wait_queue_head_t cp_wait;		/* for wait pages writeback */
+	spinlock_t cp_wb_lock;			/* for protect cp_wait */
 	unsigned long last_time[MAX_TIME];	/* to store time in jiffies */
 	long interval_time[MAX_TIME];		/* to store thresholds */
fs/f2fs/super.c +1 -0
index 19a85cf..8b25ac1 100644
@@ -1436,6 +1436,7 @@ try_onemore:
 
 	init_rwsem(&sbi->cp_rwsem);
 	init_waitqueue_head(&sbi->cp_wait);
+	spin_lock_init(&sbi->cp_wb_lock);
 	init_sb_info(sbi);
 
 	/* get an inode for meta space */
Signature · 1 line
2.7.2
maintainer
#1 lore raw

Hi Chao,

Quoted 5 lines · Chao Yu

From: Chao Yu <[email protected]>

The following condition can happen in a preemptible kernel, it may cause checkpointer hunging.

Quoted 15 lines

CPU0: CPU1:
- write_checkpoint
- do_checkpoint
- wait_on_all_pages_writeback
- f2fs_write_end_io
- wake_up
this is last writebacked page, but no sleeper in sbi->cp_wait wait queue, wake_up is not been called.
- prepare_to_wait(TASK_UNINTERRUPTIBLE)
Here, current task can been preempted,
but there will be no waker since last
write_end_io has bypassed wake_up. So
current task will sleep forever.
- io_schedule_timeout

Well, io_schedule_timeout should work for this?

Thanks,

Quoted 97 lines

Now we use spinlock to create a critical section to guarantee preemption was disabled during racing in between wait_on_all_pages_writeback and f2fs_write_end_io, so that above condition can be avoided.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/checkpoint.c | 14 +++++++++-----
 fs/f2fs/data.c       |  9 +++++++--
 fs/f2fs/f2fs.h       |  3 ++-
 fs/f2fs/super.c      |  1 +
 4 files changed, 19 insertions(+), 8 deletions(-)
fs/f2fs/checkpoint.c +9 -5
index bf040b5..817cda7 100644
@@ -914,15 +914,19 @@ static void wait_on_all_pages_writeback(struct
 f2fs_sb_info *sbi)
 {
 	DEFINE_WAIT(wait);
 
-	for (;;) {
+	spin_lock(&sbi->cp_wb_lock);
+
+	while (get_pages(sbi, F2FS_WRITEBACK)) {
 		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
 
-		if (!get_pages(sbi, F2FS_WRITEBACK))
-			break;
+		spin_unlock(&sbi->cp_wb_lock);
+		io_schedule();
+		spin_lock(&sbi->cp_wb_lock);
 
-		io_schedule_timeout(5*HZ);
+		finish_wait(&sbi->cp_wait, &wait);
 	}
-	finish_wait(&sbi->cp_wait, &wait);
+
+	spin_unlock(&sbi->cp_wb_lock);
 }

static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)

fs/f2fs/data.c +7 -2
index 38ce5d6..8faeada 100644
@@ -59,6 +59,7 @@ static void f2fs_write_end_io(struct bio *bio)
 {
 	struct f2fs_sb_info *sbi = bio->bi_private;
 	struct bio_vec *bvec;
+	unsigned long flags;
 	int i;
 
 	bio_for_each_segment_all(bvec, bio, i) {

@@ -74,8 +75,12 @@ static void f2fs_write_end_io(struct bio *bio)
 		dec_page_count(sbi, F2FS_WRITEBACK);
 	}
 
-	if (!get_pages(sbi, F2FS_WRITEBACK) && wq_has_sleeper(&sbi->cp_wait))
-		wake_up(&sbi->cp_wait);
+	if (!get_pages(sbi, F2FS_WRITEBACK)) {
+		spin_lock_irqsave(&sbi->cp_wb_lock, flags);
+		if (wq_has_sleeper(&sbi->cp_wait))
+			wake_up(&sbi->cp_wait);
+		spin_unlock_irqrestore(&sbi->cp_wb_lock, flags);
+	}
 
 	bio_put(bio);
 }
fs/f2fs/f2fs.h +2 -1
index 0786a45..cf646b3 100644
@@ -725,7 +725,8 @@ struct f2fs_sb_info {
 	struct rw_semaphore cp_rwsem;		/* blocking FS operations */
 	struct rw_semaphore node_write;		/* locking node writes */
 	struct mutex writepages;		/* mutex for writepages() */
-	wait_queue_head_t cp_wait;
+	wait_queue_head_t cp_wait;		/* for wait pages writeback */
+	spinlock_t cp_wb_lock;			/* for protect cp_wait */
 	unsigned long last_time[MAX_TIME];	/* to store time in jiffies */
 	long interval_time[MAX_TIME];		/* to store thresholds */
fs/f2fs/super.c +1 -0
index 19a85cf..8b25ac1 100644
@@ -1436,6 +1436,7 @@ try_onemore:
 
 	init_rwsem(&sbi->cp_rwsem);
 	init_waitqueue_head(&sbi->cp_wait);
+	spin_lock_init(&sbi->cp_wb_lock);
 	init_sb_info(sbi);
 
 	/* get an inode for meta space */
Signature · 1 line
2.7.2
#2 lore raw

+Cc Peter,

Hi Peter,

This patch should be RFC, I haven't saw such issue in real world though, I still worry that it may happen. Could you help to have a look at it.

This the question I'd like to ask here is that: in a preemptible kernel, if thread A add itself into wait queue with TASK_UNINTERRUPTIBLE status through prepare_to_wait, after that, thread B do the preemption, if there are no waker for thread A, will thread A sleep forever?

Thanks,

Quoted 122 lines · Jaegeuk Kim

Hi Chao,

Quoted 5 lines · Chao Yu

From: Chao Yu <[email protected]>

The following condition can happen in a preemptible kernel, it may cause checkpointer hunging.

Quoted 15 lines

CPU0: CPU1:
- write_checkpoint
- do_checkpoint
- wait_on_all_pages_writeback
- f2fs_write_end_io
- wake_up
this is last writebacked page, but no sleeper in sbi->cp_wait wait queue, wake_up is not been called.
- prepare_to_wait(TASK_UNINTERRUPTIBLE)
Here, current task can been preempted,
but there will be no waker since last
write_end_io has bypassed wake_up. So
current task will sleep forever.
- io_schedule_timeout

Well, io_schedule_timeout should work for this?

Thanks,

Quoted 94 lines

Now we use spinlock to create a critical section to guarantee preemption was disabled during racing in between wait_on_all_pages_writeback and f2fs_write_end_io, so that above condition can be avoided.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/checkpoint.c | 14 +++++++++-----
 fs/f2fs/data.c       |  9 +++++++--
 fs/f2fs/f2fs.h       |  3 ++-
 fs/f2fs/super.c      |  1 +
 4 files changed, 19 insertions(+), 8 deletions(-)
fs/f2fs/checkpoint.c +9 -5
index bf040b5..817cda7 100644
@@ -914,15 +914,19 @@ static void wait_on_all_pages_writeback(struct
 f2fs_sb_info *sbi)
 {
 	DEFINE_WAIT(wait);
 
-	for (;;) {
+	spin_lock(&sbi->cp_wb_lock);
+
+	while (get_pages(sbi, F2FS_WRITEBACK)) {
 		prepare_to_wait(&sbi->cp_wait, &wait, TASK_UNINTERRUPTIBLE);
 
-		if (!get_pages(sbi, F2FS_WRITEBACK))
-			break;
+		spin_unlock(&sbi->cp_wb_lock);
+		io_schedule();
+		spin_lock(&sbi->cp_wb_lock);
 
-		io_schedule_timeout(5*HZ);
+		finish_wait(&sbi->cp_wait, &wait);
 	}
-	finish_wait(&sbi->cp_wait, &wait);
+
+	spin_unlock(&sbi->cp_wb_lock);
 }

static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)

fs/f2fs/data.c +7 -2
index 38ce5d6..8faeada 100644
@@ -59,6 +59,7 @@ static void f2fs_write_end_io(struct bio *bio)
 {
 	struct f2fs_sb_info *sbi = bio->bi_private;
 	struct bio_vec *bvec;
+	unsigned long flags;
 	int i;
 
 	bio_for_each_segment_all(bvec, bio, i) {

@@ -74,8 +75,12 @@ static void f2fs_write_end_io(struct bio *bio)
 		dec_page_count(sbi, F2FS_WRITEBACK);
 	}
 
-	if (!get_pages(sbi, F2FS_WRITEBACK) && wq_has_sleeper(&sbi->cp_wait))
-		wake_up(&sbi->cp_wait);
+	if (!get_pages(sbi, F2FS_WRITEBACK)) {
+		spin_lock_irqsave(&sbi->cp_wb_lock, flags);
+		if (wq_has_sleeper(&sbi->cp_wait))
+			wake_up(&sbi->cp_wait);
+		spin_unlock_irqrestore(&sbi->cp_wb_lock, flags);
+	}
 
 	bio_put(bio);
 }
fs/f2fs/f2fs.h +2 -1
index 0786a45..cf646b3 100644
@@ -725,7 +725,8 @@ struct f2fs_sb_info {
 	struct rw_semaphore cp_rwsem;		/* blocking FS operations */
 	struct rw_semaphore node_write;		/* locking node writes */
 	struct mutex writepages;		/* mutex for writepages() */
-	wait_queue_head_t cp_wait;
+	wait_queue_head_t cp_wait;		/* for wait pages writeback */
+	spinlock_t cp_wb_lock;			/* for protect cp_wait */
 	unsigned long last_time[MAX_TIME];	/* to store time in jiffies */
 	long interval_time[MAX_TIME];		/* to store thresholds */
fs/f2fs/super.c +1 -0
index 19a85cf..8b25ac1 100644
@@ -1436,6 +1436,7 @@ try_onemore:
 
 	init_rwsem(&sbi->cp_rwsem);
 	init_waitqueue_head(&sbi->cp_wait);
+	spin_lock_init(&sbi->cp_wb_lock);
 	init_sb_info(sbi);
Quoted 16 lines
Quoted 3 lines
fs/f2fs/super.c +0 -0
 	/* get an inode for meta space */
Signature · 1 line
2.7.2

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
.

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z

#3 lore raw
Quoted 6 lines · Chao Yu
Quoted 6 lines · Chao Yu
Quoted 5 lines

From: Chao Yu <[email protected]>

The following condition can happen in a preemptible kernel, it may cause checkpointer hunging.

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines

CPU0: CPU1:
- write_checkpoint
- do_checkpoint
- wait_on_all_pages_writeback
- f2fs_write_end_io
- wake_up

Quoted 8 lines
Quoted 8 lines

this is last writebacked page, but no sleeper in sbi->cp_wait wait queue, wake_up is not been called.
- prepare_to_wait(TASK_UNINTERRUPTIBLE)
Here, current task can been preempted,
but there will be no waker since last
write_end_io has bypassed wake_up. So
current task will sleep forever.

But here, you should be verifying if you really should go sleep; as the code did; it tests for !get_pages(, F2FS_WRITEBACK), and if you've just completed that very last one, this will break out.

Quoted 2 lines
Quoted 2 lines
Quoted 1 line

- io_schedule_timeout

author maintainer
#4 lore raw
Quoted 22 lines · Peter Zijlstra
Quoted 6 lines · Chao Yu
Quoted 6 lines · Chao Yu
Quoted 5 lines

From: Chao Yu <[email protected]>

The following condition can happen in a preemptible kernel, it may cause checkpointer hunging.

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines

CPU0: CPU1:
- write_checkpoint
- do_checkpoint
- wait_on_all_pages_writeback
- f2fs_write_end_io
- wake_up

Quoted 8 lines
Quoted 8 lines

this is last writebacked page, but no sleeper in sbi->cp_wait wait queue, wake_up is not been called.
- prepare_to_wait(TASK_UNINTERRUPTIBLE)
Here, current task can been preempted,
but there will be no waker since last
write_end_io has bypassed wake_up. So
current task will sleep forever.

Quoted 3 lines

But here, you should be verifying if you really should go sleep; as the code did; it tests for !get_pages(, F2FS_WRITEBACK), and if you've just completed that very last one, this will break out.

You mean after being preempted with TASK_UNINTERRUPTIBLE status, that task still has chance to be scheduled to check '!get_pages(, F2FS_WRITEBACK)', is that right?

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines

- io_schedule_timeout

#5 lore raw
Quoted 23 lines · Chao Yu
Quoted 22 lines · Peter Zijlstra
Quoted 20 lines · Chao Yu
Quoted 20 lines · Chao Yu
Quoted 19 lines

From: Chao Yu <[email protected]>

The following condition can happen in a preemptible kernel, it may cause checkpointer hunging.

CPU0: CPU1:
- write_checkpoint
- do_checkpoint
- wait_on_all_pages_writeback
- f2fs_write_end_io
- wake_up
this is last writebacked page, but no sleeper in sbi->cp_wait wait queue, wake_up is not been called.
- prepare_to_wait(TASK_UNINTERRUPTIBLE)
Here, current task can been preempted,
but there will be no waker since last
write_end_io has bypassed wake_up. So
current task will sleep forever.

Quoted 6 lines
Quoted 3 lines

But here, you should be verifying if you really should go sleep; as the code did; it tests for !get_pages(, F2FS_WRITEBACK), and if you've just completed that very last one, this will break out.

You mean after being preempted with TASK_UNINTERRUPTIBLE status, that task still has chance to be scheduled to check '!get_pages(, F2FS_WRITEBACK)', is that right?

Yes, preemption ignores task_struct::state.

author maintainer
#6 lore raw
Quoted 24 lines · Peter Zijlstra
Quoted 23 lines · Chao Yu
Quoted 22 lines · Peter Zijlstra
Quoted 20 lines · Chao Yu
Quoted 20 lines · Chao Yu
Quoted 19 lines

From: Chao Yu <[email protected]>

The following condition can happen in a preemptible kernel, it may cause checkpointer hunging.

CPU0: CPU1:
- write_checkpoint
- do_checkpoint
- wait_on_all_pages_writeback
- f2fs_write_end_io
- wake_up
this is last writebacked page, but no sleeper in sbi->cp_wait wait queue, wake_up is not been called.
- prepare_to_wait(TASK_UNINTERRUPTIBLE)
Here, current task can been preempted,
but there will be no waker since last
write_end_io has bypassed wake_up. So
current task will sleep forever.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

But here, you should be verifying if you really should go sleep; as the code did; it tests for !get_pages(, F2FS_WRITEBACK), and if you've just completed that very last one, this will break out.

You mean after being preempted with TASK_UNINTERRUPTIBLE status, that task still has chance to be scheduled to check '!get_pages(, F2FS_WRITEBACK)', is that right?

Yes, preemption ignores task_struct::state.

Got it. Thanks very much for your help! :)

Quoted 1 line