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

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

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

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 <yuchao0@huawei.com>
---
 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(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index bf040b5..817cda7 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -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)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 38ce5d6..8faeada 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -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);
 }
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 0786a45..cf646b3 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -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 */

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 19a85cf..8b25ac1 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -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 */
-- 
2.7.2

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

[Jaegeuk Kim]

Hi Chao,

On Wed, Apr 27, 2016 at 09:41:48PM +0800, Chao Yu wrote:
> From: Chao Yu <yuchao0@huawei.com>
> 
> 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

Well, io_schedule_timeout should work for this?

Thanks,

> 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 <yuchao0@huawei.com>
> ---
>  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(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index bf040b5..817cda7 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -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)
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 38ce5d6..8faeada 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -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);
>  }
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 0786a45..cf646b3 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -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 */
> 
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 19a85cf..8b25ac1 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -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 */
> -- 
> 2.7.2

Re: [f2fs-dev] [PATCH RESEND 2/2] f2fs: disable preemption when waiting on all pages writeback

[Chao Yu]

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

On 2016/4/28 2:09, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On Wed, Apr 27, 2016 at 09:41:48PM +0800, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> 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
> 
> Well, io_schedule_timeout should work for this?
> 
> Thanks,
> 
>> 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 <yuchao0@huawei.com>
>> ---
>>  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(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index bf040b5..817cda7 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -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)
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 38ce5d6..8faeada 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -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);
>>  }
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 0786a45..cf646b3 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -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 */
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index 19a85cf..8b25ac1 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -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 */
>> -- 
>> 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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> .
> 

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

[Chao Yu]

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

On 2016/4/28 2:09, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On Wed, Apr 27, 2016 at 09:41:48PM +0800, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> 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
> 
> Well, io_schedule_timeout should work for this?
> 
> Thanks,
> 
>> 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 <yuchao0@huawei.com>
>> ---
>>  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(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index bf040b5..817cda7 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -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)
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>> index 38ce5d6..8faeada 100644
>> --- a/fs/f2fs/data.c
>> +++ b/fs/f2fs/data.c
>> @@ -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);
>>  }
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 0786a45..cf646b3 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -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 */
>>
>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
>> index 19a85cf..8b25ac1 100644
>> --- a/fs/f2fs/super.c
>> +++ b/fs/f2fs/super.c
>> @@ -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 */
>> -- 
>> 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
> Linux-f2fs-devel@lists.sourceforge.net
> 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

Re: [f2fs-dev] [PATCH RESEND 2/2] f2fs: disable preemption when waiting on all pages writeback

[Peter Zijlstra]

On Thu, Apr 28, 2016 at 07:51:04PM +0800, Chao Yu wrote:
> > On Wed, Apr 27, 2016 at 09:41:48PM +0800, Chao Yu wrote:
> >> From: Chao Yu <yuchao0@huawei.com>
> >>
> >> 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.

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.

> >>     - io_schedule_timeout
> > 

Re: [f2fs-dev] [PATCH RESEND 2/2] f2fs: disable preemption when waiting on all pages writeback

[Chao Yu]

On 2016/4/28 22:03, Peter Zijlstra wrote:
> On Thu, Apr 28, 2016 at 07:51:04PM +0800, Chao Yu wrote:
>>> On Wed, Apr 27, 2016 at 09:41:48PM +0800, Chao Yu wrote:
>>>> From: Chao Yu <yuchao0@huawei.com>
>>>>
>>>> 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.
> 
> 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?

> 
>>>>     - io_schedule_timeout
>>>

Re: [f2fs-dev] [PATCH RESEND 2/2] f2fs: disable preemption when waiting on all pages writeback

[Peter Zijlstra]

On Thu, Apr 28, 2016 at 10:30:55PM +0800, Chao Yu wrote:
> On 2016/4/28 22:03, Peter Zijlstra wrote:
> > On Thu, Apr 28, 2016 at 07:51:04PM +0800, Chao Yu wrote:
> >>> On Wed, Apr 27, 2016 at 09:41:48PM +0800, Chao Yu wrote:
> >>>> From: Chao Yu <yuchao0@huawei.com>
> >>>>
> >>>> 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.
> > 
> > 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.

Re: [f2fs-dev] [PATCH RESEND 2/2] f2fs: disable preemption when waiting on all pages writeback

[Chao Yu]

On 2016/4/28 22:37, Peter Zijlstra wrote:
> On Thu, Apr 28, 2016 at 10:30:55PM +0800, Chao Yu wrote:
>> On 2016/4/28 22:03, Peter Zijlstra wrote:
>>> On Thu, Apr 28, 2016 at 07:51:04PM +0800, Chao Yu wrote:
>>>>> On Wed, Apr 27, 2016 at 09:41:48PM +0800, Chao Yu wrote:
>>>>>> From: Chao Yu <yuchao0@huawei.com>
>>>>>>
>>>>>> 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.
>>>
>>> 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! :)

> 

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

[Chao Yu]

On 2016/4/28 22:37, Peter Zijlstra wrote:
> On Thu, Apr 28, 2016 at 10:30:55PM +0800, Chao Yu wrote:
>> On 2016/4/28 22:03, Peter Zijlstra wrote:
>>> On Thu, Apr 28, 2016 at 07:51:04PM +0800, Chao Yu wrote:
>>>>> On Wed, Apr 27, 2016 at 09:41:48PM +0800, Chao Yu wrote:
>>>>>> From: Chao Yu <yuchao0@huawei.com>
>>>>>>
>>>>>> 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.
>>>
>>> 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! :)

> 

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