| Message ID | 1438235311-23788-20-git-send-email-yangds.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 30.07.2015 um 07:48 schrieb Dongsheng Yang: > In ubifs, we have to do a budget for inode before marking > it as dirty. But sometimes, we would call dirty_inode in vfs > which will not do a budget for inode. In this case, we have > to do a budget in ubifs_dirty_inode() by ourselvies. How is this commit related to quota support? > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > --- > fs/ubifs/super.c | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c > index 2491fff..bc57685 100644 > --- a/fs/ubifs/super.c > +++ b/fs/ubifs/super.c > @@ -383,15 +383,38 @@ done: > clear_inode(inode); > } > > +/* > + * In theory, ubifs should take the full control of dirty<->clean > + * of an inode with ui->ui_mutex. But there are callers of > + * ubifs_dirty_inode in vfs without holding ui->ui_mutex and > + * budgeting. So when we found the ui_mutex is not locked, we have > + * to lock ui->ui_mutex by itself and do a budget by itself. > + */ > static void ubifs_dirty_inode(struct inode *inode, int flags) > { > struct ubifs_inode *ui = ubifs_inode(inode); > + int locked = mutex_is_locked(&ui->ui_mutex); > + struct ubifs_info *c = inode->i_sb->s_fs_info; > + int ret = 0; > + > + if (!locked) > + mutex_lock(&ui->ui_mutex); > > - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); So, currently this assert can be reached? How? 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
On Thu, 2015-07-30 at 13:48 +0800, Dongsheng Yang wrote: > In ubifs, we have to do a budget for inode before marking > it as dirty. But sometimes, we would call dirty_inode in vfs > which will not do a budget for inode. In this case, we have > to do a budget in ubifs_dirty_inode() by ourselvies. > > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> Could you please explain some more the problem you are trying to solve. Locking looks confusing and broken. It looks like what you are expressing is that the 'ui_mutex' is optional, and this smells fishy. > static void ubifs_dirty_inode(struct inode *inode, int flags) > { > struct ubifs_inode *ui = ubifs_inode(inode); > + int locked = mutex_is_locked(&ui->ui_mutex); Suppose another process has it locked, so 'locked' is set to 1 here. > + struct ubifs_info *c = inode->i_sb->s_fs_info; > + int ret = 0; > + > + if (!locked) So we skip this. > + mutex_lock(&ui->ui_mutex); And if the other process has released the lock by this time, we do not mind, right? Therefore, ui_mutex is "optional"? > - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); > if (!ui->dirty) { > + if (!locked) { And similar here, we do not run this code because 'locked' is 1. > + struct ubifs_budget_req req = { .dirtied_ino > = 1, > + .dirtied_ino_d = ALIGN(ui > ->data_len, 8) }; > + ret = ubifs_budget_space(c, &req); > + if (ret) > + goto out; > + } But the other process has already released it, and we do not mind? Please, try to explain what you want to achieve some more. I am not sure I understand the end goal. Artem. -- 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
On 08/05/2015 04:11 PM, Artem Bityutskiy wrote: > On Thu, 2015-07-30 at 13:48 +0800, Dongsheng Yang wrote: >> In ubifs, we have to do a budget for inode before marking >> it as dirty. But sometimes, we would call dirty_inode in vfs >> which will not do a budget for inode. In this case, we have >> to do a budget in ubifs_dirty_inode() by ourselvies. >> >> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > > Could you please explain some more the problem you are trying to solve. > Locking looks confusing and broken. It looks like what you are > expressing is that the 'ui_mutex' is optional, and this smells fishy. Oh, yes, that's TRUE. This patch makes the locking broken. I am sorry about it. > [...] > > Please, try to explain what you want to achieve some more. I am not > sure I understand the end goal. Okey, what I want here is to doing a budget for the inode in .dirty_inode called by vfs. Currently, the all work is under the full control of ubifs as the comment of @ui_mutex said. But the dquot_disable() is doing a dirty_inode() without asking ubifs is that allowed. So I want to do the budget in ubifs_dirty_inode() itself here. But, that's INCORRECT. Yes, my bad. Thanx for your comment. And I found another solution for it. To introduce a callback in quota to allow filesystem to dirty inode in dquot_disable(). I believe that works. Anyway, I agree that this patch is a fishy one here. I will drop it in next version and send a better solution for it. Of course, with some more description for what I am doing. :) Thanx a lot, Richard and Atem Yang > > Artem. > . > -- 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
On Thu, 2015-08-06 at 14:46 +0800, Dongsheng Yang wrote: > On 08/05/2015 04:11 PM, Artem Bityutskiy wrote: > > On Thu, 2015-07-30 at 13:48 +0800, Dongsheng Yang wrote: > > > In ubifs, we have to do a budget for inode before marking > > > it as dirty. But sometimes, we would call dirty_inode in vfs > > > which will not do a budget for inode. In this case, we have > > > to do a budget in ubifs_dirty_inode() by ourselvies. > > > > > > Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> > > > > Could you please explain some more the problem you are trying to > > solve. > > Locking looks confusing and broken. It looks like what you are > > expressing is that the 'ui_mutex' is optional, and this smells > > fishy. > > Oh, yes, that's TRUE. This patch makes the locking broken. I am sorry > about it. > > > [...] > > > > Please, try to explain what you want to achieve some more. I am not > > sure I understand the end goal. > > Okey, what I want here is to doing a budget for the inode in > .dirty_inode called by vfs. Currently, the all work is under the full > control of ubifs as the comment of @ui_mutex said. But the > dquot_disable() is doing a dirty_inode() without asking ubifs is that > allowed. So I want to do the budget in ubifs_dirty_inode() itself > here.But, that's INCORRECT. Yes, my bad. Thanx for your comment. > > And I found another solution for it. To introduce a callback in quota > to allow filesystem to dirty inode in dquot_disable(). I believe that > works. Yes, the high-level picture is this. 1. Dirtying and inode implies liability - the system becomes liable to write it to the media. 2. Most file-systems have no problems with this, because there is always space allocated for the inode. 3. Some file-systems like UBIFS may have no space for writing dirty inodes. Often UBIFS can product this space, but it will involve forcing write-back, and there are locking issues when doing write-back from the write-back path. 4. So the approach UBIFS took is to allocate space in advance for every dirty inode. 5. To do that, UBIFS requires VFS avoid dirtying inodes directly, but instead, dirty them via the file-system. The file-system then has a chance to do all the necessary things related to dirtying the inode. The FS may even refuse to dirty the inode if in knows that it is impossible to do (the FS is 100% full, for example). So yes, if the generic quota code could dirty its inodes via the FS, not directly, that would be great. But please, check that the generic quota code can handle errors, because ubifs_dirty_inode() may return -ENOSPC or other errors. Artem. -- 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
On 08/06/2015 03:26 PM, Artem Bityutskiy wrote: > On Thu, 2015-08-06 at 14:46 +0800, Dongsheng Yang wrote: >> On 08/05/2015 04:11 PM, Artem Bityutskiy wrote: >>> On Thu, 2015-07-30 at 13:48 +0800, Dongsheng Yang wrote: >>>> In ubifs, we have to do a budget for inode before marking >>>> it as dirty. But sometimes, we would call dirty_inode in vfs >>>> which will not do a budget for inode. In this case, we have >>>> to do a budget in ubifs_dirty_inode() by ourselvies. >>>> >>>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >>> >>> Could you please explain some more the problem you are trying to >>> solve. >>> Locking looks confusing and broken. It looks like what you are >>> expressing is that the 'ui_mutex' is optional, and this smells >>> fishy. >> >> Oh, yes, that's TRUE. This patch makes the locking broken. I am sorry >> about it. >>> >> [...] >>> >>> Please, try to explain what you want to achieve some more. I am not >>> sure I understand the end goal. >> >> Okey, what I want here is to doing a budget for the inode in >> .dirty_inode called by vfs. Currently, the all work is under the full >> control of ubifs as the comment of @ui_mutex said. But the >> dquot_disable() is doing a dirty_inode() without asking ubifs is that >> allowed. So I want to do the budget in ubifs_dirty_inode() itself >> here.But, that's INCORRECT. Yes, my bad. Thanx for your comment. >> >> And I found another solution for it. To introduce a callback in quota >> to allow filesystem to dirty inode in dquot_disable(). I believe that >> works. > > Yes, the high-level picture is this. > > 1. Dirtying and inode implies liability - the system becomes liable to > write it to the media. > > 2. Most file-systems have no problems with this, because there is > always space allocated for the inode. > > 3. Some file-systems like UBIFS may have no space for writing dirty > inodes. Often UBIFS can product this space, but it will involve forcing > write-back, and there are locking issues when doing write-back from the > write-back path. > > 4. So the approach UBIFS took is to allocate space in advance for every > dirty inode. > > 5. To do that, UBIFS requires VFS avoid dirtying inodes directly, but > instead, dirty them via the file-system. The file-system then has a > chance to do all the necessary things related to dirtying the inode. > The FS may even refuse to dirty the inode if in knows that it is > impossible to do (the FS is 100% full, for example). Hi Atem, Thanx for your detail. Yes, I figured this out by reading code and comments in last days. Maybe I should have asked it to you earlier. :) > > So yes, if the generic quota code could dirty its inodes via the FS, > not directly, that would be great. > > But please, check that the generic quota code can handle errors, > because ubifs_dirty_inode() may return -ENOSPC or other errors. Yes, of course. I will do that. And It would also be helpful to other FS, such as btrfs which works in cow. Thanx Yang > > Artem. > . > -- 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
Hi Richard, Thanx for your review. I decide to drop it in next version. :) Thanx Yang On 08/04/2015 05:13 AM, Richard Weinberger wrote: > Am 30.07.2015 um 07:48 schrieb Dongsheng Yang: >> In ubifs, we have to do a budget for inode before marking >> it as dirty. But sometimes, we would call dirty_inode in vfs >> which will not do a budget for inode. In this case, we have >> to do a budget in ubifs_dirty_inode() by ourselvies. > > How is this commit related to quota support? > >> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> >> --- >> fs/ubifs/super.c | 25 ++++++++++++++++++++++++- >> 1 file changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c >> index 2491fff..bc57685 100644 >> --- a/fs/ubifs/super.c >> +++ b/fs/ubifs/super.c >> @@ -383,15 +383,38 @@ done: >> clear_inode(inode); >> } >> >> +/* >> + * In theory, ubifs should take the full control of dirty<->clean >> + * of an inode with ui->ui_mutex. But there are callers of >> + * ubifs_dirty_inode in vfs without holding ui->ui_mutex and >> + * budgeting. So when we found the ui_mutex is not locked, we have >> + * to lock ui->ui_mutex by itself and do a budget by itself. >> + */ >> static void ubifs_dirty_inode(struct inode *inode, int flags) >> { >> struct ubifs_inode *ui = ubifs_inode(inode); >> + int locked = mutex_is_locked(&ui->ui_mutex); >> + struct ubifs_info *c = inode->i_sb->s_fs_info; >> + int ret = 0; >> + >> + if (!locked) >> + mutex_lock(&ui->ui_mutex); >> >> - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); > > So, currently this assert can be reached? How? > > 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
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c index 2491fff..bc57685 100644 --- a/fs/ubifs/super.c +++ b/fs/ubifs/super.c @@ -383,15 +383,38 @@ done: clear_inode(inode); } +/* + * In theory, ubifs should take the full control of dirty<->clean + * of an inode with ui->ui_mutex. But there are callers of + * ubifs_dirty_inode in vfs without holding ui->ui_mutex and + * budgeting. So when we found the ui_mutex is not locked, we have + * to lock ui->ui_mutex by itself and do a budget by itself. + */ static void ubifs_dirty_inode(struct inode *inode, int flags) { struct ubifs_inode *ui = ubifs_inode(inode); + int locked = mutex_is_locked(&ui->ui_mutex); + struct ubifs_info *c = inode->i_sb->s_fs_info; + int ret = 0; + + if (!locked) + mutex_lock(&ui->ui_mutex); - ubifs_assert(mutex_is_locked(&ui->ui_mutex)); if (!ui->dirty) { + if (!locked) { + struct ubifs_budget_req req = { .dirtied_ino = 1, + .dirtied_ino_d = ALIGN(ui->data_len, 8) }; + ret = ubifs_budget_space(c, &req); + if (ret) + goto out; + } ui->dirty = 1; dbg_gen("inode %lu", inode->i_ino); } +out: + if (!locked) + mutex_unlock(&ui->ui_mutex); + return; } static int ubifs_statfs(struct dentry *dentry, struct kstatfs *buf)
In ubifs, we have to do a budget for inode before marking it as dirty. But sometimes, we would call dirty_inode in vfs which will not do a budget for inode. In this case, we have to do a budget in ubifs_dirty_inode() by ourselvies. Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com> --- fs/ubifs/super.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)