[Ocfs2-devel] [PATCH 5/5] Fix files when an inode is written in file_fix
Gang He
ghe at suse.com
Wed Jun 1 20:06:17 PDT 2016
>>>
>
> On 05/31/2016 09:05 PM, Gang He wrote:
>> Hello Goldwyn,
>>
>>
>>>>>
>>
>>>
>>> On 05/30/2016 04:45 AM, Gang He wrote:
>>>> Hello Goldwyn,
>>>>
>>>> Please see my comments inline.
>>>>
>>>>
>>>> Thanks
>>>> Gang
>>>>
>>>>
>>>>>>>
>>>>> From: Goldwyn Rodrigues <rgoldwyn at suse.com>
>>>>>
>>>>> Check that the entriy exists and has been filed for check.
>>>>> Also perform some code cleanup
>>>>>
>>>>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.com>
>>>>> ---
>>>>> fs/ocfs2/filecheck.c | 41 +++++++++++++++++++++++++----------------
>>>>> fs/ocfs2/filecheck.h | 1 +
>>>>> fs/ocfs2/sysfs.c | 2 +-
>>>>> 3 files changed, 27 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/fs/ocfs2/filecheck.c b/fs/ocfs2/filecheck.c
>>>>> index 006d521..fc6e183 100644
>>>>> --- a/fs/ocfs2/filecheck.c
>>>>> +++ b/fs/ocfs2/filecheck.c
>>>>> @@ -198,22 +198,6 @@ ocfs2_filecheck_handle(struct ocfs2_super *osb,
>>>>> return ret;
>>>>> }
>>>>>
>>>>> -static void
>>>>> -ocfs2_filecheck_handle_entry(struct ocfs2_super *osb,
>>>>> - struct ocfs2_filecheck_entry *entry)
>>>>> -{
>>>>> - if (entry->fe_type == OCFS2_FILECHECK_TYPE_CHK)
>>>>> - entry->fe_status = ocfs2_filecheck_handle(osb,
>>>>> - entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_CHK);
>>>>> - else if (entry->fe_type == OCFS2_FILECHECK_TYPE_FIX)
>>>>> - entry->fe_status = ocfs2_filecheck_handle(osb,
>>>>> - entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
>>>>> - else
>>>>> - entry->fe_status = OCFS2_FILECHECK_ERR_UNSUPPORTED;
>>>>> -
>>>>> - ocfs2_filecheck_done_entry(osb, entry);
>>>>> -}
>>>>> -
>>>>> int ocfs2_filecheck_add_inode(struct ocfs2_super *osb,
>>>>> unsigned long ino)
>>>>> {
>>>>> @@ -268,3 +252,28 @@ unlock:
>>>>> ocfs2_filecheck_done_entry(osb, entry);
>>>>> return 0;
>>>>> }
>>>>> +
>>>>> +int ocfs2_filefix_add_inode(struct ocfs2_super *osb,
>>>>> + unsigned long ino)
>>>>> +{
>>>>> + struct ocfs2_filecheck_entry *entry;
>>>>> + int ret = -ENOENT;
>>>>> +
>>>>> + spin_lock(&osb->fc_lock);
>>>>> + list_for_each_entry(entry, &osb->file_check_entries, fe_list)
>>>>> + if (entry->fe_ino == ino) {
>>>>> + entry->fe_type = OCFS2_FILECHECK_TYPE_FIX;
>>>> Gang: It looks that we can not do it directly, why? because the entry
>>> pointer can be freed by the function ocfs2_filecheck_erase_entries().
>>>> We can not use the same entry pointer within two user processes.
>>>> The simple solution is to return -EBUSY error in case there is the same ino
>>> number entry in the list, then the user can try again after the previous
> user
>>> process is returned.
>>>
>>> How? is it not protected under spinlock?
>> Gang: please aware that this spinlock fc_lock is used to protect entry list
> (adding entry/deleting entry), not protect entry itself.
>> The first user process will mark the entry's status to done after the file
> check operation is finished, then the function
> ocfs2_filecheck_erase_entries() will possibly delete this entry from the
> list, to free the entry memory during the second user process is referencing
> on this entry.
>> You know, the spinlock can not protect the file check operation (too long
> time IO operation).
>
> Yes, a possible reason for separating the lists too. The entries will be
> deleted after a check, and the user will not be able to perform a fix on it.
Gang: we can not delete the file check entry after it's operation is done, since we need to keep
them to provide the result history records.
>
> In any case, if we check on status, we can do away with it. We may
> require filecheck_entry spinlocks later on, but for now it is not required.
Gang: Why we need a spinlock for each filecheck entry? the entry is only occupied by one user process.
we only need to get the lock when adding/deleting the entry from the list, this operation is very short.
>
>
>>
>>> Anyways, I plan to make a separate list for this so we can do away with
>>> more macros.
>> Gang: you can use two lists, but I think that one list is also OK, keep the
> thing simple.
>> Just return a -EBUSY error when the same inode is on the progress, then the
> user can try again after that file check process returns.
>
> Thanks for the suggestions, I have incorporated that, with the two
> lists, of course.
>
>>
>>>
>>> Besides, any I/O and check operation should be done in a separate
>>> thread/work queue.
>> Gang: current design is no a separated thread is used to run file check
> operation, each user trigger process is used to execute its file check
> operation.
>> Why? first, keep the thing simple, avoid to involve more logic/thread.
> second, if we involved a thread/work queue to run the file check operations,
>> that means the user trigger process will return directly and need not to
> wait for the actual file check operation, there will be a risk that the user
> can
>> not see the result from the result history record (via cat check/fix), since
> the new submits result will make the oldest result records to be released,
>> we have a fixed length list to keep these status records. If we use the user
> trigger process to do the file check operation, the user can surely see the
> result after this user process returns (since this file check operation is
> done in the kernel space).
>>
>
> In the future, how do you plan to extend this? How would you check for
> extent_blocks? Or worse, system files? Would you keep the system waiting
> until all I/O has completed?
Gang: Yes, the trigger user process can wait for this I/O operation, just like a system call.
If we want to check multiple files (inode), we can use a batch to call this interface one by one
(of course, you can use multiple threads/processes), this is why I did not use a work queue/thread
in the kernel module to do this asynchronously, because in the user space, we can do the same thing.
let's keep the kernel module logic simple.
>
>>
>>>
>>>>
>>>>> + ret = 0;
>>>>> + break;
>>>>> + }
>>>>> + spin_unlock(&osb->fc_lock);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + entry->fe_status = ocfs2_filecheck_handle(osb,
>>>>> + entry->fe_ino, OCFS2_FI_FLAG_FILECHECK_FIX);
>>>>> +
>>>>> + ocfs2_filecheck_done_entry(osb, entry);
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> diff --git a/fs/ocfs2/filecheck.h b/fs/ocfs2/filecheck.h
>>>>> index b1a0d8c..d5f81a7 100644
>>>>> --- a/fs/ocfs2/filecheck.h
>>>>> +++ b/fs/ocfs2/filecheck.h
>>>>> @@ -53,6 +53,7 @@ int ocfs2_filecheck_create_sysfs(struct super_block *sb);
>>>>> int ocfs2_filecheck_remove_sysfs(struct super_block *sb);
>>>>> int ocfs2_filefix_inode(struct ocfs2_super *osb, unsigned long ino);
>>>>> int ocfs2_filecheck_add_inode(struct ocfs2_super *osb, unsigned long
> ino);
>>>>> +int ocfs2_filefix_add_inode(struct ocfs2_super *osb, unsigned long ino);
>>>>> int ocfs2_filecheck_set_max_entries(struct ocfs2_super *osb, int num);
>>>>> int ocfs2_filecheck_show(struct ocfs2_super *osb, unsigned int type, char
>>>>> *buf);
>>>>>
>>>>> diff --git a/fs/ocfs2/sysfs.c b/fs/ocfs2/sysfs.c
>>>>> index acc4834..ac149fb 100644
>>>>> --- a/fs/ocfs2/sysfs.c
>>>>> +++ b/fs/ocfs2/sysfs.c
>>>>> @@ -91,7 +91,7 @@ static ssize_t file_fix_store(struct ocfs2_super *osb,
>>>>> ret = kstrtoul(skip_spaces(buf), 0, &t);
>>>>> if (ret)
>>>>> return ret;
>>>>> - ret = ocfs2_filecheck_add_inode(osb, t);
>>>>> + ret = ocfs2_filefix_add_inode(osb, t);
>>>>> if (ret)
>>>>> return ret;
>>>>> return count;
>>>>> --
>>>>> 2.6.6
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Ocfs2-devel mailing list
>>>>> Ocfs2-devel at oss.oracle.com
>>>>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>>
>>> --
>>> Goldwyn
>
> --
> Goldwyn
More information about the Ocfs2-devel
mailing list