Re: [PATCH 13/25] ubifs: introduce a field named as budgeted to ubifs_inode

Dongsheng Yang <yangds.fnst@xxxxxxxxxxxxxx> · Wed, 22 Jul 2015 14:22:36 +0800

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@xxxxxxxxxxxxxx>
---
  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
.


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html






