[PATCH 0/3] reiserfs/kill-bkl: Ioctl path fixes and unlocked_ioctl conversion

[Frederic Weisbecker-3]

Hi,

This includes some fixes in the reiserfs ioctl path for the
reiserfs/kill-bkl tree.

As usual, any reviews or testings are very welcome.

Thanks,
Frederic.
---

The whole tree that drops the bkl from reiserfs 3 can be pulled from:

  git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
        reiserfs/kill-bkl

Frederic Weisbecker (3):
      kill-the-bkl/reiserfs: always lock the ioctl path
      kill-the-bkl/reiserfs: definitely drop the bkl from reiserfs_ioctl()
      kill-the-bkl/reiserfs: drop the fs race watchdog from _get_block_create_0()

 fs/reiserfs/dir.c           |    2 +-
 fs/reiserfs/file.c          |    2 +-
 fs/reiserfs/inode.c         |   11 +-----
 fs/reiserfs/ioctl.c         |   77 +++++++++++++++++++++++-------------------
 include/linux/reiserfs_fs.h |    3 +-
 5 files changed, 47 insertions(+), 48 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@...
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Frederic Weisbecker-3]

Reiserfs uses the ioctl callback for its file operations, which means
that its ioctl path is still locked by the bkl, this was synchronizing
with the rest of the filsystem operations. We have changed that by
locking it with the new reiserfs lock but we do that only from the
compat_ioctl callback.

Fix that by locking reiserfs_ioctl() everytime.

Signed-off-by: Frederic Weisbecker <fweisbec@...>
Cc: Jeff Mahoney <jeffm@...>
Cc: Chris Mason <chris.mason@...>
Cc: Ingo Molnar <mingo@...>
Cc: Alexander Beregalov <a.beregalov@...>
Cc: Laurent Riffard <laurent.riffard@...>
Cc: Thomas Gleixner <tglx@...>
---
 fs/reiserfs/ioctl.c |   66 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/fs/reiserfs/ioctl.c b/fs/reiserfs/ioctl.c
index 5e40b0c..e30e8be 100644
--- a/fs/reiserfs/ioctl.c
+++ b/fs/reiserfs/ioctl.c
@@ -13,44 +13,52 @@
 #include <linux/compat.h>
 
 /*
-** reiserfs_ioctl - handler for ioctl for inode
-** supported commands:
-**  1) REISERFS_IOC_UNPACK - try to unpack tail from direct item into indirect
-**                           and prevent packing file (argument arg has to be non-zero)
-**  2) REISERFS_IOC_[GS]ETFLAGS, REISERFS_IOC_[GS]ETVERSION
-**  3) That's all for a while ...
-*/
+ * reiserfs_ioctl - handler for ioctl for inode
+ * supported commands:
+ *  1) REISERFS_IOC_UNPACK - try to unpack tail from direct item into indirect
+ *                           and prevent packing file (argument arg has to be non-zero)
+ *  2) REISERFS_IOC_[GS]ETFLAGS, REISERFS_IOC_[GS]ETVERSION
+ *  3) That's all for a while ...
+ */
 int reiserfs_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
    unsigned long arg)
 {
  unsigned int flags;
  int err = 0;
 
+ reiserfs_write_lock(inode->i_sb);
+
  switch (cmd) {
  case REISERFS_IOC_UNPACK:
  if (S_ISREG(inode->i_mode)) {
  if (arg)
- return reiserfs_unpack(inode, filp);
- else
- return 0;
+ err = reiserfs_unpack(inode, filp);
  } else
- return -ENOTTY;
- /* following two cases are taken from fs/ext2/ioctl.c by Remy
-   Card (card@...) */
+ err = -ENOTTY;
+ break;
+ /*
+ * following two cases are taken from fs/ext2/ioctl.c by Remy
+ * Card (card@...)
+ */
  case REISERFS_IOC_GETFLAGS:
- if (!reiserfs_attrs(inode->i_sb))
- return -ENOTTY;
+ if (!reiserfs_attrs(inode->i_sb)) {
+ err = -ENOTTY;
+ break;
+ }
 
  flags = REISERFS_I(inode)->i_attrs;
  i_attrs_to_sd_attrs(inode, (__u16 *) & flags);
- return put_user(flags, (int __user *)arg);
+ err = put_user(flags, (int __user *)arg);
+ break;
  case REISERFS_IOC_SETFLAGS:{
- if (!reiserfs_attrs(inode->i_sb))
- return -ENOTTY;
+ if (!reiserfs_attrs(inode->i_sb)) {
+ err = -ENOTTY;
+ break;
+ }
 
  err = mnt_want_write(filp->f_path.mnt);
  if (err)
- return err;
+ break;
 
  if (!is_owner_or_cap(inode)) {
  err = -EPERM;
@@ -90,16 +98,18 @@ int reiserfs_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
  mark_inode_dirty(inode);
 setflags_out:
  mnt_drop_write(filp->f_path.mnt);
- return err;
+ break;
  }
  case REISERFS_IOC_GETVERSION:
- return put_user(inode->i_generation, (int __user *)arg);
+ err = put_user(inode->i_generation, (int __user *)arg);
+ break;
  case REISERFS_IOC_SETVERSION:
  if (!is_owner_or_cap(inode))
- return -EPERM;
+ err = -EPERM;
+ break;
  err = mnt_want_write(filp->f_path.mnt);
  if (err)
- return err;
+ break;
  if (get_user(inode->i_generation, (int __user *)arg)) {
  err = -EFAULT;
  goto setversion_out;
@@ -108,10 +118,14 @@ setflags_out:
  mark_inode_dirty(inode);
 setversion_out:
  mnt_drop_write(filp->f_path.mnt);
- return err;
+ break;
  default:
- return -ENOTTY;
+ err = -ENOTTY;
  }
+
+ reiserfs_write_unlock(inode->i_sb);
+
+ return err;
 }
 
 #ifdef CONFIG_COMPAT
@@ -142,9 +156,7 @@ long reiserfs_compat_ioctl(struct file *file, unsigned int cmd,
  return -ENOIOCTLCMD;
  }
 
- reiserfs_write_lock(inode->i_sb);
  ret = reiserfs_ioctl(inode, file, cmd, (unsigned long) compat_ptr(arg));
- reiserfs_write_unlock(inode->i_sb);
 
  return ret;
 }
--
1.6.2.3

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

[Frederic Weisbecker-3]

The reiserfs ioctl path doesn't need the big kernel lock anymore , now
that the filesystem synchronizes through its own lock.

We can then turn reiserfs_ioctl() into an unlocked_ioctl callback.

Signed-off-by: Frederic Weisbecker <fweisbec@...>
Cc: Jeff Mahoney <jeffm@...>
Cc: Chris Mason <chris.mason@...>
Cc: Ingo Molnar <mingo@...>
Cc: Alexander Beregalov <a.beregalov@...>
Cc: Laurent Riffard <laurent.riffard@...>
Cc: Thomas Gleixner <tglx@...>
---
 fs/reiserfs/dir.c           |    2 +-
 fs/reiserfs/file.c          |    2 +-
 fs/reiserfs/ioctl.c         |   11 +++--------
 include/linux/reiserfs_fs.h |    3 +--
 4 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/reiserfs/dir.c b/fs/reiserfs/dir.c
index 17f31ad..c094f58 100644
--- a/fs/reiserfs/dir.c
+++ b/fs/reiserfs/dir.c
@@ -20,7 +20,7 @@ const struct file_operations reiserfs_dir_operations = {
  .read = generic_read_dir,
  .readdir = reiserfs_readdir,
  .fsync = reiserfs_dir_fsync,
- .ioctl = reiserfs_ioctl,
+ .unlocked_ioctl = reiserfs_ioctl,
 #ifdef CONFIG_COMPAT
  .compat_ioctl = reiserfs_compat_ioctl,
 #endif
diff --git a/fs/reiserfs/file.c b/fs/reiserfs/file.c
index 9f43666..da2dba0 100644
--- a/fs/reiserfs/file.c
+++ b/fs/reiserfs/file.c
@@ -284,7 +284,7 @@ static ssize_t reiserfs_file_write(struct file *file, /* the file we are going t
 const struct file_operations reiserfs_file_operations = {
  .read = do_sync_read,
  .write = reiserfs_file_write,
- .ioctl = reiserfs_ioctl,
+ .unlocked_ioctl = reiserfs_ioctl,
 #ifdef CONFIG_COMPAT
  .compat_ioctl = reiserfs_compat_ioctl,
 #endif
diff --git a/fs/reiserfs/ioctl.c b/fs/reiserfs/ioctl.c
index e30e8be..ace7745 100644
--- a/fs/reiserfs/ioctl.c
+++ b/fs/reiserfs/ioctl.c
@@ -20,9 +20,9 @@
  *  2) REISERFS_IOC_[GS]ETFLAGS, REISERFS_IOC_[GS]ETVERSION
  *  3) That's all for a while ...
  */
-int reiserfs_ioctl(struct inode *inode, struct file *filp, unsigned int cmd,
-   unsigned long arg)
+long reiserfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
+ struct inode *inode = filp->f_path.dentry->d_inode;
  unsigned int flags;
  int err = 0;
 
@@ -132,9 +132,6 @@ setversion_out:
 long reiserfs_compat_ioctl(struct file *file, unsigned int cmd,
  unsigned long arg)
 {
- struct inode *inode = file->f_path.dentry->d_inode;
- int ret;
-
  /* These are just misnamed, they actually get/put from/to user an int */
  switch (cmd) {
  case REISERFS_IOC32_UNPACK:
@@ -156,9 +153,7 @@ long reiserfs_compat_ioctl(struct file *file, unsigned int cmd,
  return -ENOIOCTLCMD;
  }
 
- ret = reiserfs_ioctl(inode, file, cmd, (unsigned long) compat_ptr(arg));
-
- return ret;
+ return reiserfs_ioctl(file, cmd, (unsigned long) compat_ptr(arg));
 }
 #endif
 
diff --git a/include/linux/reiserfs_fs.h b/include/linux/reiserfs_fs.h
index a498d92..a05b4a2 100644
--- a/include/linux/reiserfs_fs.h
+++ b/include/linux/reiserfs_fs.h
@@ -2314,8 +2314,7 @@ __u32 r5_hash(const signed char *msg, int len);
 #define SPARE_SPACE 500
 
 /* prototypes from ioctl.c */
-int reiserfs_ioctl(struct inode *inode, struct file *filp,
-   unsigned int cmd, unsigned long arg);
+long reiserfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 long reiserfs_compat_ioctl(struct file *filp,
    unsigned int cmd, unsigned long arg);
 int reiserfs_unpack(struct inode *inode, struct file *filp);
--
1.6.2.3

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

[Frederic Weisbecker-3]

We had a watchdog in _get_block_create_0() that jumped to a fixup retry
path in case the bkl got relaxed while calling kmap().
This is not necessary anymore since we now have a reiserfs lock that is
not implicitly relaxed while sleeping.

Signed-off-by: Frederic Weisbecker <fweisbec@...>
Cc: Jeff Mahoney <jeffm@...>
Cc: Chris Mason <chris.mason@...>
Cc: Ingo Molnar <mingo@...>
Cc: Alexander Beregalov <a.beregalov@...>
Cc: Laurent Riffard <laurent.riffard@...>
Cc: Thomas Gleixner <tglx@...>
---
 fs/reiserfs/inode.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/fs/reiserfs/inode.c b/fs/reiserfs/inode.c
index 965c8ea..0d493a3 100644
--- a/fs/reiserfs/inode.c
+++ b/fs/reiserfs/inode.c
@@ -251,7 +251,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
  struct cpu_key key;
  struct buffer_head *bh;
  struct item_head *ih, tmp_ih;
- int fs_gen;
  b_blocknr_t blocknr;
  char *p = NULL;
  int chars;
@@ -265,7 +264,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
      (loff_t) block * inode->i_sb->s_blocksize + 1, TYPE_ANY,
      3);
 
-      research:
  result = search_for_position_by_key(inode->i_sb, &key, &path);
  if (result != POSITION_FOUND) {
  pathrelse(&path);
@@ -340,7 +338,6 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
  }
  // read file tail into part of page
  offset = (cpu_key_k_offset(&key) - 1) & (PAGE_CACHE_SIZE - 1);
- fs_gen = get_generation(inode->i_sb);
  copy_item_head(&tmp_ih, ih);
 
  /* we only want to kmap if we are reading the tail into the page.
@@ -348,13 +345,9 @@ static int _get_block_create_0(struct inode *inode, sector_t block,
  ** sure we need to.  But, this means the item might move if
  ** kmap schedules
  */
- if (!p) {
+ if (!p)
  p = (char *)kmap(bh_result->b_page);
- if (fs_changed(fs_gen, inode->i_sb)
-    && item_moved(&tmp_ih, &path)) {
- goto research;
- }
- }
+
  p += offset;
  memset(p, 0, inode->i_sb->s_blocksize);
  do {
--
1.6.2.3

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