[PATCH 13/25] ubifs: introduce a field named as budgeted to ubifs_inode
Dongsheng Yang
yangds.fnst at cn.fujitsu.com
Tue Jul 21 23:22:36 PDT 2015
On 07/22/2015 08:56 AM, Dongsheng Yang wrote:
> On 07/22/2015 04:47 AM, Richard Weinberger wrote:
>> Am 21.07.2015 um 10:37 schrieb Dongsheng Yang:
>>> There is a long-term pain in ubifs, we have to introduce a
>>> ui_mutex in ubifs_inode to solve two problems below:
>>>
>>> 1: make some process atomic, such as ubifs_rename.
>>> 2: make sure we budget space for inode before dirting it.
>>>
>>> About 1, it's true and we have to do it.
>>>
>>> About 2, we can do it better.
>>> There is a ubifs_assert(mutex_is_locked(&ui->mutex)) in
>>> ubifs_dirty_inode(). But there is probably some processes
>>> are very long, and we can not make all of it into a pair of
>>> lock/unlock ui->mutex.
>>>
>>> E.g: dquot_disable().
>>> It would mark the quota files as dirty and write the
>>> inode back. We can do a budget before this function, but we
>>> can not make the whole dquot_disable() into mutex_lock/mutex_unlock.
>>> Because we need to lock the ui_mutex in dquot_disable().
>>>
>>> So, this commit introduce a ui->budgeted to allow us to make
>>> budgeting and dirting in two different lock windows.
>>>
>>> Result:
>>> ubifs_budget_space()
>>> mutex_lock();
>>> ui->budgeted = 1;
>>> mutex_unlock();
>>> ...
>>> dquot_disable();
On my second thought of it, I have to find out another solution
for it. My patch here can not solve it and would cause some
other problem.
Sorry for the noise.
Yang
>>
>> I'm confused by this changelog.
>
> Sorry, it is indeed confusing. Let me try to explain it more.
>
> Currently, we have a ubifs_assert(mutex_is_locked(&ui->ui_mutex));
> in ubifs_dirty_inode(). The reason of it is to make sure we have
> done the budget before dirting it.
>
> So we have to use it like that:
> struct ubifs_budget_req req = { .dirtied_ino = 1};
> ubifs_budget_space(req);
> mutex_lock(&ui->ui_mutex);
> ubifs_dirty_inode();
> mutex_unlock(&ui->ui_mutex);
> We are checking the ui_mutex in ubifs_dirty_inode() to make sure
> we are taking a full control of this process and we are sure there
> is enough space to write this inode.
>
>
> But the problem is: we can not put all process which are going to
> dirty inode into the lock window, such as dquot_disable(), it will
> acquire the ui_mutex in itself. (Although we can blame vfs to dirty
> inode without asking ubifs is that correct)
>
> So, I introduce a ui->budgeted here to replace ubifs_assert() in
> ubifs_dirty_inode(); In ubifs_dirty_inode(), we can only check
> the ui->budgeted to know whether we have done the budget for this
> inode. Take the advantage of it, we can get what we want and solve
> the problem I mentioned above.
>
> I hope it more clear.
>
> Thanx
> Yang
>>
>> Why do you need ui->budgeted? You set it but never read it
>> except in an ubifs_assert().
>>
>>> Signed-off-by: Dongsheng Yang <yangds.fnst at cn.fujitsu.com>
>>> ---
>>> fs/ubifs/file.c | 7 +++++++
>>> fs/ubifs/ioctl.c | 1 +
>>> fs/ubifs/super.c | 14 +++++++++++++-
>>> fs/ubifs/ubifs.h | 1 +
>>> 4 files changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>>> index dc8bf0b..113c3a6 100644
>>> --- a/fs/ubifs/file.c
>>> +++ b/fs/ubifs/file.c
>>> @@ -307,6 +307,7 @@ static int write_begin_slow(struct address_space
>>> *mapping,
>>> * budget we allocated.
>>> */
>>> ubifs_release_dirty_inode_budget(c, ui);
>>> + ui->budgeted = 1;
>>> }
>>>
>>> *pagep = page;
>>> @@ -357,6 +358,7 @@ static int allocate_budget(struct ubifs_info *c,
>>> struct page *page,
>>> * we need to budget the inode change.
>>> */
>>> req.dirtied_ino = 1;
>>> + ui->budgeted = 1;
>>> } else {
>>> if (PageChecked(page))
>>> /*
>>> @@ -384,6 +386,7 @@ static int allocate_budget(struct ubifs_info *c,
>>> struct page *page,
>>> * needs a budget.
>>> */
>>> req.dirtied_ino = 1;
>>> + ui->budgeted = 1;
>>> }
>>> }
>>>
>>> @@ -1237,6 +1240,7 @@ static int do_setattr(struct ubifs_info *c,
>>> struct inode *inode,
>>> do_attr_changes(inode, attr);
>>>
>>> release = ui->dirty;
>>> + ui->budgeted = 1;
>>> if (attr->ia_valid & ATTR_SIZE)
>>> /*
>>> * Inode length changed, so we have to make sure
>>> @@ -1397,6 +1401,7 @@ static int ubifs_update_time(struct inode
>>> *inode, struct timespec *time,
>>> iflags |= I_DIRTY_SYNC;
>>>
>>> release = ui->dirty;
>>> + ui->budgeted = 1;
>>> __mark_inode_dirty(inode, iflags);
>>> mutex_unlock(&ui->ui_mutex);
>>> if (release)
>>> @@ -1430,6 +1435,7 @@ static int update_mctime(struct inode *inode)
>>> mutex_lock(&ui->ui_mutex);
>>> inode->i_mtime = inode->i_ctime = ubifs_current_time(inode);
>>> release = ui->dirty;
>>> + ui->budgeted = 1;
>>> mark_inode_dirty_sync(inode);
>>> mutex_unlock(&ui->ui_mutex);
>>> if (release)
>>> @@ -1556,6 +1562,7 @@ static int ubifs_vm_page_mkwrite(struct
>>> vm_area_struct *vma,
>>> mutex_lock(&ui->ui_mutex);
>>> inode->i_mtime = inode->i_ctime = ubifs_current_time(inode);
>>> release = ui->dirty;
>>> + ui->budgeted = 1;
>>> mark_inode_dirty_sync(inode);
>>> mutex_unlock(&ui->ui_mutex);
>>> if (release)
>>> diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c
>>> index 3c7b29d..f015b81 100644
>>> --- a/fs/ubifs/ioctl.c
>>> +++ b/fs/ubifs/ioctl.c
>>> @@ -128,6 +128,7 @@ static int setflags(struct inode *inode, int flags)
>>> ubifs_set_inode_flags(inode);
>>> inode->i_ctime = ubifs_current_time(inode);
>>> release = ui->dirty;
>>> + ui->budgeted = 1;
>>> mark_inode_dirty_sync(inode);
>>> mutex_unlock(&ui->ui_mutex);
>>>
>>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>>> index 2491fff..5fa21d6 100644
>>> --- a/fs/ubifs/super.c
>>> +++ b/fs/ubifs/super.c
>>> @@ -331,6 +331,7 @@ static int ubifs_write_inode(struct inode *inode,
>>> struct writeback_control *wbc)
>>> }
>>>
>>> ui->dirty = 0;
>>> + ui->budgeted = 0;
>>> mutex_unlock(&ui->ui_mutex);
>>> ubifs_release_dirty_inode_budget(c, ui);
>>> return err;
>>> @@ -386,12 +387,23 @@ done:
>>> static void ubifs_dirty_inode(struct inode *inode, int flags)
>>> {
>>> struct ubifs_inode *ui = ubifs_inode(inode);
>>> + int need_unlock = 0;
>>>
>>> - ubifs_assert(mutex_is_locked(&ui->ui_mutex));
>>> + if (unlikely(!mutex_is_locked(&ui->ui_mutex))) {
>>> + /* We need to lock ui_mutex to access ui->budgeted */
>>> + mutex_lock(&ui->ui_mutex);
>>> + need_unlock = 1;
>>> + }
>>> +
>>> + /* Check the budget for this inode */
>>> + ubifs_assert(ui->budgeted);
>>> if (!ui->dirty) {
>>> ui->dirty = 1;
>>> dbg_gen("inode %lu", inode->i_ino);
>>> }
>>> +
>>> + if (need_unlock)
>>> + mutex_unlock(&ui->ui_mutex);
>>> }
>>>
>>> static int ubifs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>>> index 9754bb6..28392a6 100644
>>> --- a/fs/ubifs/ubifs.h
>>> +++ b/fs/ubifs/ubifs.h
>>> @@ -410,6 +410,7 @@ struct ubifs_inode {
>>> unsigned int xattr_cnt;
>>> unsigned int xattr_names;
>>> unsigned int dirty:1;
>>> + unsigned int budgeted:1;
>>
>> Please document that new flag too.
>>
>> Thanks,
>> //richard
>> .
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> .
>
More information about the linux-mtd
mailing list