Skip to content

Commit

Permalink
reiserfs: kill-the-BKL
Browse files Browse the repository at this point in the history
This patch is an attempt to remove the Bkl based locking scheme from
reiserfs and is intended.

It is a bit inspired from an old attempt by Peter Zijlstra:

   http://lkml.indiana.edu/hypermail/linux/kernel/0704.2/2174.html

The bkl is heavily used in this filesystem to prevent from
concurrent write accesses on the filesystem.

Reiserfs makes a deep use of the specific properties of the Bkl:

- It can be acqquired recursively by a same task
- It is released on the schedule() calls and reacquired when schedule() returns

The two properties above are a roadmap for the reiserfs write locking so it's
very hard to simply replace it with a common mutex.

- We need a recursive-able locking unless we want to restructure several blocks
  of the code.
- We need to identify the sites where the bkl was implictly relaxed
  (schedule, wait, sync, etc...) so that we can in turn release and
  reacquire our new lock explicitly.
  Such implicit releases of the lock are often required to let other
  resources producer/consumer do their job or we can suffer unexpected
  starvations or deadlocks.

So the new lock that replaces the bkl here is a per superblock mutex with a
specific property: it can be acquired recursively by a same task, like the
bkl.

For such purpose, we integrate a lock owner and a lock depth field on the
superblock information structure.

The first axis on this patch is to turn reiserfs_write_(un)lock() function
into a wrapper to manage this mutex. Also some explicit calls to
lock_kernel() have been converted to reiserfs_write_lock() helpers.

The second axis is to find the important blocking sites (schedule...(),
wait_on_buffer(), sync_dirty_buffer(), etc...) and then apply an explicit
release of the write lock on these locations before blocking. Then we can
safely wait for those who can give us resources or those who need some.
Typically this is a fight between the current writer, the reiserfs workqueue
(aka the async commiter) and the pdflush threads.

The third axis is a consequence of the second. The write lock is usually
on top of a lock dependency chain which can include the journal lock, the
flush lock or the commit lock. So it's dangerous to release and trying to
reacquire the write lock while we still hold other locks.

This is fine with the bkl:

      T1                       T2

lock_kernel()
    mutex_lock(A)
    unlock_kernel()
    // do something
                            lock_kernel()
                                mutex_lock(A) -> already locked by T1
                                schedule() (and then unlock_kernel())
    lock_kernel()
    mutex_unlock(A)
    ....

This is not fine with a mutex:

      T1                       T2

mutex_lock(write)
    mutex_lock(A)
    mutex_unlock(write)
    // do something
                           mutex_lock(write)
                              mutex_lock(A) -> already locked by T1
                              schedule()

    mutex_lock(write) -> already locked by T2
    deadlock

The solution in this patch is to provide a helper which releases the write
lock and sleep a bit if we can't lock a mutex that depend on it. It's another
simulation of the bkl behaviour.

The last axis is to locate the fs callbacks that are called with the bkl held,
according to Documentation/filesystem/Locking.

Those are:

- reiserfs_remount
- reiserfs_fill_super
- reiserfs_put_super

Reiserfs didn't need to explicitly lock because of the context of these callbacks.
But now we must take care of that with the new locking.

After this patch, reiserfs suffers from a slight performance regression (for now).
On UP, a high volume write with dd reports an average of 27 MB/s instead
of 30 MB/s without the patch applied.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Reviewed-by: Ingo Molnar <mingo@elte.hu>
Cc: Jeff Mahoney <jeffm@suse.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Bron Gondwana <brong@fastmail.fm>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
LKML-Reference: <1239070789-13354-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
  • Loading branch information
Frederic Weisbecker committed Sep 14, 2009
1 parent 74fca6a commit 8ebc423
Show file tree
Hide file tree
Showing 13 changed files with 261 additions and 49 deletions.
2 changes: 1 addition & 1 deletion fs/reiserfs/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ obj-$(CONFIG_REISERFS_FS) += reiserfs.o
reiserfs-objs := bitmap.o do_balan.o namei.o inode.o file.o dir.o fix_node.o \
super.o prints.o objectid.o lbalance.o ibalance.o stree.o \
hashes.o tail_conversion.o journal.o resize.o \
item_ops.o ioctl.o procfs.o xattr.o
item_ops.o ioctl.o procfs.o xattr.o lock.o

ifeq ($(CONFIG_REISERFS_FS_XATTR),y)
reiserfs-objs += xattr_user.o xattr_trusted.o
Expand Down
2 changes: 2 additions & 0 deletions fs/reiserfs/bitmap.c
Original file line number Diff line number Diff line change
Expand Up @@ -1256,7 +1256,9 @@ struct buffer_head *reiserfs_read_bitmap_block(struct super_block *sb,
else {
if (buffer_locked(bh)) {
PROC_INFO_INC(sb, scan_bitmap.wait);
reiserfs_write_unlock(sb);
__wait_on_buffer(bh);
reiserfs_write_lock(sb);
}
BUG_ON(!buffer_uptodate(bh));
BUG_ON(atomic_read(&bh->b_count) == 0);
Expand Down
8 changes: 8 additions & 0 deletions fs/reiserfs/dir.c
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,22 @@ int reiserfs_readdir_dentry(struct dentry *dentry, void *dirent,
// user space buffer is swapped out. At that time
// entry can move to somewhere else
memcpy(local_buf, d_name, d_reclen);

/*
* Since filldir might sleep, we can release
* the write lock here for other waiters
*/
reiserfs_write_unlock(inode->i_sb);
if (filldir
(dirent, local_buf, d_reclen, d_off, d_ino,
DT_UNKNOWN) < 0) {
reiserfs_write_lock(inode->i_sb);
if (local_buf != small_buf) {
kfree(local_buf);
}
goto end;
}
reiserfs_write_lock(inode->i_sb);
if (local_buf != small_buf) {
kfree(local_buf);
}
Expand Down
10 changes: 10 additions & 0 deletions fs/reiserfs/fix_node.c
Original file line number Diff line number Diff line change
Expand Up @@ -1022,7 +1022,11 @@ static int get_far_parent(struct tree_balance *tb,
/* Check whether the common parent is locked. */

if (buffer_locked(*pcom_father)) {

/* Release the write lock while the buffer is busy */
reiserfs_write_unlock(tb->tb_sb);
__wait_on_buffer(*pcom_father);
reiserfs_write_lock(tb->tb_sb);
if (FILESYSTEM_CHANGED_TB(tb)) {
brelse(*pcom_father);
return REPEAT_SEARCH;
Expand Down Expand Up @@ -1927,7 +1931,9 @@ static int get_direct_parent(struct tree_balance *tb, int h)
return REPEAT_SEARCH;

if (buffer_locked(bh)) {
reiserfs_write_unlock(tb->tb_sb);
__wait_on_buffer(bh);
reiserfs_write_lock(tb->tb_sb);
if (FILESYSTEM_CHANGED_TB(tb))
return REPEAT_SEARCH;
}
Expand Down Expand Up @@ -2278,7 +2284,9 @@ static int wait_tb_buffers_until_unlocked(struct tree_balance *tb)
REPEAT_SEARCH : CARRY_ON;
}
#endif
reiserfs_write_unlock(tb->tb_sb);
__wait_on_buffer(locked);
reiserfs_write_lock(tb->tb_sb);
if (FILESYSTEM_CHANGED_TB(tb))
return REPEAT_SEARCH;
}
Expand Down Expand Up @@ -2349,7 +2357,9 @@ int fix_nodes(int op_mode, struct tree_balance *tb,

/* if it possible in indirect_to_direct conversion */
if (buffer_locked(tbS0)) {
reiserfs_write_unlock(tb->tb_sb);
__wait_on_buffer(tbS0);
reiserfs_write_lock(tb->tb_sb);
if (FILESYSTEM_CHANGED_TB(tb))
return REPEAT_SEARCH;
}
Expand Down
23 changes: 18 additions & 5 deletions fs/reiserfs/inode.c
Original file line number Diff line number Diff line change
Expand Up @@ -489,10 +489,14 @@ static int reiserfs_get_blocks_direct_io(struct inode *inode,
disappeared */
if (REISERFS_I(inode)->i_flags & i_pack_on_close_mask) {
int err;
lock_kernel();

reiserfs_write_lock(inode->i_sb);

err = reiserfs_commit_for_inode(inode);
REISERFS_I(inode)->i_flags &= ~i_pack_on_close_mask;
unlock_kernel();

reiserfs_write_unlock(inode->i_sb);

if (err < 0)
ret = err;
}
Expand Down Expand Up @@ -616,7 +620,6 @@ int reiserfs_get_block(struct inode *inode, sector_t block,
loff_t new_offset =
(((loff_t) block) << inode->i_sb->s_blocksize_bits) + 1;

/* bad.... */
reiserfs_write_lock(inode->i_sb);
version = get_inode_item_key_version(inode);

Expand Down Expand Up @@ -997,10 +1000,14 @@ int reiserfs_get_block(struct inode *inode, sector_t block,
if (retval)
goto failure;
}
/* inserting indirect pointers for a hole can take a
** long time. reschedule if needed
/*
* inserting indirect pointers for a hole can take a
* long time. reschedule if needed and also release the write
* lock for others.
*/
reiserfs_write_unlock(inode->i_sb);
cond_resched();
reiserfs_write_lock(inode->i_sb);

retval = search_for_position_by_key(inode->i_sb, &key, &path);
if (retval == IO_ERROR) {
Expand Down Expand Up @@ -2608,7 +2615,10 @@ int reiserfs_prepare_write(struct file *f, struct page *page,
int ret;
int old_ref = 0;

reiserfs_write_unlock(inode->i_sb);
reiserfs_wait_on_write_block(inode->i_sb);
reiserfs_write_lock(inode->i_sb);

fix_tail_page_for_writing(page);
if (reiserfs_transaction_running(inode->i_sb)) {
struct reiserfs_transaction_handle *th;
Expand Down Expand Up @@ -2758,7 +2768,10 @@ int reiserfs_commit_write(struct file *f, struct page *page,
int update_sd = 0;
struct reiserfs_transaction_handle *th = NULL;

reiserfs_write_unlock(inode->i_sb);
reiserfs_wait_on_write_block(inode->i_sb);
reiserfs_write_lock(inode->i_sb);

if (reiserfs_transaction_running(inode->i_sb)) {
th = current->journal_info;
}
Expand Down
6 changes: 4 additions & 2 deletions fs/reiserfs/ioctl.c
Original file line number Diff line number Diff line change
Expand Up @@ -141,9 +141,11 @@ long reiserfs_compat_ioctl(struct file *file, unsigned int cmd,
default:
return -ENOIOCTLCMD;
}
lock_kernel();

reiserfs_write_lock(inode->i_sb);
ret = reiserfs_ioctl(inode, file, cmd, (unsigned long) compat_ptr(arg));
unlock_kernel();
reiserfs_write_unlock(inode->i_sb);

return ret;
}
#endif
Expand Down
Loading

0 comments on commit 8ebc423

Please sign in to comment.