linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* RE: [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
       [not found] <1409735782-7831-1-git-send-email-guz.fnst@cn.fujitsu.com>
@ 2014-09-05  7:07 ` Chao Yu
  2014-09-05 10:17   ` Gu Zheng
  0 siblings, 1 reply; 2+ messages in thread
From: Chao Yu @ 2014-09-05  7:07 UTC (permalink / raw)
  To: 'Gu Zheng', jaegeuk; +Cc: linux-kernel, linux-f2fs-devel

Hi Gu,

> -----Original Message-----
> From: Gu Zheng [mailto:guz.fnst@cn.fujitsu.com]
> Sent: Wednesday, September 03, 2014 5:16 PM
> To: jaegeuk@kernel.org
> Cc: Gu Zheng; linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
> Subject: [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
> 
> We use flush cmd control to collect many flush cmds, and flush them
> together. In this case, we use two list to manage the flush cmds
> (collect and dispatch), and one spin lock is used to protect this.
> In fact, the lock-less list(llist) is very suitable to this case,
> and we use simplify this routine.

Good catch for finding this scenario with llist!

> 
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  fs/f2fs/f2fs.h    |    7 +++----
>  fs/f2fs/segment.c |   31 +++++++++----------------------
>  2 files changed, 12 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index e921242..1dd861c 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -334,16 +334,15 @@ enum {
>  struct flush_cmd {
>  	struct flush_cmd *next;

No needed here, we can remove it.

>  	struct completion wait;
> +	struct llist_node llnode;
>  	int ret;
>  };
> 
>  struct flush_cmd_control {
>  	struct task_struct *f2fs_issue_flush;	/* flush thread */
>  	wait_queue_head_t flush_wait_queue;	/* waiting queue for wake-up */
> -	struct flush_cmd *issue_list;		/* list for command issue */
> -	struct flush_cmd *dispatch_list;	/* list for command dispatch */
> -	spinlock_t issue_lock;			/* for issue list lock */
> -	struct flush_cmd *issue_tail;		/* list tail of issue list */
> +	struct llist_head issue_list;		/* list for command issue */
> +	struct llist_node *dispatch_list;	/* list for command dispatch */
>  };
> 
>  struct f2fs_sm_info {
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 0aa337c..2db1f95 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -205,24 +205,19 @@ repeat:
>  	if (kthread_should_stop())
>  		return 0;
> 
> -	spin_lock(&fcc->issue_lock);
> -	if (fcc->issue_list) {
> -		fcc->dispatch_list = fcc->issue_list;
> -		fcc->issue_list = fcc->issue_tail = NULL;
> -	}
> -	spin_unlock(&fcc->issue_lock);
> -
> -	if (fcc->dispatch_list) {
> +	if (!llist_empty(&fcc->issue_list)) {
>  		struct bio *bio = bio_alloc(GFP_NOIO, 0);
> -		struct flush_cmd *cmd, *next;
> +		struct flush_cmd *cmd;
>  		int ret;
> 
> +		fcc->dispatch_list = llist_del_all(&fcc->issue_list);
> +		fcc->dispatch_list = llist_reverse_order(fcc->dispatch_list);
> +
>  		bio->bi_bdev = sbi->sb->s_bdev;
>  		ret = submit_bio_wait(WRITE_FLUSH, bio);
> 
> -		for (cmd = fcc->dispatch_list; cmd; cmd = next) {
> +		llist_for_each_entry(cmd, fcc->dispatch_list, llnode) {

Please use llist_for_each_entry_safe, Otherwise we might encounter oops as below:

[18727.381582] Call Trace:
[18727.381613]  [<c107c0bf>] __wake_up_locked+0x1f/0x30
[18727.381626]  [<c107c886>] complete+0x36/0x50
[18727.381645]  [<f0a01313>] issue_flush_thread+0x93/0x140 [f2fs]
[18727.381658]  [<c107c530>] ? add_wait_queue+0x50/0x50
[18727.381672]  [<f0a01280>] ? add_sit_entry+0xc0/0xc0 [f2fs]
[18727.381687]  [<c105fabb>] kthread+0x9b/0xb0
[18727.381701]  [<c159ad81>] ret_from_kernel_thread+0x21/0x30
[18727.381714]  [<c105fa20>] ? flush_kthread_worker+0x80/0x80

Thanks,
Yu

>  			cmd->ret = ret;
> -			next = cmd->next;
>  			complete(&cmd->wait);
>  		}
>  		bio_put(bio);
> @@ -230,7 +225,7 @@ repeat:
>  	}
> 
>  	wait_event_interruptible(*q,
> -			kthread_should_stop() || fcc->issue_list);
> +		kthread_should_stop() || !llist_empty(&fcc->issue_list));
>  	goto repeat;
>  }
> 
> @@ -249,17 +244,10 @@ int f2fs_issue_flush(struct f2fs_sb_info *sbi)
>  		return blkdev_issue_flush(sbi->sb->s_bdev, GFP_KERNEL, NULL);
> 
>  	init_completion(&cmd.wait);
> -	cmd.next = NULL;
> 
> -	spin_lock(&fcc->issue_lock);
> -	if (fcc->issue_list)
> -		fcc->issue_tail->next = &cmd;
> -	else
> -		fcc->issue_list = &cmd;
> -	fcc->issue_tail = &cmd;
> -	spin_unlock(&fcc->issue_lock);
> +	llist_add(&cmd.llnode, &fcc->issue_list);
> 
> -	if (!fcc->dispatch_list)
> +	if (fcc->dispatch_list == NULL)
>  		wake_up(&fcc->flush_wait_queue);
> 
>  	wait_for_completion(&cmd.wait);
> @@ -276,7 +264,6 @@ int create_flush_cmd_control(struct f2fs_sb_info *sbi)
>  	fcc = kzalloc(sizeof(struct flush_cmd_control), GFP_KERNEL);
>  	if (!fcc)
>  		return -ENOMEM;
> -	spin_lock_init(&fcc->issue_lock);
>  	init_waitqueue_head(&fcc->flush_wait_queue);
>  	SM_I(sbi)->cmd_control_info = fcc;
>  	fcc->f2fs_issue_flush = kthread_run(issue_flush_thread, sbi,
> --
> 1.7.7
> 
> 
> ------------------------------------------------------------------------------
> Slashdot TV.
> Video for Nerds.  Stuff that matters.
> http://tv.slashdot.org/
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
  2014-09-05  7:07 ` [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management Chao Yu
@ 2014-09-05 10:17   ` Gu Zheng
  0 siblings, 0 replies; 2+ messages in thread
From: Gu Zheng @ 2014-09-05 10:17 UTC (permalink / raw)
  To: Chao Yu; +Cc: jaegeuk, linux-kernel, linux-f2fs-devel

Hi Yu,

On 09/05/2014 03:07 PM, Chao Yu wrote:

> Hi Gu,
> 
>> -----Original Message-----
>> From: Gu Zheng [mailto:guz.fnst@cn.fujitsu.com]
>> Sent: Wednesday, September 03, 2014 5:16 PM
>> To: jaegeuk@kernel.org
>> Cc: Gu Zheng; linux-kernel@vger.kernel.org; linux-f2fs-devel@lists.sourceforge.net
>> Subject: [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
>>
>> We use flush cmd control to collect many flush cmds, and flush them
>> together. In this case, we use two list to manage the flush cmds
>> (collect and dispatch), and one spin lock is used to protect this.
>> In fact, the lock-less list(llist) is very suitable to this case,
>> and we use simplify this routine.
> 
> Good catch for finding this scenario with llist!
> 
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  fs/f2fs/f2fs.h    |    7 +++----
>>  fs/f2fs/segment.c |   31 +++++++++----------------------
>>  2 files changed, 12 insertions(+), 26 deletions(-)
>>
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index e921242..1dd861c 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -334,16 +334,15 @@ enum {
>>  struct flush_cmd {
>>  	struct flush_cmd *next;
> 
> No needed here, we can remove it.
> 
>>  	struct completion wait;
>> +	struct llist_node llnode;
>>  	int ret;
>>  };
>>
>>  struct flush_cmd_control {
>>  	struct task_struct *f2fs_issue_flush;	/* flush thread */
>>  	wait_queue_head_t flush_wait_queue;	/* waiting queue for wake-up */
>> -	struct flush_cmd *issue_list;		/* list for command issue */
>> -	struct flush_cmd *dispatch_list;	/* list for command dispatch */
>> -	spinlock_t issue_lock;			/* for issue list lock */
>> -	struct flush_cmd *issue_tail;		/* list tail of issue list */
>> +	struct llist_head issue_list;		/* list for command issue */
>> +	struct llist_node *dispatch_list;	/* list for command dispatch */
>>  };
>>
>>  struct f2fs_sm_info {
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 0aa337c..2db1f95 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -205,24 +205,19 @@ repeat:
>>  	if (kthread_should_stop())
>>  		return 0;
>>
>> -	spin_lock(&fcc->issue_lock);
>> -	if (fcc->issue_list) {
>> -		fcc->dispatch_list = fcc->issue_list;
>> -		fcc->issue_list = fcc->issue_tail = NULL;
>> -	}
>> -	spin_unlock(&fcc->issue_lock);
>> -
>> -	if (fcc->dispatch_list) {
>> +	if (!llist_empty(&fcc->issue_list)) {
>>  		struct bio *bio = bio_alloc(GFP_NOIO, 0);
>> -		struct flush_cmd *cmd, *next;
>> +		struct flush_cmd *cmd;
>>  		int ret;
>>
>> +		fcc->dispatch_list = llist_del_all(&fcc->issue_list);
>> +		fcc->dispatch_list = llist_reverse_order(fcc->dispatch_list);
>> +
>>  		bio->bi_bdev = sbi->sb->s_bdev;
>>  		ret = submit_bio_wait(WRITE_FLUSH, bio);
>>
>> -		for (cmd = fcc->dispatch_list; cmd; cmd = next) {
>> +		llist_for_each_entry(cmd, fcc->dispatch_list, llnode) {
> 
> Please use llist_for_each_entry_safe, Otherwise we might encounter oops as below:
> 
> [18727.381582] Call Trace:
> [18727.381613]  [<c107c0bf>] __wake_up_locked+0x1f/0x30
> [18727.381626]  [<c107c886>] complete+0x36/0x50
> [18727.381645]  [<f0a01313>] issue_flush_thread+0x93/0x140 [f2fs]
> [18727.381658]  [<c107c530>] ? add_wait_queue+0x50/0x50
> [18727.381672]  [<f0a01280>] ? add_sit_entry+0xc0/0xc0 [f2fs]
> [18727.381687]  [<c105fabb>] kthread+0x9b/0xb0
> [18727.381701]  [<c159ad81>] ret_from_kernel_thread+0x21/0x30
> [18727.381714]  [<c105fa20>] ? flush_kthread_worker+0x80/0x80

Thanks very much. Fixed all in the new version.

Regards,
Gu

> 
> Thanks,
> Yu
> 
>>  			cmd->ret = ret;
>> -			next = cmd->next;
>>  			complete(&cmd->wait);
>>  		}
>>  		bio_put(bio);
>> @@ -230,7 +225,7 @@ repeat:
>>  	}
>>
>>  	wait_event_interruptible(*q,
>> -			kthread_should_stop() || fcc->issue_list);
>> +		kthread_should_stop() || !llist_empty(&fcc->issue_list));
>>  	goto repeat;
>>  }
>>
>> @@ -249,17 +244,10 @@ int f2fs_issue_flush(struct f2fs_sb_info *sbi)
>>  		return blkdev_issue_flush(sbi->sb->s_bdev, GFP_KERNEL, NULL);
>>
>>  	init_completion(&cmd.wait);
>> -	cmd.next = NULL;
>>
>> -	spin_lock(&fcc->issue_lock);
>> -	if (fcc->issue_list)
>> -		fcc->issue_tail->next = &cmd;
>> -	else
>> -		fcc->issue_list = &cmd;
>> -	fcc->issue_tail = &cmd;
>> -	spin_unlock(&fcc->issue_lock);
>> +	llist_add(&cmd.llnode, &fcc->issue_list);
>>
>> -	if (!fcc->dispatch_list)
>> +	if (fcc->dispatch_list == NULL)
>>  		wake_up(&fcc->flush_wait_queue);
>>
>>  	wait_for_completion(&cmd.wait);
>> @@ -276,7 +264,6 @@ int create_flush_cmd_control(struct f2fs_sb_info *sbi)
>>  	fcc = kzalloc(sizeof(struct flush_cmd_control), GFP_KERNEL);
>>  	if (!fcc)
>>  		return -ENOMEM;
>> -	spin_lock_init(&fcc->issue_lock);
>>  	init_waitqueue_head(&fcc->flush_wait_queue);
>>  	SM_I(sbi)->cmd_control_info = fcc;
>>  	fcc->f2fs_issue_flush = kthread_run(issue_flush_thread, sbi,
>> --
>> 1.7.7
>>
>>
>> ------------------------------------------------------------------------------
>> Slashdot TV.
>> Video for Nerds.  Stuff that matters.
>> http://tv.slashdot.org/
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
> 
> .
> 



^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2014-09-05 10:30 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1409735782-7831-1-git-send-email-guz.fnst@cn.fujitsu.com>
2014-09-05  7:07 ` [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management Chao Yu
2014-09-05 10:17   ` Gu Zheng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).