* [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
@ 2014-09-03 9:16 Gu Zheng
2014-09-05 7:07 ` [f2fs-dev] " Chao Yu
0 siblings, 1 reply; 5+ messages in thread
From: Gu Zheng @ 2014-09-03 9:16 UTC (permalink / raw)
To: jaegeuk; +Cc: Gu Zheng, linux-kernel, linux-f2fs-devel
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.
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;
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) {
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/
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
2014-09-03 9:16 [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management Gu Zheng
@ 2014-09-05 7:07 ` Chao Yu
0 siblings, 0 replies; 5+ 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
------------------------------------------------------------------------------
Slashdot TV.
Video for Nerds. Stuff that matters.
http://tv.slashdot.org/
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
@ 2014-09-05 7:07 ` Chao Yu
0 siblings, 0 replies; 5+ 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] 5+ messages in thread* Re: [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
2014-09-05 7:07 ` [f2fs-dev] " Chao Yu
@ 2014-09-05 10:17 ` Gu Zheng
-1 siblings, 0 replies; 5+ 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
>
> .
>
------------------------------------------------------------------------------
Slashdot TV.
Video for Nerds. Stuff that matters.
http://tv.slashdot.org/
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management
@ 2014-09-05 10:17 ` Gu Zheng
0 siblings, 0 replies; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2014-09-05 10:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-03 9:16 [PATCH] f2fs: use lock-less list(llist) to simplify the flush cmd management Gu Zheng
2014-09-05 7:07 ` Chao Yu
2014-09-05 7:07 ` [f2fs-dev] " Chao Yu
2014-09-05 10:17 ` Gu Zheng
2014-09-05 10:17 ` [f2fs-dev] " Gu Zheng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.