lkml.org 
[lkml]   [2012]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 4/4] writeback: Avoid iput() from flusher thread
    On Fri, Mar 09, 2012 at 10:02:28AM +0100, Jan Kara wrote:
    > Doing iput() from flusher thread (writeback_sb_inodes()) can create problems
    > because iput() can do a lot of work - for example truncate the inode if it's
    > the last iput on unlinked file. Some filesystems (e.g. ubifs) may need to
    > allocate blocks during truncate (due to their COW nature) and in some cases
    > they thus need to flush dirty data from truncate to reduce uncertainty in the
    > amount of free space. This effectively creates a deadlock.
    >
    > We get rid of iput() in flusher thread by using the fact that I_SYNC inode
    > flag effectively pins the inode in memory. So if we take care to either hold
    > i_lock or have I_SYNC set, we can get away without taking inode reference
    > in writeback_sb_inodes().

    I created this graph while trying to understand/prove the new locking
    rules, and it looks perfectly fine.

    flusher:

    I_FREEING check requeue/dequeue
    . .
    b_io.prev . writepages write_inode .
    . . . . .
    . . . . .
    I_SYNC . . ==============================
    i_lock . ============ === ======
    list_lock ============= . . ==============
    . .
    . .
    check/wait I_SYNC I_DIRTY handling


    evict/end_writeback:

    check I_SYNC
    set I_FREEING . wait I_SYNC destroy_inode
    . dequeue . . .
    I_SYNC . . . ********************* .
    i_lock ===== . ==== *==== .
    list_lock ======

    It may be worthwhile to note that evict() needs to grab i_lock *after*
    waiting for I_SYNC, so that the flusher won't be accessing possibly
    freed inode when unlocking i_lock.

    > As a side effect, we also fix possible use-after-free in wb_writeback() because
    > inode_wait_for_writeback() call could try to reacquire i_lock on the inode that
    > was already free.

    Good catch! I think you are referring to this code:

    /*
    * Nothing written. Wait for some inode to
    * become available for writeback. Otherwise
    * we'll just busyloop.
    */
    if (!list_empty(&wb->b_more_io)) {
    trace_writeback_wait(wb->bdi, work);
    inode = wb_inode(wb->b_more_io.prev);
    spin_lock(&inode->i_lock);
    inode_wait_for_writeback(inode, wb);
    spin_unlock(&inode->i_lock);
    }

    > Signed-off-by: Jan Kara <jack@suse.cz>
    > ---
    > fs/fs-writeback.c | 38 ++++++++++++++++++++++++--------------
    > fs/inode.c | 11 ++++++++++-
    > include/linux/fs.h | 7 ++++---
    > include/linux/writeback.h | 7 +------
    > 4 files changed, 39 insertions(+), 24 deletions(-)
    >
    > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
    > index 1e8bf44..f9f9b61 100644
    > --- a/fs/fs-writeback.c
    > +++ b/fs/fs-writeback.c
    > @@ -325,19 +325,21 @@ static int write_inode(struct inode *inode, struct writeback_control *wbc)
    > }
    >
    > /*
    > - * Wait for writeback on an inode to complete.
    > + * Wait for writeback on an inode to complete. Called with i_lock held.
    > + * Return 1 if we dropped i_lock and waited, 0 is returned otherwise.
    > */
    > -static void inode_wait_for_writeback(struct inode *inode)
    > +int __must_check inode_wait_for_writeback(struct inode *inode)
    > {
    > DEFINE_WAIT_BIT(wq, &inode->i_state, __I_SYNC);
    > wait_queue_head_t *wqh;
    >
    > wqh = bit_waitqueue(&inode->i_state, __I_SYNC);
    > - while (inode->i_state & I_SYNC) {
    > + if (inode->i_state & I_SYNC) {
    > spin_unlock(&inode->i_lock);
    > __wait_on_bit(wqh, &wq, inode_wait, TASK_UNINTERRUPTIBLE);
    > - spin_lock(&inode->i_lock);
    > + return 1;
    > }
    > + return 0;
    > }
    >
    > /*
    > @@ -426,9 +428,12 @@ writeback_single_inode(struct inode *inode, struct bdi_writeback *wb,
    > return 0;
    > }
    > /*
    > - * It's a data-integrity sync. We must wait.
    > + * It's a data-integrity sync. We must wait. Since callers hold
    > + * inode reference or inode has I_WILL_FREE set, it cannot go
    > + * away under us.
    > */
    > - inode_wait_for_writeback(inode);
    > + while (inode_wait_for_writeback(inode))
    > + spin_lock(&inode->i_lock);
    > }
    >
    > ret = __writeback_single_inode(inode, wb, wbc);
    > @@ -604,12 +609,20 @@ static long writeback_sb_inodes(struct super_block *sb,
    > }
    > spin_unlock(&wb->list_lock);
    >
    > - __iget(inode);
    > - inode_wait_for_writeback(inode);
    > + /* Did we drop i_lock to wait for I_SYNC? */
    > + if (inode_wait_for_writeback(inode)) {
    > + /* Inode may be gone, start again */
    > + spin_lock(&wb->list_lock);
    > + continue;
    > + }
    > write_chunk = writeback_chunk_size(wb->bdi, work);
    > wbc.nr_to_write = write_chunk;
    > wbc.pages_skipped = 0;
    >
    > + /*
    > + * We use I_SYNC to pin the inode in memory. While it is set
    > + * end_writeback() will wait so the inode cannot be freed.
    > + */
    > __writeback_single_inode(inode, wb, &wbc);
    >
    > work->nr_pages -= write_chunk - wbc.nr_to_write;
    > @@ -633,10 +646,7 @@ static long writeback_sb_inodes(struct super_block *sb,
    > continue_unlock:
    > inode_sync_complete(inode);
    > spin_unlock(&inode->i_lock);
    > - spin_unlock(&wb->list_lock);
    > - iput(inode);
    > - cond_resched();
    > - spin_lock(&wb->list_lock);
    > + cond_resched_lock(&wb->list_lock);
    > /*
    > * bail out to wb_writeback() often enough to check
    > * background threshold and other termination conditions.
    > @@ -831,8 +841,8 @@ static long wb_writeback(struct bdi_writeback *wb,
    > inode = wb_inode(wb->b_more_io.prev);
    > spin_lock(&inode->i_lock);
    > spin_unlock(&wb->list_lock);
    > - inode_wait_for_writeback(inode);
    > - spin_unlock(&inode->i_lock);
    > + if (!inode_wait_for_writeback(inode))
    > + spin_unlock(&inode->i_lock);
    > spin_lock(&wb->list_lock);
    > }
    > }
    > diff --git a/fs/inode.c b/fs/inode.c
    > index d3ebdbe..b64e2fe 100644
    > --- a/fs/inode.c
    > +++ b/fs/inode.c
    > @@ -510,7 +510,16 @@ void end_writeback(struct inode *inode)
    > BUG_ON(!list_empty(&inode->i_data.private_list));
    > BUG_ON(!(inode->i_state & I_FREEING));
    > BUG_ON(inode->i_state & I_CLEAR);
    > - inode_sync_wait(inode);
    > + /*
    > + * Wait for flusher thread to be done with the inode. Since the inode
    > + * has I_FREEING set, flusher thread won't start new work on the inode.
    > + * We just have to wait for running writeback to finish. We must use
    > + * i_lock here because flusher thread might be working with the inode
    > + * without I_SYNC set but under i_lock.
    > + */
    > + spin_lock(&inode->i_lock);
    > + if (!inode_wait_for_writeback(inode))
    > + spin_unlock(&inode->i_lock);
    > /* don't need i_lock here, no concurrent mods to i_state */
    > inode->i_state = I_FREEING | I_CLEAR;
    > }
    > diff --git a/include/linux/fs.h b/include/linux/fs.h
    > index 69cd5bb..e1f0f5a 100644
    > --- a/include/linux/fs.h
    > +++ b/include/linux/fs.h
    > @@ -1742,9 +1742,10 @@ struct super_operations {
    > * anew. Other functions will just ignore such inodes,
    > * if appropriate. I_NEW is used for waiting.
    > *
    > - * I_SYNC Synchonized write of dirty inode data. The bits is
    > - * set during data writeback, and cleared with a wakeup
    > - * on the bit address once it is done.
    > + * I_SYNC Writeback of inode is running. The bits is set during

    s/bits/bit/

    > + * data writeback, and cleared with a wakeup on the bit
    > + * address once it is done. The bit is also used to pin
    > + * the inode in memory for flusher thread.
    > *
    > * I_REFERENCED Marks the inode as recently references on the LRU list.
    > *
    > diff --git a/include/linux/writeback.h b/include/linux/writeback.h
    > index 995b8bf..3a34dc0 100644
    > --- a/include/linux/writeback.h
    > +++ b/include/linux/writeback.h
    > @@ -94,6 +94,7 @@ long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
    > enum wb_reason reason);
    > long wb_do_writeback(struct bdi_writeback *wb, int force_wait);
    > void wakeup_flusher_threads(long nr_pages, enum wb_reason reason);
    > +int __must_check inode_wait_for_writeback(struct inode *inode);
    >
    > /* writeback.h requires fs.h; it, too, is not included from here. */
    > static inline void wait_on_inode(struct inode *inode)
    > @@ -101,12 +102,6 @@ static inline void wait_on_inode(struct inode *inode)
    > might_sleep();
    > wait_on_bit(&inode->i_state, __I_NEW, inode_wait, TASK_UNINTERRUPTIBLE);
    > }
    > -static inline void inode_sync_wait(struct inode *inode)
    > -{
    > - might_sleep();
    > - wait_on_bit(&inode->i_state, __I_SYNC, inode_wait,
    > - TASK_UNINTERRUPTIBLE);
    > -}
    >
    >
    > /*
    > --
    > 1.7.1


    \
     
     \ /
      Last update: 2012-03-19 04:51    [W:0.036 / U:5.036 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site