From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dongsheng Yang Subject: Re: [PATCH 13/25] ubifs: introduce a field named as budgeted to ubifs_inode Date: Wed, 22 Jul 2015 14:22:36 +0800 Message-ID: <55AF36AC.9010409@cn.fujitsu.com> References: <1437467876-22106-1-git-send-email-yangds.fnst@cn.fujitsu.com> <1437467876-22106-14-git-send-email-yangds.fnst@cn.fujitsu.com> <55AEAFDA.3000008@nod.at> <55AEEA24.20203@cn.fujitsu.com> Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-15"; format=flowed Content-Transfer-Encoding: 7bit Cc: , To: Richard Weinberger , , Return-path: Received: from cn.fujitsu.com ([59.151.112.132]:8695 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1753138AbbGVG2G (ORCPT ); Wed, 22 Jul 2015 02:28:06 -0400 In-Reply-To: <55AEEA24.20203@cn.fujitsu.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 >>> --- >>> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > . >