From: Sabrina Dubroca <sd@queasysnail.net>
To: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Jens Axboe <axboe@kernel.dk>, Theodore Ts'o <tytso@mit.edu>,
linux-next@vger.kernel.org, linux-kernel@vger.kernel.org,
Tejun Heo <tj@kernel.org>, Jeremiah Mahler <jmmahler@gmail.com>
Subject: Re: linux-next: manual merge of the block tree with the ext4 tree
Date: Sat, 29 Nov 2014 11:08:33 +0100 [thread overview]
Message-ID: <20141129100833.GA31892@kria> (raw)
In-Reply-To: <20141127145347.0083c0bc@canb.auug.org.au>
Hello,
[adding Jeremiah Mahler to CC]
2014-11-27, 14:53:47 +1100, Stephen Rothwell wrote:
> Hi Jens,
>
> Today's linux-next merge of the block tree got a conflict in
> fs/fs-writeback.c between commit ef7fdf5e8c87 ("vfs: add support for a
> lazytime mount option") from the ext4 tree and commit 9c6ac78eb352
> ("writeback: fix a subtle race condition in I_DIRTY clearing") from the
> block tree.
>
> I fixed it up (I took a guess, plese check - see below) and can carry
> the fix as necessary (no action is required).
>
> --
> Cheers,
> Stephen Rothwell sfr@canb.auug.org.au
>
> diff --cc fs/fs-writeback.c
> index 3d87174408ae,2d609a5fbfea..000000000000
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@@ -482,14 -479,30 +482,30 @@@ __writeback_single_inode(struct inode *
> * write_inode()
> */
> spin_lock(&inode->i_lock);
> - /* Clear I_DIRTY_PAGES if we've written out all dirty pages */
> - if (!mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
> - inode->i_state &= ~I_DIRTY_PAGES;
> +
> - dirty = inode->i_state & I_DIRTY;
> - inode->i_state &= ~I_DIRTY;
> + dirty = inode->i_state & I_DIRTY_INODE;
> + inode->i_state &= ~I_DIRTY_INODE;
> +
> + /*
> + * Paired with smp_mb() in __mark_inode_dirty(). This allows
> + * __mark_inode_dirty() to test i_state without grabbing i_lock -
> + * either they see the I_DIRTY bits cleared or we see the dirtied
> + * inode.
> + *
> + * I_DIRTY_PAGES is always cleared together above even if @mapping
> + * still has dirty pages. The flag is reinstated after smp_mb() if
> + * necessary. This guarantees that either __mark_inode_dirty()
> + * sees clear I_DIRTY_PAGES or we see PAGECACHE_TAG_DIRTY.
> + */
> + smp_mb();
> +
> + if (mapping_tagged(mapping, PAGECACHE_TAG_DIRTY))
> + inode->i_state |= I_DIRTY_PAGES;
> +
> spin_unlock(&inode->i_lock);
> +
> /* Don't write the inode if only I_DIRTY_PAGES was set */
> - if (dirty & (I_DIRTY_SYNC | I_DIRTY_DATASYNC)) {
> + if (dirty) {
> int err = write_inode(inode, wbc);
> if (ret == 0)
> ret = err;
I think there's a problem in your fix, Stephen.
I'm getting hangs at boot (strangely -- in QEMU -- only when booting
via grub, not when using -kernel) and during shutdown. Jeremiah seems
to have the same problem and his bisection led to the merge commit:
https://lkml.org/lkml/2014/11/29/17
The following solves both issues for me. I think it makes sense given
the #defines from ef7fdf5e8c87, since Tejun intended to clear
I_DIRTY_PAGES.
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index b70e45f45afa..6b2510d97a0a 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -484,7 +484,7 @@ __writeback_single_inode(struct inode *inode, struct writeback_control *wbc)
spin_lock(&inode->i_lock);
dirty = inode->i_state & I_DIRTY_INODE;
- inode->i_state &= ~I_DIRTY_INODE;
+ inode->i_state &= ~I_DIRTY;
/*
* Paired with smp_mb() in __mark_inode_dirty(). This allows
--
Thanks,
Sabrina
next prev parent reply other threads:[~2014-11-29 10:08 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-27 3:53 linux-next: manual merge of the block tree with the ext4 tree Stephen Rothwell
2014-11-27 7:38 ` Christoph Hellwig
2014-11-29 10:08 ` Sabrina Dubroca [this message]
2014-11-29 17:38 ` Theodore Ts'o
2014-11-29 22:08 ` Jeremiah Mahler
-- strict thread matches above, loose matches on Subject: below --
2023-06-26 3:09 Stephen Rothwell
2023-06-26 8:00 ` Christoph Hellwig
2023-06-27 0:46 ` Stephen Rothwell
2023-06-13 2:54 Stephen Rothwell
2023-06-13 5:00 ` Christoph Hellwig
2016-07-15 4:33 Stephen Rothwell
2014-11-26 2:41 Stephen Rothwell
2010-09-21 4:04 Stephen Rothwell
2010-10-23 2:47 ` Stephen Rothwell
2010-04-29 3:51 Stephen Rothwell
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=20141129100833.GA31892@kria \
--to=sd@queasysnail.net \
--cc=axboe@kernel.dk \
--cc=jmmahler@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=sfr@canb.auug.org.au \
--cc=tj@kernel.org \
--cc=tytso@mit.edu \
/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).