* [Ocfs2-devel] [patch -next] ocfs2: scheduling in atomic in ocfs2_filecheck_store()
@ 2015-07-27 21:04 ` Andrew Morton
0 siblings, 0 replies; 8+ messages in thread
From: Andrew Morton @ 2015-07-27 21:04 UTC (permalink / raw)
To: ocfs2-devel
On Mon, 27 Jul 2015 11:27:23 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote:
> We're hold "spin_lock(&ent->fs_fcheck->fc_lock)" so the allocation has
> to be GFP_ATOMIC.
>
> I changed the sizeof() because otherwise the line goes over the 80
> character limit and also the new way is prefered kernel style.
>
> Fixes: e467fe5da718 ('ocfs2: sysfile interfaces for online file check')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
> index 3332af1..9613663 100644
> --- a/fs/ocfs2/filecheck.c
> +++ b/fs/ocfs2/filecheck.c
> @@ -544,7 +544,7 @@ static ssize_t ocfs2_filecheck_store(struct kobject *kobj,
> BUG_ON(!ocfs2_filecheck_erase_entry(ent));
> }
>
> - entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
> + entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
> if (entry) {
> entry->fe_ino = args.fa_ino;
> entry->fe_type = args.fa_type;
GFP_NOFS is more reliable than GFP_ATOMIC, so we should retain that if
possible. And it's pretty easy to do here.
Gang, please carefully review and test?
--- a/fs/ocfs2/filecheck.c~ocfs2-sysfile-interfaces-for-online-file-check-fix-2
+++ a/fs/ocfs2/filecheck.c
@@ -501,7 +501,7 @@ static ssize_t ocfs2_filecheck_store(str
const char *buf, size_t count)
{
struct ocfs2_filecheck_args args;
- struct ocfs2_filecheck_entry *entry = NULL;
+ struct ocfs2_filecheck_entry *entry;
struct ocfs2_filecheck_sysfs_entry *ent;
ssize_t ret = 0;
@@ -527,12 +527,19 @@ static ssize_t ocfs2_filecheck_store(str
return (!ret ? count : ret);
}
+ entry = kmalloc(sizeof(*entry), GFP_NOFS);
+ if (!entry) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
spin_lock(&ent->fs_fcheck->fc_lock);
if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
- (ent->fs_fcheck->fc_done == 0)) {
- mlog(ML_ERROR,
- "Online file check queue(%u) is full\n",
- ent->fs_fcheck->fc_max);
+ (ent->fs_fcheck->fc_done == 0)) {
+ mlog(ML_ERROR, "Online file check queue(%u) is full\n",
+ ent->fs_fcheck->fc_max);
+ kfree(entry);
+ entry = NULL;
ret = -EBUSY;
} else {
if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
@@ -544,26 +551,21 @@ static ssize_t ocfs2_filecheck_store(str
BUG_ON(!ocfs2_filecheck_erase_entry(ent));
}
- entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
- if (entry) {
- entry->fe_ino = args.fa_ino;
- entry->fe_type = args.fa_type;
- entry->fe_done = 0;
- entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
- list_add_tail(&entry->fe_list,
- &ent->fs_fcheck->fc_head);
-
- ent->fs_fcheck->fc_size++;
- ret = count;
- } else {
- ret = -ENOMEM;
- }
+ entry->fe_ino = args.fa_ino;
+ entry->fe_type = args.fa_type;
+ entry->fe_done = 0;
+ entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
+ list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
+
+ ent->fs_fcheck->fc_size++;
+ ret = count;
}
spin_unlock(&ent->fs_fcheck->fc_lock);
if (entry)
ocfs2_filecheck_handle_entry(ent, entry);
+out:
ocfs2_filecheck_sysfs_put(ent);
return ret;
}
_
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [patch -next] ocfs2: scheduling in atomic in ocfs2_filecheck_store()
2015-07-27 21:04 ` [Ocfs2-devel] " Andrew Morton
@ 2015-07-27 21:19 ` Dan Carpenter
-1 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2015-07-27 21:19 UTC (permalink / raw)
To: ocfs2-devel
Thanks, Andrew. Looks good.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [patch -next] ocfs2: scheduling in atomic in ocfs2_filecheck_store()
2015-07-27 21:04 ` [Ocfs2-devel] " Andrew Morton
@ 2015-07-28 3:15 ` Gang He
-1 siblings, 0 replies; 8+ messages in thread
From: Gang He @ 2015-07-28 3:15 UTC (permalink / raw)
To: ocfs2-devel
Hi Andrew and Dan,
Thank for your efforts.
It is better than before.
Thanks a lot.
Gang
>>>
> On Mon, 27 Jul 2015 11:27:23 +0300 Dan Carpenter <dan.carpenter@oracle.com>
> wrote:
>
>> We're hold "spin_lock(&ent->fs_fcheck->fc_lock)" so the allocation has
>> to be GFP_ATOMIC.
>>
>> I changed the sizeof() because otherwise the line goes over the 80
>> character limit and also the new way is prefered kernel style.
>>
>> Fixes: e467fe5da718 ('ocfs2: sysfile interfaces for online file check')
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
>> index 3332af1..9613663 100644
>> --- a/fs/ocfs2/filecheck.c
>> +++ b/fs/ocfs2/filecheck.c
>> @@ -544,7 +544,7 @@ static ssize_t ocfs2_filecheck_store(struct kobject
> *kobj,
>> BUG_ON(!ocfs2_filecheck_erase_entry(ent));
>> }
>>
>> - entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
>> + entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
>> if (entry) {
>> entry->fe_ino = args.fa_ino;
>> entry->fe_type = args.fa_type;
>
> GFP_NOFS is more reliable than GFP_ATOMIC, so we should retain that if
> possible. And it's pretty easy to do here.
>
> Gang, please carefully review and test?
>
>
> --- a/fs/ocfs2/filecheck.c~ocfs2-sysfile-interfaces-for-online-file-check-fix-2
> +++ a/fs/ocfs2/filecheck.c
> @@ -501,7 +501,7 @@ static ssize_t ocfs2_filecheck_store(str
> const char *buf, size_t count)
> {
> struct ocfs2_filecheck_args args;
> - struct ocfs2_filecheck_entry *entry = NULL;
> + struct ocfs2_filecheck_entry *entry;
> struct ocfs2_filecheck_sysfs_entry *ent;
> ssize_t ret = 0;
>
> @@ -527,12 +527,19 @@ static ssize_t ocfs2_filecheck_store(str
> return (!ret ? count : ret);
> }
>
> + entry = kmalloc(sizeof(*entry), GFP_NOFS);
> + if (!entry) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> spin_lock(&ent->fs_fcheck->fc_lock);
> if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
> - (ent->fs_fcheck->fc_done = 0)) {
> - mlog(ML_ERROR,
> - "Online file check queue(%u) is full\n",
> - ent->fs_fcheck->fc_max);
> + (ent->fs_fcheck->fc_done = 0)) {
> + mlog(ML_ERROR, "Online file check queue(%u) is full\n",
> + ent->fs_fcheck->fc_max);
> + kfree(entry);
> + entry = NULL;
> ret = -EBUSY;
> } else {
> if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
> @@ -544,26 +551,21 @@ static ssize_t ocfs2_filecheck_store(str
> BUG_ON(!ocfs2_filecheck_erase_entry(ent));
> }
>
> - entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
> - if (entry) {
> - entry->fe_ino = args.fa_ino;
> - entry->fe_type = args.fa_type;
> - entry->fe_done = 0;
> - entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
> - list_add_tail(&entry->fe_list,
> - &ent->fs_fcheck->fc_head);
> -
> - ent->fs_fcheck->fc_size++;
> - ret = count;
> - } else {
> - ret = -ENOMEM;
> - }
> + entry->fe_ino = args.fa_ino;
> + entry->fe_type = args.fa_type;
> + entry->fe_done = 0;
> + entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
> + list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
> +
> + ent->fs_fcheck->fc_size++;
> + ret = count;
> }
> spin_unlock(&ent->fs_fcheck->fc_lock);
>
> if (entry)
> ocfs2_filecheck_handle_entry(ent, entry);
>
> +out:
> ocfs2_filecheck_sysfs_put(ent);
> return ret;
> }
> _
^ permalink raw reply [flat|nested] 8+ messages in thread* [Ocfs2-devel] [patch -next] ocfs2: scheduling in atomic in ocfs2_filecheck_store()
@ 2015-07-28 3:15 ` Gang He
0 siblings, 0 replies; 8+ messages in thread
From: Gang He @ 2015-07-28 3:15 UTC (permalink / raw)
To: ocfs2-devel
Hi Andrew and Dan,
Thank for your efforts.
It is better than before.
Thanks a lot.
Gang
>>>
> On Mon, 27 Jul 2015 11:27:23 +0300 Dan Carpenter <dan.carpenter@oracle.com>
> wrote:
>
>> We're hold "spin_lock(&ent->fs_fcheck->fc_lock)" so the allocation has
>> to be GFP_ATOMIC.
>>
>> I changed the sizeof() because otherwise the line goes over the 80
>> character limit and also the new way is prefered kernel style.
>>
>> Fixes: e467fe5da718 ('ocfs2: sysfile interfaces for online file check')
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
>> index 3332af1..9613663 100644
>> --- a/fs/ocfs2/filecheck.c
>> +++ b/fs/ocfs2/filecheck.c
>> @@ -544,7 +544,7 @@ static ssize_t ocfs2_filecheck_store(struct kobject
> *kobj,
>> BUG_ON(!ocfs2_filecheck_erase_entry(ent));
>> }
>>
>> - entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
>> + entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
>> if (entry) {
>> entry->fe_ino = args.fa_ino;
>> entry->fe_type = args.fa_type;
>
> GFP_NOFS is more reliable than GFP_ATOMIC, so we should retain that if
> possible. And it's pretty easy to do here.
>
> Gang, please carefully review and test?
>
>
> --- a/fs/ocfs2/filecheck.c~ocfs2-sysfile-interfaces-for-online-file-check-fix-2
> +++ a/fs/ocfs2/filecheck.c
> @@ -501,7 +501,7 @@ static ssize_t ocfs2_filecheck_store(str
> const char *buf, size_t count)
> {
> struct ocfs2_filecheck_args args;
> - struct ocfs2_filecheck_entry *entry = NULL;
> + struct ocfs2_filecheck_entry *entry;
> struct ocfs2_filecheck_sysfs_entry *ent;
> ssize_t ret = 0;
>
> @@ -527,12 +527,19 @@ static ssize_t ocfs2_filecheck_store(str
> return (!ret ? count : ret);
> }
>
> + entry = kmalloc(sizeof(*entry), GFP_NOFS);
> + if (!entry) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> spin_lock(&ent->fs_fcheck->fc_lock);
> if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
> - (ent->fs_fcheck->fc_done == 0)) {
> - mlog(ML_ERROR,
> - "Online file check queue(%u) is full\n",
> - ent->fs_fcheck->fc_max);
> + (ent->fs_fcheck->fc_done == 0)) {
> + mlog(ML_ERROR, "Online file check queue(%u) is full\n",
> + ent->fs_fcheck->fc_max);
> + kfree(entry);
> + entry = NULL;
> ret = -EBUSY;
> } else {
> if ((ent->fs_fcheck->fc_size >= ent->fs_fcheck->fc_max) &&
> @@ -544,26 +551,21 @@ static ssize_t ocfs2_filecheck_store(str
> BUG_ON(!ocfs2_filecheck_erase_entry(ent));
> }
>
> - entry = kmalloc(sizeof(struct ocfs2_filecheck_entry), GFP_NOFS);
> - if (entry) {
> - entry->fe_ino = args.fa_ino;
> - entry->fe_type = args.fa_type;
> - entry->fe_done = 0;
> - entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
> - list_add_tail(&entry->fe_list,
> - &ent->fs_fcheck->fc_head);
> -
> - ent->fs_fcheck->fc_size++;
> - ret = count;
> - } else {
> - ret = -ENOMEM;
> - }
> + entry->fe_ino = args.fa_ino;
> + entry->fe_type = args.fa_type;
> + entry->fe_done = 0;
> + entry->fe_status = OCFS2_FILECHECK_ERR_INPROGRESS;
> + list_add_tail(&entry->fe_list, &ent->fs_fcheck->fc_head);
> +
> + ent->fs_fcheck->fc_size++;
> + ret = count;
> }
> spin_unlock(&ent->fs_fcheck->fc_lock);
>
> if (entry)
> ocfs2_filecheck_handle_entry(ent, entry);
>
> +out:
> ocfs2_filecheck_sysfs_put(ent);
> return ret;
> }
> _
^ permalink raw reply [flat|nested] 8+ messages in thread