linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Martin Bligh <mbligh@google.com>
To: Wu Fengguang <fengguang.wu@intel.com>
Cc: Chad Talbott <ctalbott@google.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	Michael Rubin <mrubin@google.com>,
	Andrew Morton <akpm@google.com>,
	"sandeen@redhat.com" <sandeen@redhat.com>,
	Michael Davidson <md@google.com>
Subject: Re: Bug in kernel 2.6.31, Slow wb_kupdate writeout
Date: Thu, 30 Jul 2009 13:33:09 -0700	[thread overview]
Message-ID: <33307c790907301333i28b571eat29460164d558d370@mail.gmail.com> (raw)
In-Reply-To: <20090730031927.GA17669@localhost>

(BTW: background ... I'm not picking through this code for fun, I'm
trying to debug writeback problems introduced in our new kernel
that are affecting Google production workloads ;-))

>> Well, I see two problems. One is that we set more_io based on
>> whether s_more_io is empty or not before we finish the loop.
>> I can't see how this can be correct, especially as there can be
>> other concurrent writers. So somehow we need to check when
>> we exit the loop, not during it.
>
> It is correct inside the loop, however with some overheads.
>
> We put it inside the loop because sometimes the whole filesystem is
> skipped and we shall not set more_io on them whether or not s_more_io
> is empty.

My point was that you're setting more_io based on a condition
at a point in time that isn't when you return to the caller.

By the time you return to the caller (after several more loops
iterations), that condition may no longer be true.

One other way to address that would to be only to set if if we're
about to fall off the end of the loop, ie change it to:

if (!list_empty(&sb->s_more_io) && list_empty(&sb->s_io))
       wbc->more_io = 1;

>> The other is that we're saying we are setting more_io when
>> nr_to_write is <=0 ... but we only really check it when
>> nr_to_write is > 0 ... I can't see how this can be useful?
>
> That's the caller's fault - I guess the logic was changed a bit by
> Jens in linux-next. I noticed this just now. It shall be fixed.

I am guessing you're setting more_io here because we're stopping
because our slice expired, presumably without us completing
all the io there was to do? That doesn't seem entirely accurate,
we could have finished all the pending IO (particularly given that
we can go over nr_to_write somewhat and send it negative).
Hence, I though that checking whether s_more_io and s_io were
empty at the time of return might be a more accurate check,
but on the other hand they are shared lists.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

  reply	other threads:[~2009-07-30 20:33 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-07-28 19:11 Bug in kernel 2.6.31, Slow wb_kupdate writeout Chad Talbott
2009-07-28 21:49 ` Martin Bligh
2009-07-29  7:15   ` Martin Bligh
2009-07-29 11:43     ` Wu Fengguang
2009-07-29 14:11       ` Martin Bligh
2009-07-30  1:06         ` Wu Fengguang
2009-07-30  1:12           ` Martin Bligh
2009-07-30  1:57             ` Wu Fengguang
2009-07-30  2:59               ` Martin Bligh
2009-07-30  4:08                 ` Wu Fengguang
2009-07-30 19:55                   ` Martin Bligh
2009-08-01  2:02                     ` Wu Fengguang
2009-07-30  0:19       ` Martin Bligh
2009-07-30  1:28         ` Martin Bligh
2009-07-30  2:09           ` Wu Fengguang
2009-07-30  2:57             ` Martin Bligh
2009-07-30  3:19               ` Wu Fengguang
2009-07-30 20:33                 ` Martin Bligh [this message]
2009-08-01  2:58                   ` Wu Fengguang
2009-08-01  4:10                   ` Wu Fengguang
2009-07-30  1:49         ` Wu Fengguang
2009-07-30 21:39 ` Jens Axboe
2009-07-30 22:01   ` Martin Bligh
2009-07-30 22:17     ` Jens Axboe
2009-07-30 22:34       ` Martin Bligh
2009-07-30 22:43         ` Jens Axboe
2009-07-30 22:48           ` Martin Bligh
2009-07-31  7:50             ` Peter Zijlstra
2009-08-01  4:03             ` Wu Fengguang
2009-08-01  4:53               ` Wu Fengguang
2009-08-01  5:03                 ` Wu Fengguang
2009-08-01  4:02         ` Wu Fengguang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=33307c790907301333i28b571eat29460164d558d370@mail.gmail.com \
    --to=mbligh@google.com \
    --cc=akpm@google.com \
    --cc=ctalbott@google.com \
    --cc=fengguang.wu@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=md@google.com \
    --cc=mrubin@google.com \
    --cc=sandeen@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).