On Sun 04-09-11 05:35:27, Wu Fengguang wrote:
> On Sat, Sep 03, 2011 at 09:13:15AM +0800, Jan Kara wrote:
> > On Sat 27-08-11 21:58:25, Wu Fengguang wrote:
> > > Christoph,
> > >
> > > On Sat, Aug 27, 2011 at 02:14:09PM +0800, Christoph Hellwig wrote:
> > > > Right now ->write_inode has no way to safely return a EAGAIN without explicitly
> > > > redirtying the inode, as we would lose the dirty state otherwise. Most
> > > > filesystems get this wrong, but XFS makes heavy use of it to avoid blocking
> > > > the flusher thread when ->write_inode hits contentended inode locks. A
> > > > contended ilock is something XFS can hit very easibly when extending files, as
> > > > the data I/O completion handler takes the lock to update the size, and the
> > > > ->write_inode call can race with it fairly easily if writing enough data
> > > > in one go so that the completion for the first write come in just before
> > > > we call ->write_inode.
> > > >
> > > > Change the handling of this case to use requeue_io for a quick retry instead
> > > > of redirty_tail, which keeps moving out the dirtied_when data and thus keeps
> > > > delaying the writeout more and more with every failed attempt to get the lock.
> > >
> > > Yeah redirty_tail() does have the problem of possibly delay inodes for
> > > too long time. However, you know requeue_io() always has the danger of
> > > triggering busy wb_writeback() loops in corner cases.
> > >
> > > For example, nfs_write_inode()/nfs_commit_unstable_pages() often
> > > redirty the inode without doing anything (hence no any progress, a
> > > prerequisite for busy loops) depending on the in flight writes, which
> > > unfortunately further depends on _external_ network/server states..
> > > That means some stalled network/sever state could lead to busy loops
> > > in NFS clients.
> > >
> > > The alternative solution may be to firstly apply the attached patch,
> > > and change this one to:
> > >
> > > - redirty_tail(inode, wb);
> > > + requeue_io_wait(inode, wb);
> > But your patch doesn't solve the busyloop when the problematic inodes are
> > the only ones under writeback, does it? Then b_more_io and b_more_io_wait
> > are effectively the same if I understand it right.
>
> The difference lies in the
>
> /*
> * No more inodes for IO, bail
> */
> if (list_empty(&wb->b_more_io))
> break;
>
> check in wb_writeback(). So when what's left are all b_more_io_wait
> inodes, the above check will take effect and break the loop. This is
> the tricky point of the patch: it relies on the code not touched by
> the patch to work. I've updated the changelog to explain this.
Argh, that's really subtle! Two points here.
1) We will immediately retry inodes in b_more_io_wait list because of
if (progress)
continue;
check.
2) The time writeout will be delayed is only <=dirty_expire_centisecs but
can be arbitrarily small if someone submits more work. Also if
wb_writeback() was called from wb_do_writeback(), we will retry
b_more_io_wait inodes twice immediately because of
wb_check_old_data_flush() and wb_check_background_flush() calls.
> > I think that busylooping in cases like these could be fixed improving the
> > busyloop prevention at the end of the loop in wb_writeback(). Maybe we
> > could just take a short nap before continuting with writeback instead of /
> > in addition to waiting for inode writeback. What do you think?
>
> That's a reasonable robust option, however at the cost of keeping the
> writeback code in some ambiguous state ;)
What do you exactly mean by ambiguous state? I don't see anything
ambiguous in waiting for a jiffie or so. Not that I'd be completely happy
about "just wait for a while and see if things are better" but your
solution does not seem ideal either...
Honza
> ---
> Subject: writeback: introduce queue b_more_io_wait
> Date: Sun Jul 31 18:44:44 CST 2011
>
> The problem is, redirty_tail() may update i_dirtied_when and result in
> 30s max delay. If redirty_tail() is called often enough, some inode may
> even be delayed for ever.
>
> So introduce the b_more_io_wait queue to park inodes that for some
> reason cannot be synced immediately. The inodes will be sent to b_io at
> the next b_io refill time, however won't be busy retried as b_more_io:
> when the redirtied inodes are all in b_more_io_wait, wb_writeback() will
> see empty b_more_io and hence break out of the loop.
>
> The new data flow after converting all redirty_tail() calls to
> requeue_io_wait():
>
> b_dirty --> b_io --> b_more_io/b_more_io_wait --+
> ^ |
> | |
> +----------------------------------+
>
> Cc: Jan Kara <jack@xxxxxxx>
> Cc: Dave Chinner <david@xxxxxxxxxxxxx>
> Signed-off-by: Wu Fengguang <fengguang.wu@xxxxxxxxx>
> ---
> fs/fs-writeback.c | 10 ++++++++++
> include/linux/backing-dev.h | 8 +++++---
> mm/backing-dev.c | 10 ++++++++--
> 3 files changed, 23 insertions(+), 5 deletions(-)
>
> --- linux-next.orig/fs/fs-writeback.c 2011-08-27 15:28:27.000000000 +0800
> +++ linux-next/fs/fs-writeback.c 2011-08-27 15:45:10.000000000 +0800
> @@ -220,6 +220,15 @@ static void requeue_io(struct inode *ino
> list_move(&inode->i_wb_list, &wb->b_more_io);
> }
>
> +/*
> + * The inode should be retried in an opportunistic way.
> + */
> +static void requeue_io_wait(struct inode *inode, struct bdi_writeback *wb)
> +{
> + assert_spin_locked(&wb->list_lock);
> + list_move(&inode->i_wb_list, &wb->b_more_io_wait);
> +}
> +
> static void inode_sync_complete(struct inode *inode)
> {
> /*
> @@ -307,6 +316,7 @@ static void queue_io(struct bdi_writebac
> int moved;
> assert_spin_locked(&wb->list_lock);
> list_splice_init(&wb->b_more_io, &wb->b_io);
> + list_splice_init(&wb->b_more_io_wait, &wb->b_io);
> moved = move_expired_inodes(&wb->b_dirty, &wb->b_io, older_than_this);
> trace_writeback_queue_io(wb, older_than_this, moved);
> }
> --- linux-next.orig/include/linux/backing-dev.h 2011-08-26 19:27:20.000000000 +0800
> +++ linux-next/include/linux/backing-dev.h 2011-08-27 15:45:10.000000000 +0800
> @@ -59,6 +59,7 @@ struct bdi_writeback {
> struct list_head b_dirty; /* dirty inodes */
> struct list_head b_io; /* parked for writeback */
> struct list_head b_more_io; /* parked for more writeback */
> + struct list_head b_more_io_wait;/* opportunistic retry io */
> spinlock_t list_lock; /* protects the b_* lists */
> };
>
> @@ -129,9 +130,10 @@ extern struct list_head bdi_pending_list
>
> static inline int wb_has_dirty_io(struct bdi_writeback *wb)
> {
> - return !list_empty(&wb->b_dirty) ||
> - !list_empty(&wb->b_io) ||
> - !list_empty(&wb->b_more_io);
> + return !list_empty(&wb->b_dirty) ||
> + !list_empty(&wb->b_io) ||
> + !list_empty(&wb->b_more_io) ||
> + !list_empty(&wb->b_more_io_wait);
> }
>
> static inline void __add_bdi_stat(struct backing_dev_info *bdi,
> --- linux-next.orig/mm/backing-dev.c 2011-08-26 19:27:20.000000000 +0800
> +++ linux-next/mm/backing-dev.c 2011-08-27 15:45:10.000000000 +0800
> @@ -74,10 +74,10 @@ static int bdi_debug_stats_show(struct s
> unsigned long background_thresh;
> unsigned long dirty_thresh;
> unsigned long bdi_thresh;
> - unsigned long nr_dirty, nr_io, nr_more_io;
> + unsigned long nr_dirty, nr_io, nr_more_io, nr_more_io_wait;
> struct inode *inode;
>
> - nr_dirty = nr_io = nr_more_io = 0;
> + nr_dirty = nr_io = nr_more_io = nr_more_io_wait = 0;
> spin_lock(&wb->list_lock);
> list_for_each_entry(inode, &wb->b_dirty, i_wb_list)
> nr_dirty++;
> @@ -85,6 +85,8 @@ static int bdi_debug_stats_show(struct s
> nr_io++;
> list_for_each_entry(inode, &wb->b_more_io, i_wb_list)
> nr_more_io++;
> + list_for_each_entry(inode, &wb->b_more_io_wait, i_wb_list)
> + nr_more_io_wait++;
> spin_unlock(&wb->list_lock);
>
> global_dirty_limits(&background_thresh, &dirty_thresh);
> @@ -103,6 +105,7 @@ static int bdi_debug_stats_show(struct s
> "b_dirty: %10lu\n"
> "b_io: %10lu\n"
> "b_more_io: %10lu\n"
> + "b_more_io_wait: %10lu\n"
> "bdi_list: %10u\n"
> "state: %10lx\n",
> (unsigned long) K(bdi_stat(bdi, BDI_WRITEBACK)),
> @@ -116,6 +119,7 @@ static int bdi_debug_stats_show(struct s
> nr_dirty,
> nr_io,
> nr_more_io,
> + nr_more_io_wait,
> !list_empty(&bdi->bdi_list), bdi->state);
> #undef K
>
> @@ -635,6 +639,7 @@ static void bdi_wb_init(struct bdi_write
> INIT_LIST_HEAD(&wb->b_dirty);
> INIT_LIST_HEAD(&wb->b_io);
> INIT_LIST_HEAD(&wb->b_more_io);
> + INIT_LIST_HEAD(&wb->b_more_io_wait);
> spin_lock_init(&wb->list_lock);
> setup_timer(&wb->wakeup_timer, wakeup_timer_fn, (unsigned long)bdi);
> }
> @@ -701,6 +706,7 @@ void bdi_destroy(struct backing_dev_info
> list_splice(&bdi->wb.b_dirty, &dst->b_dirty);
> list_splice(&bdi->wb.b_io, &dst->b_io);
> list_splice(&bdi->wb.b_more_io, &dst->b_more_io);
> + list_splice(&bdi->wb.b_more_io_wait, &dst->b_more_io_wait);
> spin_unlock(&bdi->wb.list_lock);
> spin_unlock(&dst->list_lock);
> }
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
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
