Re: [PATCH 11/18] fs: split locking of inode writeback and LRU lists

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 13, 2010 at 8:15 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Given that the inode LRU and IO lists are split apart, they do not
> need to be protected by the same lock. So in preparation for removal
> of the inode_lock, add new locks for them. The writeback lists are
> only ever accessed in the context of a bdi, so add a per-BDI lock to
> protect manipulations of these lists.
>
> For the inode LRU, introduce a simple global lock to protect it.
> While this could be made per-sb, it is unclear yet as to what is the
> next step for optimising/parallelising reclaim of inodes. Rather
> than optimise now, leave it as a global list and lock until further
> analysis can be done.
>
> Because there will now be a situation where the inode is on
> different lists protected by different locks during the freeing of
> the inode (i.e. not an atomic state transition), we need to ensure
> that we set the I_FREEING state flag before we start removing inodes
> from the IO and LRU lists. This ensures that if we race with other
> threads during freeing, they will notice the I_FREEING flag is set
> and be able to take appropriate action to avoid problems.
>
> Based on a patch originally from Nick Piggin.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  fs/fs-writeback.c           |   51 +++++++++++++++++++++++++++++++++++++---
>  fs/inode.c                  |   54 ++++++++++++++++++++++++++++++++++++------
>  fs/internal.h               |    5 ++++
>  include/linux/backing-dev.h |    1 +
>  mm/backing-dev.c            |   18 ++++++++++++++
>  5 files changed, 117 insertions(+), 12 deletions(-)
>
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 387385b..45046af 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -157,6 +157,18 @@ void bdi_start_background_writeback(struct backing_dev_info *bdi)
>  }
>
>  /*
> + * Remove the inode from the writeback list it is on.
> + */
> +void inode_wb_list_del(struct inode *inode)
> +{
> +       struct backing_dev_info *bdi = inode_to_bdi(inode);
> +
> +       spin_lock(&bdi->wb.b_lock);
> +       list_del_init(&inode->i_wb_list);
> +       spin_unlock(&bdi->wb.b_lock);
> +}
> +
> +/*
>  * Redirty an inode: set its when-it-was dirtied timestamp and move it to the
>  * furthest end of its superblock's dirty-inode list.
>  *
> @@ -169,6 +181,7 @@ static void redirty_tail(struct inode *inode)
>  {
>        struct bdi_writeback *wb = &inode_to_bdi(inode)->wb;
>
> +       assert_spin_locked(&wb->b_lock);
>        if (!list_empty(&wb->b_dirty)) {
>                struct inode *tail;
>
> @@ -186,6 +199,7 @@ static void requeue_io(struct inode *inode)
>  {
>        struct bdi_writeback *wb = &inode_to_bdi(inode)->wb;
>
> +       assert_spin_locked(&wb->b_lock);
>        list_move(&inode->i_wb_list, &wb->b_more_io);
>  }
>
> @@ -269,6 +283,7 @@ static void move_expired_inodes(struct list_head *delaying_queue,
>  */
>  static void queue_io(struct bdi_writeback *wb, unsigned long *older_than_this)
>  {
> +       assert_spin_locked(&wb->b_lock);
>        list_splice_init(&wb->b_more_io, &wb->b_io);
>        move_expired_inodes(&wb->b_dirty, &wb->b_io, older_than_this);
>  }
> @@ -311,6 +326,7 @@ static void inode_wait_for_writeback(struct inode *inode)
>  static int
>  writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
>  {
> +       struct backing_dev_info *bdi = inode_to_bdi(inode);
>        struct address_space *mapping = inode->i_mapping;
>        unsigned dirty;
>        int ret;
> @@ -330,7 +346,9 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
>                 * completed a full scan of b_io.
>                 */
>                if (wbc->sync_mode != WB_SYNC_ALL) {
> +                       spin_lock(&bdi->wb.b_lock);
>                        requeue_io(inode);
> +                       spin_unlock(&bdi->wb.b_lock);
>                        return 0;
>                }
>
> @@ -385,6 +403,7 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
>                         * sometimes bales out without doing anything.
>                         */
>                        inode->i_state |= I_DIRTY_PAGES;
> +                       spin_lock(&bdi->wb.b_lock);
>                        if (wbc->nr_to_write <= 0) {
>                                /*
>                                 * slice used up: queue for next turn
> @@ -400,6 +419,7 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
>                                 */
>                                redirty_tail(inode);
>                        }
> +                       spin_unlock(&bdi->wb.b_lock);
>                } else if (inode->i_state & I_DIRTY) {
>                        /*
>                         * Filesystems can dirty the inode during writeback
> @@ -407,10 +427,12 @@ writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
>                         * submission or metadata updates after data IO
>                         * completion.
>                         */
> +                       spin_lock(&bdi->wb.b_lock);
>                        redirty_tail(inode);
> +                       spin_unlock(&bdi->wb.b_lock);
>                } else {
>                        /* The inode is clean */
> -                       list_del_init(&inode->i_wb_list);
> +                       inode_wb_list_del(inode);
>                        inode_lru_list_add(inode);
>                }
>        }
> @@ -457,6 +479,7 @@ static bool pin_sb_for_writeback(struct super_block *sb)
>  static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb,
>                struct writeback_control *wbc, bool only_this_sb)
>  {
> +       assert_spin_locked(&wb->b_lock);
>        while (!list_empty(&wb->b_io)) {
>                long pages_skipped;
>                struct inode *inode = list_entry(wb->b_io.prev,
> @@ -472,7 +495,6 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb,
>                                redirty_tail(inode);
>                                continue;
>                        }
> -
>                        /*
>                         * The inode belongs to a different superblock.
>                         * Bounce back to the caller to unpin this and
> @@ -481,7 +503,15 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb,
>                        return 0;
>                }
>
> -               if (inode->i_state & (I_NEW | I_WILL_FREE)) {
> +               /*
> +                * We can see I_FREEING here when the inod isin the process of

s/inod isin/inode is in/

> +                * being reclaimed. In that case the freer is waiting on the
> +                * wb->b_lock that we currently hold to remove the inode from
> +                * the writeback list. So we don't spin on it here, requeue it
> +                * and move on to the next inode, which will allow the other
> +                * thread to free the inode when we drop the lock.
> +                */
> +               if (inode->i_state & (I_NEW | I_WILL_FREE | I_FREEING)) {
>                        requeue_io(inode);
>                        continue;
>                }
> @@ -492,10 +522,11 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb,
>                if (inode_dirtied_after(inode, wbc->wb_start))
>                        return 1;
>
> -               BUG_ON(inode->i_state & I_FREEING);
>                spin_lock(&inode->i_lock);
>                inode->i_ref++;
>                spin_unlock(&inode->i_lock);
> +               spin_unlock(&wb->b_lock);
> +
>                pages_skipped = wbc->pages_skipped;
>                writeback_single_inode(inode, wbc);
>                if (wbc->pages_skipped != pages_skipped) {
> @@ -503,12 +534,15 @@ static int writeback_sb_inodes(struct super_block *sb, struct bdi_writeback *wb,
>                         * writeback is not making progress due to locked
>                         * buffers.  Skip this inode for now.
>                         */
> +                       spin_lock(&wb->b_lock);
>                        redirty_tail(inode);
> +                       spin_unlock(&wb->b_lock);
>                }
>                spin_unlock(&inode_lock);
>                iput(inode);
>                cond_resched();
>                spin_lock(&inode_lock);
> +               spin_lock(&wb->b_lock);
>                if (wbc->nr_to_write <= 0) {
>                        wbc->more_io = 1;
>                        return 1;
> @@ -528,6 +562,8 @@ void writeback_inodes_wb(struct bdi_writeback *wb,
>        if (!wbc->wb_start)
>                wbc->wb_start = jiffies; /* livelock avoidance */
>        spin_lock(&inode_lock);
> +       spin_lock(&wb->b_lock);
> +
>        if (!wbc->for_kupdate || list_empty(&wb->b_io))
>                queue_io(wb, wbc->older_than_this);
>
> @@ -546,6 +582,7 @@ void writeback_inodes_wb(struct bdi_writeback *wb,
>                if (ret)
>                        break;
>        }
> +       spin_unlock(&wb->b_lock);
>        spin_unlock(&inode_lock);
>        /* Leave any unwritten inodes on b_io */
>  }
> @@ -556,9 +593,11 @@ static void __writeback_inodes_sb(struct super_block *sb,
>        WARN_ON(!rwsem_is_locked(&sb->s_umount));
>
>        spin_lock(&inode_lock);
> +       spin_lock(&wb->b_lock);
>        if (!wbc->for_kupdate || list_empty(&wb->b_io))
>                queue_io(wb, wbc->older_than_this);
>        writeback_sb_inodes(sb, wb, wbc, true);
> +       spin_unlock(&wb->b_lock);
>        spin_unlock(&inode_lock);
>  }
>
> @@ -671,8 +710,10 @@ static long wb_writeback(struct bdi_writeback *wb,
>                 */
>                spin_lock(&inode_lock);
>                if (!list_empty(&wb->b_more_io))  {
> +                       spin_lock(&wb->b_lock);
>                        inode = list_entry(wb->b_more_io.prev,
>                                                struct inode, i_wb_list);
> +                       spin_unlock(&wb->b_lock);
>                        trace_wbc_writeback_wait(&wbc, wb->bdi);
>                        inode_wait_for_writeback(inode);
>                }
> @@ -985,8 +1026,10 @@ void __mark_inode_dirty(struct inode *inode, int flags)
>                                        wakeup_bdi = true;
>                        }
>
> +                       spin_lock(&bdi->wb.b_lock);
>                        inode->dirtied_when = jiffies;
>                        list_move(&inode->i_wb_list, &bdi->wb.b_dirty);
> +                       spin_unlock(&bdi->wb.b_lock);
>                }
>        }
>  out:
> diff --git a/fs/inode.c b/fs/inode.c
> index ab65f99..a9ba18a 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -26,6 +26,8 @@
>  #include <linux/posix_acl.h>
>  #include <linux/bit_spinlock.h>
>
> +#include "internal.h"
> +
>  /*
>  * Locking rules.
>  *
> @@ -35,6 +37,10 @@
>  *   inode hash table, i_hash
>  * sb inode lock protects:
>  *   s_inodes, i_sb_list
> + * bdi writeback lock protects:
> + *   b_io, b_more_io, b_dirty, i_io

s/i_io/i_wb_list/

> + * inode_lru_lock protects:
> + *   inode_lru, i_lru
>  *
>  * Lock orders
>  * inode_lock
> @@ -43,7 +49,9 @@
>  *
>  * inode_lock
>  *   sb inode lock
> - *     inode->i_lock
> + *     inode_lru_lock
> + *       wb->b_lock
> + *         inode->i_lock
>  */
>  /*
>  * This is needed for the following functions:
> @@ -93,6 +101,7 @@ static struct hlist_bl_head *inode_hashtable __read_mostly;
>  * allowing for low-overhead inode sync() operations.
>  */
>  static LIST_HEAD(inode_lru);
> +static DEFINE_SPINLOCK(inode_lru_lock);
>
>  /*
>  * A simple spinlock to protect the list manipulations.
> @@ -353,20 +362,28 @@ void iref(struct inode *inode)
>  }
>  EXPORT_SYMBOL_GPL(iref);
>
> +/*
> + * check against I_FREEING as inode writeback completion could race with
> + * setting the I_FREEING and removing the inode from the LRU.
> + */
>  void inode_lru_list_add(struct inode *inode)
>  {
> -       if (list_empty(&inode->i_lru)) {
> +       spin_lock(&inode_lru_lock);
> +       if (list_empty(&inode->i_lru) && !(inode->i_state & I_FREEING)) {
>                list_add(&inode->i_lru, &inode_lru);
>                percpu_counter_inc(&nr_inodes_unused);
>        }
> +       spin_unlock(&inode_lru_lock);
>  }
>
>  void inode_lru_list_del(struct inode *inode)
>  {
> +       spin_lock(&inode_lru_lock);
>        if (!list_empty(&inode->i_lru)) {
>                list_del_init(&inode->i_lru);
>                percpu_counter_dec(&nr_inodes_unused);
>        }
> +       spin_unlock(&inode_lru_lock);
>  }
>
>  static unsigned long hash(struct super_block *sb, unsigned long hashval)
> @@ -524,8 +541,18 @@ static int invalidate_list(struct super_block *sb, struct list_head *head,
>                        spin_unlock(&inode->i_lock);
>                        WARN_ON(inode->i_state & I_NEW);
>                        inode->i_state |= I_FREEING;
> +
> +                       /*
> +                        * move the inode off the IO lists and LRU once

s/IO lists/writeback lists/

> +                        * I_FREEING is set so that it won't get moved back on
> +                        * there if it is dirty.
> +                        */
> +                       inode_wb_list_del(inode);
> +
> +                       spin_lock(&inode_lru_lock);
>                        list_move(&inode->i_lru, dispose);
>                        percpu_counter_dec(&nr_inodes_unused);
> +                       spin_unlock(&inode_lru_lock);
>                        continue;
>                }
>                spin_unlock(&inode->i_lock);
> @@ -599,6 +626,7 @@ static void prune_icache(int nr_to_scan)
>
>        down_read(&iprune_sem);
>        spin_lock(&inode_lock);
> +       spin_lock(&inode_lru_lock);
>        for (nr_scanned = 0; nr_scanned < nr_to_scan; nr_scanned++) {
>                struct inode *inode;
>
> @@ -629,12 +657,14 @@ static void prune_icache(int nr_to_scan)
>                if (inode_has_buffers(inode) || inode->i_data.nrpages) {
>                        inode->i_ref++;
>                        spin_unlock(&inode->i_lock);
> +                       spin_unlock(&inode_lru_lock);
>                        spin_unlock(&inode_lock);
>                        if (remove_inode_buffers(inode))
>                                reap += invalidate_mapping_pages(&inode->i_data,
>                                                                0, -1);
>                        iput(inode);
>                        spin_lock(&inode_lock);
> +                       spin_lock(&inode_lru_lock);
>
>                        /*
>                         * if we can't reclaim this inode immediately, give it
> @@ -647,16 +677,24 @@ static void prune_icache(int nr_to_scan)
>                        }
>                } else
>                        spin_unlock(&inode->i_lock);
> -               list_move(&inode->i_lru, &freeable);
> -               list_del_init(&inode->i_wb_list);
>                WARN_ON(inode->i_state & I_NEW);
>                inode->i_state |= I_FREEING;
> +
> +               /*
> +                * move the inode off the io lists and lru once
> +                * i_freeing is set so that it won't get moved back on
> +                * there if it is dirty.
> +                */

s/io lists/writeback lists/
s/i_freeing/I_FREEING/

> +               inode_wb_list_del(inode);
> +
> +               list_move(&inode->i_lru, &freeable);
>                percpu_counter_dec(&nr_inodes_unused);
>        }
>        if (current_is_kswapd())
>                __count_vm_events(KSWAPD_INODESTEAL, reap);
>        else
>                __count_vm_events(PGINODESTEAL, reap);
> +       spin_unlock(&inode_lru_lock);
>        spin_unlock(&inode_lock);
>
>        dispose_list(&freeable);
> @@ -1389,15 +1427,15 @@ static void iput_final(struct inode *inode)
>                inode->i_state &= ~I_WILL_FREE;
>                __remove_inode_hash(inode);
>        }
> -       list_del_init(&inode->i_wb_list);
>        WARN_ON(inode->i_state & I_NEW);
>        inode->i_state |= I_FREEING;
>
>        /*
> -        * After we delete the inode from the LRU here, we avoid moving dirty
> -        * inodes back onto the LRU now because I_FREEING is set and hence
> -        * writeback_single_inode() won't move the inode around.
> +        * After we delete the inode from the LRU and IO lists here, we avoid

s/IO lists/writeback lists/

Thanks,
Lin Ming
--
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


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux