* [PATCH] ext4: drop file_update_time from ext4_dax_fault
@ 2015-11-20 0:18 Dan Williams
2015-11-20 3:03 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2015-11-20 0:18 UTC (permalink / raw)
To: linux-ext4
Cc: linux-fsdevel, Ted Ts'o, linux-nvdimm, Andreas Dilger,
Jan Kara
Neither the filemap_fault() nor the xfs dax fault path is updating time.
This call leads to the following WARN() when the block device has been
torn down:
WARNING: CPU: 0 PID: 2133 at fs/fs-writeback.c:2065 __mark_inode_dirty+0x261/0x350()
bdi-block not registered
[..]
Call Trace:
[<ffffffff81459f62>] dump_stack+0x44/0x62
[<ffffffff810a2052>] warn_slowpath_common+0x82/0xc0
[<ffffffff810a20ec>] warn_slowpath_fmt+0x5c/0x80
[<ffffffff812831a1>] __mark_inode_dirty+0x261/0x350
[<ffffffff8126d109>] generic_update_time+0x79/0xd0
[<ffffffff8126d28d>] file_update_time+0xbd/0x110
[<ffffffff812e4bc8>] ext4_dax_fault+0x68/0x110
[<ffffffff811f816e>] __do_fault+0x4e/0xf0
[<ffffffff811fc2a7>] handle_mm_fault+0x5e7/0x1b50
[<ffffffff811fbd11>] ? handle_mm_fault+0x51/0x1b50
[<ffffffff810689c1>] __do_page_fault+0x191/0x3f0
[<ffffffff81068cef>] trace_do_page_fault+0x4f/0x120
[<ffffffff8106314a>] do_async_page_fault+0x1a/0xa0
[<ffffffff81902678>] async_page_fault+0x28/0x30
Cc: Jan Kara <jack@suse.com>
Cc: Ted Ts'o <tytso@mit.edu>
Cc: Andreas Dilger <adilger.kernel@dilger.ca>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
fs/ext4/file.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 113837e7ba98..76854dd6dd7d 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -214,7 +214,6 @@ static int ext4_dax_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
if (write) {
sb_start_pagefault(sb);
- file_update_time(vma->vm_file);
handle = ext4_journal_start_sb(sb, EXT4_HT_WRITE_PAGE,
EXT4_DATA_TRANS_BLOCKS(sb));
}
@@ -245,7 +244,6 @@ static int ext4_dax_pmd_fault(struct vm_area_struct *vma, unsigned long addr,
if (write) {
sb_start_pagefault(sb);
- file_update_time(vma->vm_file);
handle = ext4_journal_start_sb(sb, EXT4_HT_WRITE_PAGE,
ext4_chunk_trans_blocks(inode,
PMD_SIZE / PAGE_SIZE));
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ext4: drop file_update_time from ext4_dax_fault
2015-11-20 0:18 [PATCH] ext4: drop file_update_time from ext4_dax_fault Dan Williams
@ 2015-11-20 3:03 ` Dave Chinner
2015-11-20 4:39 ` Dan Williams
2015-11-23 14:15 ` Jan Kara
0 siblings, 2 replies; 6+ messages in thread
From: Dave Chinner @ 2015-11-20 3:03 UTC (permalink / raw)
To: Dan Williams
Cc: linux-ext4, linux-fsdevel, Ted Ts'o, linux-nvdimm,
Andreas Dilger, Jan Kara
On Thu, Nov 19, 2015 at 04:18:13PM -0800, Dan Williams wrote:
> Neither the filemap_fault() nor the xfs dax fault path is updating time.
> This call leads to the following WARN() when the block device has been
> torn down:
I don't think that is right. In xfs_filemap_fault():
....
/* DAX can shortcut the normal fault path on write faults! */
if ((vmf->flags & FAULT_FLAG_WRITE) && IS_DAX(inode))
return xfs_filemap_page_mkwrite(vma, vmf);
....
And xfs_filemap_page_mkwrite() most definitely calls file_update_time():
....
trace_xfs_filemap_page_mkwrite(XFS_I(inode));
sb_start_pagefault(inode->i_sb);
file_update_time(vma->vm_file);
xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
....
And, finally, in xfs_filemap_pmd_fault():
....
if (flags & FAULT_FLAG_WRITE) {
sb_start_pagefault(inode->i_sb);
file_update_time(vma->vm_file);
}
....
So we are clearly updating timestamps in XFS on every write fault
that occurs, whether it be through ->page_mkwrite, ->fault or
->pmd_fault. Hence removing those from ext4 can't be the righ tthing
to do.
>
> WARNING: CPU: 0 PID: 2133 at fs/fs-writeback.c:2065 __mark_inode_dirty+0x261/0x350()
> bdi-block not registered
^^^^^^^^^^^^^^^^^^^^^^^^
> [..]
> Call Trace:
> [<ffffffff81459f62>] dump_stack+0x44/0x62
> [<ffffffff810a2052>] warn_slowpath_common+0x82/0xc0
> [<ffffffff810a20ec>] warn_slowpath_fmt+0x5c/0x80
> [<ffffffff812831a1>] __mark_inode_dirty+0x261/0x350
> [<ffffffff8126d109>] generic_update_time+0x79/0xd0
> [<ffffffff8126d28d>] file_update_time+0xbd/0x110
> [<ffffffff812e4bc8>] ext4_dax_fault+0x68/0x110
> [<ffffffff811f816e>] __do_fault+0x4e/0xf0
> [<ffffffff811fc2a7>] handle_mm_fault+0x5e7/0x1b50
> [<ffffffff811fbd11>] ? handle_mm_fault+0x51/0x1b50
> [<ffffffff810689c1>] __do_page_fault+0x191/0x3f0
> [<ffffffff81068cef>] trace_do_page_fault+0x4f/0x120
> [<ffffffff8106314a>] do_async_page_fault+0x1a/0xa0
> [<ffffffff81902678>] async_page_fault+0x28/0x30
Doesn't this indicate some problem at the block/bdi level?
__mark_inode_dirty() should not throw warnings like this regardless
of where it is called from...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ext4: drop file_update_time from ext4_dax_fault
2015-11-20 3:03 ` Dave Chinner
@ 2015-11-20 4:39 ` Dan Williams
2015-11-21 21:32 ` Dave Chinner
2015-11-23 14:15 ` Jan Kara
1 sibling, 1 reply; 6+ messages in thread
From: Dan Williams @ 2015-11-20 4:39 UTC (permalink / raw)
To: Dave Chinner
Cc: Ted Ts'o, linux-nvdimm@lists.01.org, Andreas Dilger, Jan Kara,
linux-fsdevel, ext4 hackers
On Thu, Nov 19, 2015 at 7:03 PM, Dave Chinner <david@fromorbit.com> wrote:
> On Thu, Nov 19, 2015 at 04:18:13PM -0800, Dan Williams wrote:
>> Neither the filemap_fault() nor the xfs dax fault path is updating time.
>> This call leads to the following WARN() when the block device has been
>> torn down:
>
> I don't think that is right. In xfs_filemap_fault():
>
>
> ....
> /* DAX can shortcut the normal fault path on write faults! */
> if ((vmf->flags & FAULT_FLAG_WRITE) && IS_DAX(inode))
> return xfs_filemap_page_mkwrite(vma, vmf);
> ....
>
> And xfs_filemap_page_mkwrite() most definitely calls file_update_time():
>
> ....
> trace_xfs_filemap_page_mkwrite(XFS_I(inode));
>
> sb_start_pagefault(inode->i_sb);
> file_update_time(vma->vm_file);
> xfs_ilock(XFS_I(inode), XFS_MMAPLOCK_SHARED);
> ....
>
> And, finally, in xfs_filemap_pmd_fault():
>
> ....
> if (flags & FAULT_FLAG_WRITE) {
> sb_start_pagefault(inode->i_sb);
> file_update_time(vma->vm_file);
> }
> ....
>
> So we are clearly updating timestamps in XFS on every write fault
> that occurs, whether it be through ->page_mkwrite, ->fault or
> ->pmd_fault. Hence removing those from ext4 can't be the righ tthing
> to do.
>
Ah sorry I missed that. When I saw that xfs did not trigger the same
warning as ext4 I just assumed it wasn't doing the time update.
>>
>> WARNING: CPU: 0 PID: 2133 at fs/fs-writeback.c:2065 __mark_inode_dirty+0x261/0x350()
>> bdi-block not registered
> ^^^^^^^^^^^^^^^^^^^^^^^^
>> [..]
>> Call Trace:
>> [<ffffffff81459f62>] dump_stack+0x44/0x62
>> [<ffffffff810a2052>] warn_slowpath_common+0x82/0xc0
>> [<ffffffff810a20ec>] warn_slowpath_fmt+0x5c/0x80
>> [<ffffffff812831a1>] __mark_inode_dirty+0x261/0x350
>> [<ffffffff8126d109>] generic_update_time+0x79/0xd0
>> [<ffffffff8126d28d>] file_update_time+0xbd/0x110
>> [<ffffffff812e4bc8>] ext4_dax_fault+0x68/0x110
>> [<ffffffff811f816e>] __do_fault+0x4e/0xf0
>> [<ffffffff811fc2a7>] handle_mm_fault+0x5e7/0x1b50
>> [<ffffffff811fbd11>] ? handle_mm_fault+0x51/0x1b50
>> [<ffffffff810689c1>] __do_page_fault+0x191/0x3f0
>> [<ffffffff81068cef>] trace_do_page_fault+0x4f/0x120
>> [<ffffffff8106314a>] do_async_page_fault+0x1a/0xa0
>> [<ffffffff81902678>] async_page_fault+0x28/0x30
>
> Doesn't this indicate some problem at the block/bdi level?
> __mark_inode_dirty() should not throw warnings like this regardless
> of where it is called from...
>
I'll look closer at how the xfs path avoids triggering this...
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ext4: drop file_update_time from ext4_dax_fault
2015-11-20 4:39 ` Dan Williams
@ 2015-11-21 21:32 ` Dave Chinner
0 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2015-11-21 21:32 UTC (permalink / raw)
To: Dan Williams
Cc: Ted Ts'o, linux-nvdimm@lists.01.org, Andreas Dilger, Jan Kara,
linux-fsdevel, ext4 hackers
On Thu, Nov 19, 2015 at 08:39:06PM -0800, Dan Williams wrote:
> On Thu, Nov 19, 2015 at 7:03 PM, Dave Chinner <david@fromorbit.com> wrote:
> > On Thu, Nov 19, 2015 at 04:18:13PM -0800, Dan Williams wrote:
> >> WARNING: CPU: 0 PID: 2133 at fs/fs-writeback.c:2065 __mark_inode_dirty+0x261/0x350()
> >> bdi-block not registered
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> >> [..]
> >> Call Trace:
> >> [<ffffffff81459f62>] dump_stack+0x44/0x62
> >> [<ffffffff810a2052>] warn_slowpath_common+0x82/0xc0
> >> [<ffffffff810a20ec>] warn_slowpath_fmt+0x5c/0x80
> >> [<ffffffff812831a1>] __mark_inode_dirty+0x261/0x350
> >> [<ffffffff8126d109>] generic_update_time+0x79/0xd0
> >> [<ffffffff8126d28d>] file_update_time+0xbd/0x110
> >> [<ffffffff812e4bc8>] ext4_dax_fault+0x68/0x110
> >> [<ffffffff811f816e>] __do_fault+0x4e/0xf0
> >> [<ffffffff811fc2a7>] handle_mm_fault+0x5e7/0x1b50
> >> [<ffffffff811fbd11>] ? handle_mm_fault+0x51/0x1b50
> >> [<ffffffff810689c1>] __do_page_fault+0x191/0x3f0
> >> [<ffffffff81068cef>] trace_do_page_fault+0x4f/0x120
> >> [<ffffffff8106314a>] do_async_page_fault+0x1a/0xa0
> >> [<ffffffff81902678>] async_page_fault+0x28/0x30
> >
> > Doesn't this indicate some problem at the block/bdi level?
> > __mark_inode_dirty() should not throw warnings like this regardless
> > of where it is called from...
> >
>
> I'll look closer at how the xfs path avoids triggering this...
XFS doesn't mark inodes dirty at the VFS level for inode metadata
changes - dirty metadata is tracked by the journalling subsystem,
not the VFS.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: drop file_update_time from ext4_dax_fault
2015-11-20 3:03 ` Dave Chinner
2015-11-20 4:39 ` Dan Williams
@ 2015-11-23 14:15 ` Jan Kara
2015-11-23 19:55 ` Dave Chinner
1 sibling, 1 reply; 6+ messages in thread
From: Jan Kara @ 2015-11-23 14:15 UTC (permalink / raw)
To: Dave Chinner
Cc: Dan Williams, linux-ext4, linux-fsdevel, Ted Ts'o,
linux-nvdimm, Andreas Dilger, Jan Kara
On Fri 20-11-15 14:03:44, Dave Chinner wrote:
> On Thu, Nov 19, 2015 at 04:18:13PM -0800, Dan Williams wrote:
> > WARNING: CPU: 0 PID: 2133 at fs/fs-writeback.c:2065 __mark_inode_dirty+0x261/0x350()
> > bdi-block not registered
> ^^^^^^^^^^^^^^^^^^^^^^^^
> > [..]
> > Call Trace:
> > [<ffffffff81459f62>] dump_stack+0x44/0x62
> > [<ffffffff810a2052>] warn_slowpath_common+0x82/0xc0
> > [<ffffffff810a20ec>] warn_slowpath_fmt+0x5c/0x80
> > [<ffffffff812831a1>] __mark_inode_dirty+0x261/0x350
> > [<ffffffff8126d109>] generic_update_time+0x79/0xd0
> > [<ffffffff8126d28d>] file_update_time+0xbd/0x110
> > [<ffffffff812e4bc8>] ext4_dax_fault+0x68/0x110
> > [<ffffffff811f816e>] __do_fault+0x4e/0xf0
> > [<ffffffff811fc2a7>] handle_mm_fault+0x5e7/0x1b50
> > [<ffffffff811fbd11>] ? handle_mm_fault+0x51/0x1b50
> > [<ffffffff810689c1>] __do_page_fault+0x191/0x3f0
> > [<ffffffff81068cef>] trace_do_page_fault+0x4f/0x120
> > [<ffffffff8106314a>] do_async_page_fault+0x1a/0xa0
> > [<ffffffff81902678>] async_page_fault+0x28/0x30
>
> Doesn't this indicate some problem at the block/bdi level?
> __mark_inode_dirty() should not throw warnings like this regardless
> of where it is called from...
The warning warns that we are dirtying inodes against a backing device that
advertises it does writeback but is not registered with writeback code (so
flusher does not run for it). That makes sense but people seem to be
experimenting a lot with removing devices under filesystems lately and so
this warning triggers. Probably we have to refine it to not trigger for
such cases...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: drop file_update_time from ext4_dax_fault
2015-11-23 14:15 ` Jan Kara
@ 2015-11-23 19:55 ` Dave Chinner
0 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2015-11-23 19:55 UTC (permalink / raw)
To: Jan Kara
Cc: Dan Williams, linux-ext4, linux-fsdevel, Ted Ts'o,
linux-nvdimm, Andreas Dilger, Jan Kara
On Mon, Nov 23, 2015 at 03:15:56PM +0100, Jan Kara wrote:
> On Fri 20-11-15 14:03:44, Dave Chinner wrote:
> > On Thu, Nov 19, 2015 at 04:18:13PM -0800, Dan Williams wrote:
> > > WARNING: CPU: 0 PID: 2133 at fs/fs-writeback.c:2065 __mark_inode_dirty+0x261/0x350()
> > > bdi-block not registered
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> > > [..]
> > > Call Trace:
> > > [<ffffffff81459f62>] dump_stack+0x44/0x62
> > > [<ffffffff810a2052>] warn_slowpath_common+0x82/0xc0
> > > [<ffffffff810a20ec>] warn_slowpath_fmt+0x5c/0x80
> > > [<ffffffff812831a1>] __mark_inode_dirty+0x261/0x350
> > > [<ffffffff8126d109>] generic_update_time+0x79/0xd0
> > > [<ffffffff8126d28d>] file_update_time+0xbd/0x110
> > > [<ffffffff812e4bc8>] ext4_dax_fault+0x68/0x110
> > > [<ffffffff811f816e>] __do_fault+0x4e/0xf0
> > > [<ffffffff811fc2a7>] handle_mm_fault+0x5e7/0x1b50
> > > [<ffffffff811fbd11>] ? handle_mm_fault+0x51/0x1b50
> > > [<ffffffff810689c1>] __do_page_fault+0x191/0x3f0
> > > [<ffffffff81068cef>] trace_do_page_fault+0x4f/0x120
> > > [<ffffffff8106314a>] do_async_page_fault+0x1a/0xa0
> > > [<ffffffff81902678>] async_page_fault+0x28/0x30
> >
> > Doesn't this indicate some problem at the block/bdi level?
> > __mark_inode_dirty() should not throw warnings like this regardless
> > of where it is called from...
>
> The warning warns that we are dirtying inodes against a backing device that
> advertises it does writeback but is not registered with writeback code (so
> flusher does not run for it). That makes sense but people seem to be
> experimenting a lot with removing devices under filesystems lately and so
> this warning triggers. Probably we have to refine it to not trigger for
> such cases...
Or, alternatively, have the block devices inform the filesystem that
it no longer has a valid bdi backing it. That way the filesystem can
error out at a higher layer and we never get to the point of trying
to mark an inode dirty when the backing device has been unplugged...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-23 19:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-20 0:18 [PATCH] ext4: drop file_update_time from ext4_dax_fault Dan Williams
2015-11-20 3:03 ` Dave Chinner
2015-11-20 4:39 ` Dan Williams
2015-11-21 21:32 ` Dave Chinner
2015-11-23 14:15 ` Jan Kara
2015-11-23 19:55 ` Dave Chinner
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).