| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <54098D9E.3070400@cn.fujitsu.com>
Date: Fri, 5 Sep 2014 18:17:02 +0800
From: Gu Zheng <guz.fnst@...fujitsu.com>
To: Chao Yu <chao2.yu@...sung.com>
CC: <jaegeuk@...nel.org>, <linux-kernel@...r.kernel.org>,
<linux-f2fs-devel@...ts.sourceforge.net>
Subject: Re: [f2fs-dev] [PATCH] f2fs: use lock-less list(llist) to simplify
the flush cmd management
Hi Yu,
On 09/05/2014 03:07 PM, Chao Yu wrote:
> Hi Gu,
>
>> -----Original Message-----
>> From: Gu Zheng [mailto:guz.fnst@...fujitsu.com]
>> Sent: Wednesday, September 03, 2014 5:16 PM
>> To: jaegeuk@...nel.org
>> Cc: Gu Zheng; linux-kernel@...r.kernel.org; linux-f2fs-devel@...ts.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@...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@...ts.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
> .
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists