Re: [PATCH] writeback: reset inode dirty time when adding it back to empty s_dirty list

Ian Kent
2009-03-24T15:14:44Z

Jeff Layton wrote:
> On Tue, 24 Mar 2009 10:28:06 -0400
> Jeff Layton <jlayton@redhat.com> wrote:
>
>> On Tue, 24 Mar 2009 21:57:20 +0800
>> Wu Fengguang <fengguang.wu@intel.com> wrote:
>>
>>> Hi Jeff,
>>>
>>> On Mon, Mar 23, 2009 at 04:30:33PM -0400, Jeff Layton wrote:
>>>> This may be a problem on other filesystems too, but the reproducer I
>>>> have involves NFS.
>>>>
>>>> On NFS, the __mark_inode_dirty() call after writing back the inode is
>>>> done in the rpc_release handler for COMMIT calls. This call is done
>>>> asynchronously after the call completes.
>>>>
>>>> Because there's no real coordination between __mark_inode_dirty() and
>>>> __sync_single_inode(), it's often the case that these two calls will
>>>> race and __mark_inode_dirty() will get called while I_SYNC is still set.
>>>> When this happens, __sync_single_inode() should detect that the inode
>>>> was redirtied while we were flushing it and call redirty_tail() to put
>>>> it back on the s_dirty list.
>>>>
>>>> When redirty_tail() puts it back on the list, it only resets the
>>>> dirtied_when value if it's necessary to maintain the list order. Given
>>>> the right situation (the right I/O patterns and a lot of luck), this
>>>> could result in dirtied_when never getting updated on an inode that's
>>>> constantly being redirtied while pdflush is writing it back.
>>>>
>>>> Since dirtied_when is based on jiffies, it's possible for it to persist
>>>> across 2 sign-bit flips of jiffies. When that happens, the time_after()
>>>> check in sync_sb_inodes no longer works correctly and writeouts by
>>>> pdflush of this inode and any inodes after it on the list stop.
>>>>
>>>> This patch fixes this by resetting the dirtied_when value on an inode
>>>> when we're adding it back onto an empty s_dirty list. Since we generally
>>>> write inodes from oldest to newest dirtied_when values, this has the
>>>> effect of making it so that these inodes don't end up with dirtied_when
>>>> values that are frozen.
>>>>
>>>> I've also taken the liberty of fixing up the comments a bit and changed
>>>> the !time_after_eq() check in redirty_tail to be time_before(). That
>>>> should be functionally equivalent but I think it's more readable.
>>>>
>>>> I wish this were just a theoretical problem, but we've had a customer
>>>> hit a variant of it in an older kernel. Newer upstream kernels have a
>>>> number of changes that make this problem less likely. As best I can tell
>>>> though, there is nothing that really prevents it.
>>>>
>>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>>> ---
>>>> fs/fs-writeback.c | 22 +++++++++++++++++-----
>>>> 1 files changed, 17 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
>>>> index e3fe991..bd2a7ff 100644
>>>> --- a/fs/fs-writeback.c
>>>> +++ b/fs/fs-writeback.c
>>>> @@ -184,19 +184,31 @@ static int write_inode(struct inode *inode, int sync)
>>>> * furthest end of its superblock's dirty-inode list.
>>>> *
>>>> * Before stamping the inode's ->dirtied_when, we check to see whether it is
>>>> - * already the most-recently-dirtied inode on the s_dirty list. If that is
>>>> - * the case then the inode must have been redirtied while it was being written
>>>> - * out and we don't reset its dirtied_when.
>>>> + * "newer" or equal to that of the most-recently-dirtied inode on the s_dirty
>>>> + * list. If that is the case then we don't need to restamp it to maintain the
>>>> + * order of the list.
>>>> + *
>>>> + * If s_dirty is empty however, then we need to go ahead and update
>>>> + * dirtied_when for the inode. Not doing so will mean that inodes that are
>>>> + * constantly being redirtied can end up with "stuck" dirtied_when values if
>>>> + * they happen to consistently be the first one to go back on the list.
>>>> + *
>>>> + * Since we're using jiffies values in that field, letting dirtied_when grow
>>>> + * too old will be problematic if jiffies wraps. It may also be causing
>>>> + * pdflush to flush the inode too often since it'll always look like it was
>>>> + * dirtied a long time ago.
>>>> */
>>>> static void redirty_tail(struct inode *inode)
>>>> {
>>>> struct super_block *sb = inode->i_sb;
>>>>
>>>> - if (!list_empty(&sb->s_dirty)) {
>>>> + if (list_empty(&sb->s_dirty)) {
>>>> + inode->dirtied_when = jiffies;
>>>> + } else {
>>>> struct inode *tail_inode;
>>>>
>>>> tail_inode = list_entry(sb->s_dirty.next, struct inode, i_list);
>>>> - if (!time_after_eq(inode->dirtied_when,
>>>> + if (time_before(inode->dirtied_when,
>>>> tail_inode->dirtied_when))
>>>> inode->dirtied_when = jiffies;
>>>> }
>>> I'm afraid you patch is equivalent to the following one.
>>> Because once the first inode's dirtied_when is set to jiffies,
>>> in order to keep the list in order, the following ones (mostly)
>>> will also be updated. A domino effect.
>>>
>>> Thanks,
>>> Fengguang
>>>
>> Good point. One of our other engineers proposed a similar patch
>> originally. I considered it but wasn't clear whether there could be a
>> situation where unconditionally resetting dirtied_when would be a
>> problem. Now that I think about it though, I think you're right...
>>
>> So maybe something like the patch below is the right thing to do? Or,
>> maybe when we believe that the inode was fully cleaned and then
>> redirtied, we'd just unconditionally stamp dirtied_when. Something like
>> this maybe?
>>
>> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
>> index bd2a7ff..596c96e 100644
>> --- a/fs/fs-writeback.c
>> +++ b/fs/fs-writeback.c
>> @@ -364,7 +364,8 @@ __sync_single_inode(struct inode *inode, struct writeback_control *wbc)
>> * Someone redirtied the inode while were writing back
>> * the pages.
>> */
>> - redirty_tail(inode);
>> + inode->dirtied_when = jiffies;
>> + list_move(&inode->i_list, &sb->s_dirty);
>> } else if (atomic_read(&inode->i_count)) {
>> /*
>> * The inode is clean, inuse
>
> Hmm...though it is still possible that you could consistently race in
> such a way that after writepages(), I_DIRTY is never set but the
> PAGECACHE_TAG_DIRTY is still set on the mapping. And then we'd be back
> to the same problem of a stuck dirtied_when value.
>
> So maybe someone can explain to me why we take such great pains to
> preserve the dirtied_when value when we're putting the inode back on
> the tail of s_dirty? Why not just unconditionally reset it?

I think that redirty_tail() is the best place for this as it is a
central location where dirtied_when can be updated. Then all we have to
worry about is making sure it is called from all the locations needed.

I'm not sure that removing the comment is a good idea (the Wu Fengguang
patch) but it probably needs to be revised to explain why dirtied_when
is forcing a rewrite of the list entry times.

Ian