LinuxLists.cc - [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

I found one more regression failure while running tests on the dev
branch. It doesn't fail every single time you run xfstests #270; in
my test setup, the test failure happens approximatrely one in three
times. Fortunately, "kvm-xfstests -c dioread_nolock
270,270,270,270,270,270,270,270,270" made it easy to bisect the
failure to commit 84c17543ab56 (ext4: move work from io_end to inode).

I've added this commit to fix it, and restarted running the regression
tests. Hopefully this will be the last fix up required before I send
a pull request to Linus....

- Ted

On Tue, Mar 19, 2013 at 09:29:19PM -0400, Theodore Ts'o wrote:
> Commit 84c17543ab56 (ext4: move work from io_end to inode) triggered a
> regression when running xfstest #270 when the file system is mounted
> with dioread_nolock.
>
> The problem is that after ext4_evict_inode() calls ext4_ioend_wait(),
> this guarantees that last io_end structure has been freed, but it does
> not guarantee that the workqueue structure, which was moved into the
> inode by commit 84c17543ab56, is actually finished. Once
> ext4_flush_completed_IO() calls ext4_free_io_end() on CPU #1, this
> will allow ext4_ioend_wait() to return on CPU #2, at which point the
> evict_inode() codepath can race against the workqueue code on CPU #1
> accessing EXT4_I(inode)->i_unwritten_work to find the next item of
> work to do.
>
> Fix this by calling flush_work() if the work structure is still
> pending in ext$_ioend_wait(). Also, move the call to
> ext4_ioend_wait() until after truncate_inode_pages() and
> filemap_write_and_wait() are called, to make sure all dirty pages have
> been written back and flushed from the page cache first.
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<c01dda6a>] cwq_activate_delayed_work+0x3b/0x7e
> *pdpt = 0000000030bc3001 *pde = 0000000000000000
> Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
> Modules linked in:
> Pid: 6, comm: kworker/u:0 Not tainted 3.8.0-rc3-00013-g84c1754-dirty #91 Bochs Bochs
> EIP: 0060:[<c01dda6a>] EFLAGS: 00010046 CPU: 0
> EIP is at cwq_activate_delayed_work+0x3b/0x7e
> EAX: 00000000 EBX: 00000000 ECX: f505fe54 EDX: 00000000
> ESI: ed5b697c EDI: 00000006 EBP: f64b7e8c ESP: f64b7e84
> DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> CR0: 8005003b CR2: 00000000 CR3: 30bc2000 CR4: 000006f0
> DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> DR6: ffff0ff0 DR7: 00000400
> Process kworker/u:0 (pid: 6, ti=f64b6000 task=f64b4160 task.ti=f64b6000)
> Stack:
> f505fe00 00000006 f64b7e9c c01de3d7 f6435540 00000003 f64b7efc c01def1d
> f6435540 00000002 00000000 0000008a c16d0808 c040a10b c16d07d8 c16d08b0
> f505fe00 c16d0780 00000000 00000000 ee153df4 c1ce4a30 c17d0e30 00000000
> Call Trace:
> [<c01de3d7>] cwq_dec_nr_in_flight+0x71/0xfb
> [<c01def1d>] process_one_work+0x5d8/0x637
> [<c040a10b>] ? ext4_end_bio+0x300/0x300
> [<c01e3105>] worker_thread+0x249/0x3ef
> [<c01ea317>] kthread+0xd8/0xeb
> [<c01e2ebc>] ? manage_workers+0x4bb/0x4bb
> [<c023a370>] ? trace_hardirqs_on+0x27/0x37
> [<c0f1b4b7>] ret_from_kernel_thread+0x1b/0x28
> [<c01ea23f>] ? __init_kthread_worker+0x71/0x71
> Code: 01 83 15 ac ff 6c c1 00 31 db 89 c6 8b 00 a8 04 74 12 89 c3 30 db 83 05 b0 ff 6c c1 01 83 15 b4 ff 6c c1 00 89 f0 e8 42 ff ff ff <8b> 13 89 f0 83 05 b8 ff 6c c1
> 6c c1 00 31 c9 83
> EIP: [<c01dda6a>] cwq_activate_delayed_work+0x3b/0x7e SS:ESP 0068:f64b7e84
> CR2: 0000000000000000
> ---[ end trace a1923229da53d8a4 ]---
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> Cc: Jan Kara <[email protected]>
...

2013-03-20 13:22:25

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On Tue 19-03-13 21:29:19, Ted Tso wrote:
> Commit 84c17543ab56 (ext4: move work from io_end to inode) triggered a
> regression when running xfstest #270 when the file system is mounted
> with dioread_nolock.
>
> The problem is that after ext4_evict_inode() calls ext4_ioend_wait(),
> this guarantees that last io_end structure has been freed, but it does
> not guarantee that the workqueue structure, which was moved into the
> inode by commit 84c17543ab56, is actually finished. Once
> ext4_flush_completed_IO() calls ext4_free_io_end() on CPU #1, this
> will allow ext4_ioend_wait() to return on CPU #2, at which point the
> evict_inode() codepath can race against the workqueue code on CPU #1
> accessing EXT4_I(inode)->i_unwritten_work to find the next item of
> work to do.
>
> Fix this by calling flush_work() if the work structure is still
> pending in ext$_ioend_wait(). Also, move the call to
> ext4_ioend_wait() until after truncate_inode_pages() and
> filemap_write_and_wait() are called, to make sure all dirty pages have
> been written back and flushed from the page cache first.
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<c01dda6a>] cwq_activate_delayed_work+0x3b/0x7e
> *pdpt = 0000000030bc3001 *pde = 0000000000000000
> Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
> Modules linked in:
> Pid: 6, comm: kworker/u:0 Not tainted 3.8.0-rc3-00013-g84c1754-dirty #91 Bochs Bochs
> EIP: 0060:[<c01dda6a>] EFLAGS: 00010046 CPU: 0
> EIP is at cwq_activate_delayed_work+0x3b/0x7e
> EAX: 00000000 EBX: 00000000 ECX: f505fe54 EDX: 00000000
> ESI: ed5b697c EDI: 00000006 EBP: f64b7e8c ESP: f64b7e84
> DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> CR0: 8005003b CR2: 00000000 CR3: 30bc2000 CR4: 000006f0
> DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> DR6: ffff0ff0 DR7: 00000400
> Process kworker/u:0 (pid: 6, ti=f64b6000 task=f64b4160 task.ti=f64b6000)
> Stack:
> f505fe00 00000006 f64b7e9c c01de3d7 f6435540 00000003 f64b7efc c01def1d
> f6435540 00000002 00000000 0000008a c16d0808 c040a10b c16d07d8 c16d08b0
> f505fe00 c16d0780 00000000 00000000 ee153df4 c1ce4a30 c17d0e30 00000000
> Call Trace:
> [<c01de3d7>] cwq_dec_nr_in_flight+0x71/0xfb
> [<c01def1d>] process_one_work+0x5d8/0x637
> [<c040a10b>] ? ext4_end_bio+0x300/0x300
> [<c01e3105>] worker_thread+0x249/0x3ef
> [<c01ea317>] kthread+0xd8/0xeb
> [<c01e2ebc>] ? manage_workers+0x4bb/0x4bb
> [<c023a370>] ? trace_hardirqs_on+0x27/0x37
> [<c0f1b4b7>] ret_from_kernel_thread+0x1b/0x28
> [<c01ea23f>] ? __init_kthread_worker+0x71/0x71
> Code: 01 83 15 ac ff 6c c1 00 31 db 89 c6 8b 00 a8 04 74 12 89 c3 30 db 83 05 b0 ff 6c c1 01 83 15 b4 ff 6c c1 00 89 f0 e8 42 ff ff ff <8b> 13 89 f0 83 05 b8 ff 6c c1
> 6c c1 00 31 c9 83
> EIP: [<c01dda6a>] cwq_activate_delayed_work+0x3b/0x7e SS:ESP 0068:f64b7e84
> CR2: 0000000000000000
> ---[ end trace a1923229da53d8a4 ]---
>
> Signed-off-by: "Theodore Ts'o" <[email protected]>
> Cc: Jan Kara <[email protected]>
Good catch. Thanks for fixing this. Just one question below:

> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 809b310..8a004a4 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -50,11 +50,21 @@ void ext4_exit_pageio(void)
> kmem_cache_destroy(io_page_cachep);
> }
>
> +/*
> + * Called by ext4_evict_inode() to make sure there are no pending I/O
> + * completion work left to do.
> + */
> void ext4_ioend_wait(struct inode *inode)
> {
> wait_queue_head_t *wq = ext4_ioend_wq(inode);
>
> wait_event(*wq, (atomic_read(&EXT4_I(inode)->i_ioend_count) == 0));
> + /*
> + * We need to make sure the work structure is finished before
> + * we let the inode get destroyed by ext4_evict_inode()
> + */
> + if (work_pending(&EXT4_I(inode)->i_unwritten_work))
> + flush_work(&EXT4_I(inode)->i_unwritten_work);
> }
Won't it be more logical to use cancel_work_sync() here?

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2013-03-20 13:37:54

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On Wed, Mar 20, 2013 at 02:22:23PM +0100, Jan Kara wrote:
> > + if (work_pending(&EXT4_I(inode)->i_unwritten_work))
> > + flush_work(&EXT4_I(inode)->i_unwritten_work);
> > }
> Won't it be more logical to use cancel_work_sync() here?

Hmm.... yes, probably, but then ext4_ioend_wait() can only be safely
used by ext4_evict_inode(). I'll make the change, but I'll also make
a comment to this effect. (No one else is using it now, but if there
was ever a need to use it while the inode was in use, using
cancel_work_sync() would be highly dagernous/racy. That being said, I
can't really think of a good use case other than evict_inode path, so
it seems fine to make this change.)

- Ted

2013-03-20 13:42:28

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On Wed 20-03-13 09:37:51, Ted Tso wrote:
> On Wed, Mar 20, 2013 at 02:22:23PM +0100, Jan Kara wrote:
> > > + if (work_pending(&EXT4_I(inode)->i_unwritten_work))
> > > + flush_work(&EXT4_I(inode)->i_unwritten_work);
> > > }
> > Won't it be more logical to use cancel_work_sync() here?
>
> Hmm.... yes, probably, but then ext4_ioend_wait() can only be safely
> used by ext4_evict_inode(). I'll make the change, but I'll also make
> a comment to this effect. (No one else is using it now, but if there
> was ever a need to use it while the inode was in use, using
> cancel_work_sync() would be highly dagernous/racy. That being said, I
> can't really think of a good use case other than evict_inode path, so
> it seems fine to make this change.)
Yeah, we can possibly rename the function or maybe even just inline it in
ext4_evict_inode?

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2013-03-20 13:51:44

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On Wed, Mar 20, 2013 at 02:42:26PM +0100, Jan Kara wrote:
> Yeah, we can possibly rename the function or maybe even just inline it in
> ext4_evict_inode?

Good idea, I'll rename it to ext4_ioend_shutdown(). Given that the
rest of the logic is in page_io.c, it seemed better to leave that
function there than to inline it in ext4_evict_inode().

- Ted

2013-03-20 14:15:29

by Eric Sandeen

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On 3/19/13 8:29 PM, Theodore Ts'o wrote:
> Commit 84c17543ab56 (ext4: move work from io_end to inode) triggered a
> regression when running xfstest #270 when the file system is mounted
> with dioread_nolock.

As an aside, is there any reason to have "dioread_nolock" as an option
at this point? If it works now, would you ever *not* want it?

(granted it doesn't work with some journaling options etc, but that
behavior could be automatic, w/o the need for special mount options).

-Eric

2013-03-20 14:45:29

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On Wed, Mar 20, 2013 at 09:14:42AM -0500, Eric Sandeen wrote:
>
> As an aside, is there any reason to have "dioread_nolock" as an option
> at this point? If it works now, would you ever *not* want it?
>
> (granted it doesn't work with some journaling options etc, but that
> behavior could be automatic, w/o the need for special mount options).

The primary restriction is that diread_nolock doesn't work when fs
block size != page size. If your proposal is that we automatically
enable diread_nolock when we can use it safely, that's definitely
something to consider for the next merge window.

My long range plan/hope is that we eventually be able to use the
extent status tree so that we do allocating writes, we first (a)
allocate the blocks, and mark them as in use as far as the mballoc
data structures are concerned, but we do _not_ mark them as in use in
the on-disk allocation bitmaps, then (b) we write the data blocks, and
then triggered by the block I/O completion, (c) in a single journal
trnasaction, we update the allocation bitmaps, update the inode's
extent tree, and update the inode's i_size field.

This is different from the dioread_nolock approach in that we're not
initially inserting the blocks in the extent tree as uninitialized,
and then convert the extent tree entries from uninit to init after the
I/O completion.

If we get to this long-term nirvana, then (1) we can eliminate the
data=writeback vs data=ordered distiction, since we'll have the safety
benefits of data=ordered while still having the performance
characteristics of data=writeback, and (2) we can eliminate
diread_nolock, since this approach should also obviate needing to take
the read lock on the direct I/O read path. I also think this approach
in the long term will be simpler and faster, since we don't have
modify the extent tree, and start a journal transaction, before we
write the data blocks.

- Ted

2013-03-20 20:13:58

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On Wed 20-03-13 10:45:23, Ted Tso wrote:
> On Wed, Mar 20, 2013 at 09:14:42AM -0500, Eric Sandeen wrote:
> >
> > As an aside, is there any reason to have "dioread_nolock" as an option
> > at this point? If it works now, would you ever *not* want it?
> >
> > (granted it doesn't work with some journaling options etc, but that
> > behavior could be automatic, w/o the need for special mount options).
>
> The primary restriction is that diread_nolock doesn't work when fs
> block size != page size. If your proposal is that we automatically
> enable diread_nolock when we can use it safely, that's definitely
> something to consider for the next merge window.
>
> My long range plan/hope is that we eventually be able to use the
> extent status tree so that we do allocating writes, we first (a)
> allocate the blocks, and mark them as in use as far as the mballoc
> data structures are concerned, but we do _not_ mark them as in use in
> the on-disk allocation bitmaps, then (b) we write the data blocks, and
> then triggered by the block I/O completion, (c) in a single journal
> trnasaction, we update the allocation bitmaps, update the inode's
> extent tree, and update the inode's i_size field.
>
> This is different from the dioread_nolock approach in that we're not
> initially inserting the blocks in the extent tree as uninitialized,
> and then convert the extent tree entries from uninit to init after the
> I/O completion.
>
> If we get to this long-term nirvana, then (1) we can eliminate the
> data=writeback vs data=ordered distiction, since we'll have the safety
> benefits of data=ordered while still having the performance
> characteristics of data=writeback, and (2) we can eliminate
> diread_nolock, since this approach should also obviate needing to take
> the read lock on the direct I/O read path.
But this will be somewhat tricky because when we have racing buffered
write and DIO read to the same block, we have to make sure that DIO read
ignores the information in the extent status tree because data isn't
written to the blocks yet. Umm, maybe we could just mark the extent as
unwritten in the extent status tree (without having anything on disk) and
this should make DIO read work. That sounds like a nice optimization.

> I also think this approach
> in the long term will be simpler and faster, since we don't have
> modify the extent tree, and start a journal transaction, before we
> write the data blocks.
Yeah, it should be faster because we will need to perform some extent ops
only in memory and not on disk.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2013-03-26 05:37:11

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

Sorry for the late reply.

On Wed, Mar 20, 2013 at 10:45:23AM -0400, Theodore Ts'o wrote:
> On Wed, Mar 20, 2013 at 09:14:42AM -0500, Eric Sandeen wrote:
> >
> > As an aside, is there any reason to have "dioread_nolock" as an option
> > at this point? If it works now, would you ever *not* want it?
> >
> > (granted it doesn't work with some journaling options etc, but that
> > behavior could be automatic, w/o the need for special mount options).
>
> The primary restriction is that diread_nolock doesn't work when fs
> block size != page size. If your proposal is that we automatically
> enable diread_nolock when we can use it safely, that's definitely
> something to consider for the next merge window.

Yes, I also think we can automatically enable dioread_nolock because it
brings us some benefits.

BTW, I think there is an minor improvement for dio overwrite codepath
with indirect-based file. We don't need to take i_mutex in this
condition just as we have done for extent-based file. If a user mounts
a ext2/3 file system with a ext4 kernel modules, he/she could get a
lower latency. But it seems that it would break dio semantic in ext2/3.
Currently in ext2/3 if we issue a overwrite dio and then issue a read
dio. We will always read the latest data because we wait on i_mutex
lock. But after parallelizing overwite dio, this semantic might breaks.
I re-read this doc but it seems that it doesn't describe this case. Do
we need to keep this semantic?

>
> My long range plan/hope is that we eventually be able to use the
> extent status tree so that we do allocating writes, we first (a)
> allocate the blocks, and mark them as in use as far as the mballoc
> data structures are concerned, but we do _not_ mark them as in use in
> the on-disk allocation bitmaps, then (b) we write the data blocks, and
> then triggered by the block I/O completion, (c) in a single journal
> trnasaction, we update the allocation bitmaps, update the inode's
> extent tree, and update the inode's i_size field.
>
> This is different from the dioread_nolock approach in that we're not
> initially inserting the blocks in the extent tree as uninitialized,
> and then convert the extent tree entries from uninit to init after the
> I/O completion.

Yes, this approach is better. I am happy to work on this.

Regards,
- Zheng

2013-03-26 05:46:24

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On Tue, Mar 26, 2013 at 01:52:51PM +0800, Zheng Liu wrote:
> Sorry for the late reply.
>
> On Wed, Mar 20, 2013 at 10:45:23AM -0400, Theodore Ts'o wrote:
> > On Wed, Mar 20, 2013 at 09:14:42AM -0500, Eric Sandeen wrote:
> > >
> > > As an aside, is there any reason to have "dioread_nolock" as an option
> > > at this point? If it works now, would you ever *not* want it?
> > >
> > > (granted it doesn't work with some journaling options etc, but that
> > > behavior could be automatic, w/o the need for special mount options).
> >
> > The primary restriction is that diread_nolock doesn't work when fs
> > block size != page size. If your proposal is that we automatically
> > enable diread_nolock when we can use it safely, that's definitely
> > something to consider for the next merge window.
>
> Yes, I also think we can automatically enable dioread_nolock because it
> brings us some benefits.
>
> BTW, I think there is an minor improvement for dio overwrite codepath
> with indirect-based file. We don't need to take i_mutex in this
> condition just as we have done for extent-based file. If a user mounts
> a ext2/3 file system with a ext4 kernel modules, he/she could get a
> lower latency. But it seems that it would break dio semantic in ext2/3.
> Currently in ext2/3 if we issue a overwrite dio and then issue a read
> dio. We will always read the latest data because we wait on i_mutex
> lock. But after parallelizing overwite dio, this semantic might breaks.
> I re-read this doc but it seems that it doesn't describe this case.
^^^

Ah, sorry, I forgot to paste the link here.
https://ext4.wiki.kernel.org/index.php/Clarifying_Direct_IO%27s_Semantics

Regards,
- Zheng

2013-03-26 20:34:08

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

On Tue 26-03-13 13:52:51, Zheng Liu wrote:
> Sorry for the late reply.
>
> On Wed, Mar 20, 2013 at 10:45:23AM -0400, Theodore Ts'o wrote:
> > On Wed, Mar 20, 2013 at 09:14:42AM -0500, Eric Sandeen wrote:
> > >
> > > As an aside, is there any reason to have "dioread_nolock" as an option
> > > at this point? If it works now, would you ever *not* want it?
> > >
> > > (granted it doesn't work with some journaling options etc, but that
> > > behavior could be automatic, w/o the need for special mount options).
> >
> > The primary restriction is that diread_nolock doesn't work when fs
> > block size != page size. If your proposal is that we automatically
> > enable diread_nolock when we can use it safely, that's definitely
> > something to consider for the next merge window.
>
> Yes, I also think we can automatically enable dioread_nolock because it
> brings us some benefits.
But isn't there also some overhead due to buffered writes having to go
through uninit->init conversion? Plus there's this potential deadlock in
dioread_nolock code (http://www.spinics.net/lists/linux-ext4/msg36569.html)
which I'm not sure how to fix yet...

> BTW, I think there is an minor improvement for dio overwrite codepath
> with indirect-based file. We don't need to take i_mutex in this
> condition just as we have done for extent-based file. If a user mounts
> a ext2/3 file system with a ext4 kernel modules, he/she could get a
> lower latency. But it seems that it would break dio semantic in ext2/3.
> Currently in ext2/3 if we issue a overwrite dio and then issue a read
> dio. We will always read the latest data because we wait on i_mutex
> lock. But after parallelizing overwite dio, this semantic might breaks.
> I re-read this doc but it seems that it doesn't describe this case. Do
> we need to keep this semantic?
I'm not sure but also I don't think it's important to optimize that
special case.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2013-03-27 02:57:59

[permalink] [raw]

Subject: Re: [PATCH] ext4: fix ext4_evict_inode() racing against workqueue processing code

2013-03-29 07:17:12