Re: [PATCH] ext4: introduce per-inode DAX flag

Re: [PATCH] ext4: introduce per-inode DAX flag

[Christoph Hellwig]

On Thu, Aug 24, 2017 at 02:20:57PM -0400, Theodore Ts'o wrote:
> The counter-argument is that system administrators do need to have a
> way to signal that they would like the file system to "do something
> different" on a per-file basis, and no one else has come up with
> another way of doing things.  Furthermore, it would be highly
> desirable if the system adminisator can provide this per-file system
> hint with requiring changes to the application.  (For example, by
> adding madvise/fadvise hints.)

We can always add some sort of inode or subtree advice.  It's just
the binary flag that encodes a specific implementation that is
very bad in the long run.

> Is that a fair summary of the argument?

Otherwise yet.

> I have two additional questions I'd like to ask at this point.
> 
> 1)  Has there been any other difficulty that XFS has had due to the
> fact that they have this DAX flag added?  e.g., are there any
> operational, or practical code maintainability issues at stake here?
> Or is this mostly an design philosophy debate?

It hasn't yet.  It will create really annoying problems once we
use raw DAX access for metadata, which I had prototype a while ago
and plan to finnally get in in the next months.

> 2) Are there any users using the DAX flag with XFS such that, if XFS
> were to remove the DAX flag support, those users would complain
> bitterly?

I don't know of anyone that actually uses the flag.  If someone did
that would probably run into problems like changing that changing it
on a file that's currently mmaped would crash an burn badly.

Re: [PATCH] ext4: introduce per-inode DAX flag

[Theodore Ts'o]

On Fri, Aug 25, 2017 at 12:54:15AM -0700, Christoph Hellwig wrote:
> > 1)  Has there been any other difficulty that XFS has had due to the
> > fact that they have this DAX flag added?  e.g., are there any
> > operational, or practical code maintainability issues at stake here?
> > Or is this mostly an design philosophy debate?
> 
> It hasn't yet.  It will create really annoying problems once we
> use raw DAX access for metadata, which I had prototype a while ago
> and plan to finnally get in in the next months.
> 
> > 2) Are there any users using the DAX flag with XFS such that, if XFS
> > were to remove the DAX flag support, those users would complain
> > bitterly?
> 
> I don't know of anyone that actually uses the flag.  If someone did
> that would probably run into problems like changing that changing it
> on a file that's currently mmaped would crash an burn badly.

Crash and burn meaning the *kernel* will crash and burn?  Or will data
be damaged?

Given that, maybe XFS should withdraw support for the DAX and hope no
one is actually using it?

					- Ted

Re: [PATCH] ext4: introduce per-inode DAX flag

[Christoph Hellwig]

On Fri, Aug 25, 2017 at 11:14:45AM -0400, Theodore Ts'o wrote:
> > I don't know of anyone that actually uses the flag.  If someone did
> > that would probably run into problems like changing that changing it
> > on a file that's currently mmaped would crash an burn badly.
> 
> Crash and burn meaning the *kernel* will crash and burn?  Or will data
> be damaged?
> 
> Given that, maybe XFS should withdraw support for the DAX and hope no
> one is actually using it?

Nah, -o dax works very well.  It's just the flag instead of the -o dax
option or rather switching it on a mapped file will probably be very dangerous.

> 
> 					- Ted
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

Re: [PATCH] ext4: introduce per-inode DAX flag

[Theodore Ts'o]

On Fri, Aug 25, 2017 at 08:40:32AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 25, 2017 at 11:14:45AM -0400, Theodore Ts'o wrote:
> > > I don't know of anyone that actually uses the flag.  If someone did
> > > that would probably run into problems like changing that changing it
> > > on a file that's currently mmaped would crash an burn badly.
> > 
> > Crash and burn meaning the *kernel* will crash and burn?  Or will data
> > be damaged?
> > 
> > Given that, maybe XFS should withdraw support for the DAX and hope no
> > one is actually using it?
> 
> Nah, -o dax works very well.  It's just the flag instead of the -o dax
> option or rather switching it on a mapped file will probably be very dangerous.

Sorry, I was suggested XFS withdrawing support for the pre-file dax
flag.  I thought part of the argument for why ext4 should accept this
patch is that XFS already has the per-file DAX flag?

      	      	  	      	  	   - Ted

Re: [PATCH] ext4: introduce per-inode DAX flag

[Dave Chinner]

On Fri, Aug 25, 2017 at 08:40:32AM -0700, Christoph Hellwig wrote:
> On Fri, Aug 25, 2017 at 11:14:45AM -0400, Theodore Ts'o wrote:
> > > I don't know of anyone that actually uses the flag.  If someone did
> > > that would probably run into problems like changing that changing it
> > > on a file that's currently mmaped would crash an burn badly.
> > 
> > Crash and burn meaning the *kernel* will crash and burn?  Or will data
> > be damaged?
> > 
> > Given that, maybe XFS should withdraw support for the DAX and hope no
> > one is actually using it?
> 
> Nah, -o dax works very well.  It's just the flag instead of the -o dax
> option or rather switching it on a mapped file will probably be very dangerous.

In what way is it dangerous, Christoph?

Changing to/form DAX is simply a case of syncing the data and then
invalidating the existing mappings and letting the new faults go
direct or through the page cache. The mapping invalidation
requirements are no different to what we need to do to punch a hole
in a mapped file...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: introduce per-inode DAX flag

[Christoph Hellwig]

On Sat, Aug 26, 2017 at 09:33:58AM +1000, Dave Chinner wrote:
> > Nah, -o dax works very well.  It's just the flag instead of the -o dax
> > option or rather switching it on a mapped file will probably be very dangerous.
> 
> In what way is it dangerous, Christoph?

When I run the following script as a normal user:

FSXDIR=~/xfstests/ltp/
FILE=/mnt/foo

${FSXDIR}/fsx $FILE &

while true; do
    xfs_io -c 'chattr +x' $FILE
    xfs_io -c 'chattr -x' $FILE
done

I get this nice little crash:

root@testvm:~# sh test.sh
skipping zero size read
skipping insert range behind EOF
truncating to largest ever: 0x3a290
zero_range to largest ever: 0x3a8d1
zero_range to largest ever: 0x3fe3e
zero_range to largest ever: 0x40000
[  344.898390] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
[  344.899306] IP: iomap_page_mkwrite+0x17/0xf0
[  344.899795] PGD 7db37067
[  344.899796] P4D 7db37067
[  344.900099] PUD 78c61067
[  344.900389] PMD 0
[  344.900665]
[  344.901075] Oops: 0000 [#1] SMP
[  344.901536] Modules linked in:
[  344.901716] CPU: 3 PID: 6052 Comm: fsx Not tainted 4.12.0+ #2199
[  344.901716] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
[  344.901716] task: ffff880079a0da00 task.stack: ffffc900068a4000
[  344.901716] RIP: 0010:iomap_page_mkwrite+0x17/0xf0
[  344.901716] RSP: 0000:ffffc900068a7d38 EFLAGS: 00010246
[  344.901716] RAX: ffff8800798dd0d0 RBX: 0000000000000200 RCX: 0000000000000001
[  344.901716] RDX: 0000000070eb898e RSI: ffffffff82109010 RDI: ffffc900068a7df0
[  344.901716] RBP: ffffc900068a7d60 R08: ffffffff82ff9fa8 R09: 0000000000000000
[  344.901716] R10: ffffc900068a7cb0 R11: ffffffff8159b5cc R12: ffffffff82109010
[  344.901716] R13: 0000000000000000 R14: ffffc900068a7df0 R15: ffff88007da89580
[  344.901716] FS:  00007f76bc863700(0000) GS:ffff88007fd80000(0000) knlGS:0000000000000000
[  344.901716] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  344.901716] CR2: 0000000000000020 CR3: 000000007d90b000 CR4: 00000000000006e0
[  344.901716] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  344.901716] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  344.901716] Call Trace:
[  344.901716]  xfs_filemap_page_mkwrite+0x90/0x1d0
[  344.901716]  xfs_filemap_fault+0x114/0x180
[  344.901716]  __do_fault+0x19/0x60
[  344.901716]  __handle_mm_fault+0x68f/0xaf0
[  344.901716]  handle_mm_fault+0x82/0x130
[  344.901716]  __do_page_fault+0x222/0x4d0
[  344.901716]  trace_do_page_fault+0x85/0x210
[  344.901716]  do_async_page_fault+0x14/0x60
[  344.901716]  async_page_fault+0x28/0x30
[  344.901716] RIP: 0033:0x7f76bbd14049
[  344.901716] RSP: 002b:00007ffcdd6c7fe8 EFLAGS: 00010206
[  344.901716] RAX: 000000000002f144 RBX: 0000000000000fd9 RCX: 000000000000fd98
[  344.901716] RDX: 0000000000007ecc RSI: 00007f76bc80efe9 RDI: 00007f76bc7d7fd9
[  344.901716] RBP: 00007f76bc7d7000 R08: 0000000000000003 R09: 000000000002e000
[  344.901716] R10: 0000000000000001 R11: 0000000000000206 R12: 0000000000007ecc
[  344.901716] R13: 000000000002efd9 R14: 0000000000008ea5 R15: 0000000000000000
[  344.901716] Code: 50 ff ff ff 5d c3 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 55 48 89 e5 41 57 41 56 41 55 41 54 49 89 f4 53 4c 8b 6f 48 48 8b 07 <49> 8b 55 20 48 8b 80 a0 00 00 00 4c 8b 70 20 48 8d 42 ff 83 e2
[  344.901716] RIP: iomap_page_mkwrite+0x17/0xf0 RSP: ffffc900068a7d38
[  344.901716] CR2: 0000000000000020
[  344.924264] ---[ end trace c4e8d3bdccf6912b ]---
[  344.924834] Kernel panic - not syncing: Fatal exception
[  344.925504] Kernel Offset: disabled
[  344.925884] ---[ end Kernel panic - not syncing: Fatal exception

Re: [PATCH] ext4: introduce per-inode DAX flag

[Dave Chinner]

On Mon, Aug 28, 2017 at 12:38:54AM -0700, Christoph Hellwig wrote:
> On Sat, Aug 26, 2017 at 09:33:58AM +1000, Dave Chinner wrote:
> > > Nah, -o dax works very well.  It's just the flag instead of the -o dax
> > > option or rather switching it on a mapped file will probably be very dangerous.
> > 
> > In what way is it dangerous, Christoph?
> 
> When I run the following script as a normal user:
> 
> FSXDIR=~/xfstests/ltp/
> FILE=/mnt/foo
> 
> ${FSXDIR}/fsx $FILE &
> 
> while true; do
>     xfs_io -c 'chattr +x' $FILE
>     xfs_io -c 'chattr -x' $FILE
> done
>
> I get this nice little crash:

Can you please package that up into an xfstest?

> root@testvm:~# sh test.sh
> skipping zero size read
> skipping insert range behind EOF
> truncating to largest ever: 0x3a290
> zero_range to largest ever: 0x3a8d1
> zero_range to largest ever: 0x3fe3e
> zero_range to largest ever: 0x40000
> [  344.898390] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> [  344.899306] IP: iomap_page_mkwrite+0x17/0xf0
> [  344.899795] PGD 7db37067
> [  344.899796] P4D 7db37067
> [  344.900099] PUD 78c61067
> [  344.900389] PMD 0
> [  344.900665]
> [  344.901075] Oops: 0000 [#1] SMP
> [  344.901536] Modules linked in:
> [  344.901716] CPU: 3 PID: 6052 Comm: fsx Not tainted 4.12.0+ #2199
> [  344.901716] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> [  344.901716] task: ffff880079a0da00 task.stack: ffffc900068a4000
> [  344.901716] RIP: 0010:iomap_page_mkwrite+0x17/0xf0
> [  344.901716] RSP: 0000:ffffc900068a7d38 EFLAGS: 00010246
> [  344.901716] RAX: ffff8800798dd0d0 RBX: 0000000000000200 RCX: 0000000000000001
> [  344.901716] RDX: 0000000070eb898e RSI: ffffffff82109010 RDI: ffffc900068a7df0
> [  344.901716] RBP: ffffc900068a7d60 R08: ffffffff82ff9fa8 R09: 0000000000000000
> [  344.901716] R10: ffffc900068a7cb0 R11: ffffffff8159b5cc R12: ffffffff82109010
> [  344.901716] R13: 0000000000000000 R14: ffffc900068a7df0 R15: ffff88007da89580
                      ^^^^^^^^^^^^^^^^

vmf->page is null.

Which means IS_DAX changed half way through a fault, despite us
holding the MMAPLOCK and protecting all the filesystem side of the
fault code from races.

Seems to me that even allowing filesystems to switch between
different mapping tree behaviours based on an inode flag is a
fundamentally broken model. The fault action that needs to taken by
the filesystem has already been predetermined by the fault
processing that has already occurred and placed into the contents of
the vmf we've been passed.

Hence I think that if we need to process the fault as a DAX fault
then the vmf needs to tell us that, not require us to look up an
inode flag to determine what to do. ANd if the inode flag changes,
then that needs to be propagated through the mapping and VMAs in a
sane fashion, not just run an invalidation from the filesystem. I
don't know enough about the VM code to say anything useful about how
this needs to be set up, but it's clear that mapping invalidation
and behaviour swaps can't be completely serialised against page
faults from the filesystem side.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: introduce per-inode DAX flag

[Jan Kara]

On Mon 28-08-17 20:10:14, Dave Chinner wrote:
> On Mon, Aug 28, 2017 at 12:38:54AM -0700, Christoph Hellwig wrote:
> > On Sat, Aug 26, 2017 at 09:33:58AM +1000, Dave Chinner wrote:
> > > > Nah, -o dax works very well.  It's just the flag instead of the -o dax
> > > > option or rather switching it on a mapped file will probably be very dangerous.
> > > 
> > > In what way is it dangerous, Christoph?
> > 
> > When I run the following script as a normal user:
> > 
> > FSXDIR=~/xfstests/ltp/
> > FILE=/mnt/foo
> > 
> > ${FSXDIR}/fsx $FILE &
> > 
> > while true; do
> >     xfs_io -c 'chattr +x' $FILE
> >     xfs_io -c 'chattr -x' $FILE
> > done
> >
> > I get this nice little crash:
> 
> Can you please package that up into an xfstest?
> 
> > root@testvm:~# sh test.sh
> > skipping zero size read
> > skipping insert range behind EOF
> > truncating to largest ever: 0x3a290
> > zero_range to largest ever: 0x3a8d1
> > zero_range to largest ever: 0x3fe3e
> > zero_range to largest ever: 0x40000
> > [  344.898390] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> > [  344.899306] IP: iomap_page_mkwrite+0x17/0xf0
> > [  344.899795] PGD 7db37067
> > [  344.899796] P4D 7db37067
> > [  344.900099] PUD 78c61067
> > [  344.900389] PMD 0
> > [  344.900665]
> > [  344.901075] Oops: 0000 [#1] SMP
> > [  344.901536] Modules linked in:
> > [  344.901716] CPU: 3 PID: 6052 Comm: fsx Not tainted 4.12.0+ #2199
> > [  344.901716] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> > [  344.901716] task: ffff880079a0da00 task.stack: ffffc900068a4000
> > [  344.901716] RIP: 0010:iomap_page_mkwrite+0x17/0xf0
> > [  344.901716] RSP: 0000:ffffc900068a7d38 EFLAGS: 00010246
> > [  344.901716] RAX: ffff8800798dd0d0 RBX: 0000000000000200 RCX: 0000000000000001
> > [  344.901716] RDX: 0000000070eb898e RSI: ffffffff82109010 RDI: ffffc900068a7df0
> > [  344.901716] RBP: ffffc900068a7d60 R08: ffffffff82ff9fa8 R09: 0000000000000000
> > [  344.901716] R10: ffffc900068a7cb0 R11: ffffffff8159b5cc R12: ffffffff82109010
> > [  344.901716] R13: 0000000000000000 R14: ffffc900068a7df0 R15: ffff88007da89580
>                       ^^^^^^^^^^^^^^^^
> 
> vmf->page is null.
> 
> Which means IS_DAX changed half way through a fault, despite us
> holding the MMAPLOCK and protecting all the filesystem side of the
> fault code from races.
> 
> Seems to me that even allowing filesystems to switch between
> different mapping tree behaviours based on an inode flag is a
> fundamentally broken model. The fault action that needs to taken by
> the filesystem has already been predetermined by the fault
> processing that has already occurred and placed into the contents of
> the vmf we've been passed.

I don't think the problem is actually within MM in this particular case.
The problem seems to be that xfs_filemap_fault() checks IS_DAX without
holding MMAPLOCK and so it can change after that test and before the test
in xfs_filemap_page_mkwrite().

> Hence I think that if we need to process the fault as a DAX fault
> then the vmf needs to tell us that, not require us to look up an
> inode flag to determine what to do. ANd if the inode flag changes,
> then that needs to be propagated through the mapping and VMAs in a
> sane fashion, not just run an invalidation from the filesystem. I
> don't know enough about the VM code to say anything useful about how
> this needs to be set up, but it's clear that mapping invalidation
> and behaviour swaps can't be completely serialised against page
> faults from the filesystem side.

But there is no difference in vmf setup from generic MM side. In particular
vmf->page is set by the ->fault handler and then it is passed to
->page_mkwrite handler. And changes to mapping behavior between these two
callbacks should be prevented by the page lock / radix entry lock...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: introduce per-inode DAX flag

[Dave Chinner]

On Tue, Aug 29, 2017 at 05:49:22PM +0200, Jan Kara wrote:
> On Mon 28-08-17 20:10:14, Dave Chinner wrote:
> > On Mon, Aug 28, 2017 at 12:38:54AM -0700, Christoph Hellwig wrote:
> > > On Sat, Aug 26, 2017 at 09:33:58AM +1000, Dave Chinner wrote:
> > > > > Nah, -o dax works very well.  It's just the flag instead of the -o dax
> > > > > option or rather switching it on a mapped file will probably be very dangerous.
> > > > 
> > > > In what way is it dangerous, Christoph?
> > > 
> > > When I run the following script as a normal user:
> > > 
> > > FSXDIR=~/xfstests/ltp/
> > > FILE=/mnt/foo
> > > 
> > > ${FSXDIR}/fsx $FILE &
> > > 
> > > while true; do
> > >     xfs_io -c 'chattr +x' $FILE
> > >     xfs_io -c 'chattr -x' $FILE
> > > done
> > >
> > > I get this nice little crash:
> > 
> > Can you please package that up into an xfstest?
> > 
> > > root@testvm:~# sh test.sh
> > > skipping zero size read
> > > skipping insert range behind EOF
> > > truncating to largest ever: 0x3a290
> > > zero_range to largest ever: 0x3a8d1
> > > zero_range to largest ever: 0x3fe3e
> > > zero_range to largest ever: 0x40000
> > > [  344.898390] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> > > [  344.899306] IP: iomap_page_mkwrite+0x17/0xf0
> > > [  344.899795] PGD 7db37067
> > > [  344.899796] P4D 7db37067
> > > [  344.900099] PUD 78c61067
> > > [  344.900389] PMD 0
> > > [  344.900665]
> > > [  344.901075] Oops: 0000 [#1] SMP
> > > [  344.901536] Modules linked in:
> > > [  344.901716] CPU: 3 PID: 6052 Comm: fsx Not tainted 4.12.0+ #2199
> > > [  344.901716] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> > > [  344.901716] task: ffff880079a0da00 task.stack: ffffc900068a4000
> > > [  344.901716] RIP: 0010:iomap_page_mkwrite+0x17/0xf0
> > > [  344.901716] RSP: 0000:ffffc900068a7d38 EFLAGS: 00010246
> > > [  344.901716] RAX: ffff8800798dd0d0 RBX: 0000000000000200 RCX: 0000000000000001
> > > [  344.901716] RDX: 0000000070eb898e RSI: ffffffff82109010 RDI: ffffc900068a7df0
> > > [  344.901716] RBP: ffffc900068a7d60 R08: ffffffff82ff9fa8 R09: 0000000000000000
> > > [  344.901716] R10: ffffc900068a7cb0 R11: ffffffff8159b5cc R12: ffffffff82109010
> > > [  344.901716] R13: 0000000000000000 R14: ffffc900068a7df0 R15: ffff88007da89580
> >                       ^^^^^^^^^^^^^^^^
> > 
> > vmf->page is null.
> > 
> > Which means IS_DAX changed half way through a fault, despite us
> > holding the MMAPLOCK and protecting all the filesystem side of the
> > fault code from races.
> > 
> > Seems to me that even allowing filesystems to switch between
> > different mapping tree behaviours based on an inode flag is a
> > fundamentally broken model. The fault action that needs to taken by
> > the filesystem has already been predetermined by the fault
> > processing that has already occurred and placed into the contents of
> > the vmf we've been passed.
> 
> I don't think the problem is actually within MM in this particular case.
> The problem seems to be that xfs_filemap_fault() checks IS_DAX without
> holding MMAPLOCK and so it can change after that test and before the test
> in xfs_filemap_page_mkwrite().

First thing I did was fix that, and it still paniced with vmf->page
= null in iomap_page_mkwrite, then I realised it was irrelevant
because if it can change between xfs_filemap_fault() and
xfs_filemap_page_mkwrite() it can change anytime in the fault path
we are not holding the MMAPLOCK....

> > Hence I think that if we need to process the fault as a DAX fault
> > then the vmf needs to tell us that, not require us to look up an
> > inode flag to determine what to do. ANd if the inode flag changes,
> > then that needs to be propagated through the mapping and VMAs in a
> > sane fashion, not just run an invalidation from the filesystem. I
> > don't know enough about the VM code to say anything useful about how
> > this needs to be set up, but it's clear that mapping invalidation
> > and behaviour swaps can't be completely serialised against page
> > faults from the filesystem side.
> 
> But there is no difference in vmf setup from generic MM side. In particular
> vmf->page is set by the ->fault handler and then it is passed to
> ->page_mkwrite handler.

Yes there is. DAX can call the .fault() handler directly from
handle_pte_fault() for write faults on DAX when there is no pte
installed. In which case vmf->page is null because we don't go
through do_wp_page() to install the page we faulted on in the vmf
before calling .page_mkwrite().

IOWs, if we fault between the invalidation in the IS_DAX changing
code and dropping the MMAPLOCK once the transaction inode change is
complete, then that fault will find an empty pte. It will then call
->filemap_fault with vmf->page = NULL, and block on the MMAPLOCK
until the filesystem transaction is complete. Then it checks
IS_DAX, finds it's not set, so assumes that we have a page cache
fault, not a DAX fault....

It appears to me that we can't prevent this invalidation vs fault
race condition with filesystem-only locking and that means we can't
use filesystem side configuration flags to determine what action to
take in the filesystem side of the fault path. I have no idea what
mm/ side lock is needed to prevent this invalidation vs fault race
condition from occurring, nor whether it is even possible to take
and hold that lock in the filesysetm code where we would need to.

> And changes to mapping behavior between these two
> callbacks should be prevented by the page lock / radix entry lock...

No page, no page lock. We're above the mapping code, too, so no
radix entry lock is held here, either...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: introduce per-inode DAX flag

[Jan Kara]

On Wed 30-08-17 08:57:17, Dave Chinner wrote:
> On Tue, Aug 29, 2017 at 05:49:22PM +0200, Jan Kara wrote:
> > On Mon 28-08-17 20:10:14, Dave Chinner wrote:
> > > On Mon, Aug 28, 2017 at 12:38:54AM -0700, Christoph Hellwig wrote:
> > > > On Sat, Aug 26, 2017 at 09:33:58AM +1000, Dave Chinner wrote:
> > > > > > Nah, -o dax works very well.  It's just the flag instead of the -o dax
> > > > > > option or rather switching it on a mapped file will probably be very dangerous.
> > > > > 
> > > > > In what way is it dangerous, Christoph?
> > > > 
> > > > When I run the following script as a normal user:
> > > > 
> > > > FSXDIR=~/xfstests/ltp/
> > > > FILE=/mnt/foo
> > > > 
> > > > ${FSXDIR}/fsx $FILE &
> > > > 
> > > > while true; do
> > > >     xfs_io -c 'chattr +x' $FILE
> > > >     xfs_io -c 'chattr -x' $FILE
> > > > done
> > > >
> > > > I get this nice little crash:
> > > 
> > > Can you please package that up into an xfstest?
> > > 
> > > > root@testvm:~# sh test.sh
> > > > skipping zero size read
> > > > skipping insert range behind EOF
> > > > truncating to largest ever: 0x3a290
> > > > zero_range to largest ever: 0x3a8d1
> > > > zero_range to largest ever: 0x3fe3e
> > > > zero_range to largest ever: 0x40000
> > > > [  344.898390] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> > > > [  344.899306] IP: iomap_page_mkwrite+0x17/0xf0
> > > > [  344.899795] PGD 7db37067
> > > > [  344.899796] P4D 7db37067
> > > > [  344.900099] PUD 78c61067
> > > > [  344.900389] PMD 0
> > > > [  344.900665]
> > > > [  344.901075] Oops: 0000 [#1] SMP
> > > > [  344.901536] Modules linked in:
> > > > [  344.901716] CPU: 3 PID: 6052 Comm: fsx Not tainted 4.12.0+ #2199
> > > > [  344.901716] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> > > > [  344.901716] task: ffff880079a0da00 task.stack: ffffc900068a4000
> > > > [  344.901716] RIP: 0010:iomap_page_mkwrite+0x17/0xf0
> > > > [  344.901716] RSP: 0000:ffffc900068a7d38 EFLAGS: 00010246
> > > > [  344.901716] RAX: ffff8800798dd0d0 RBX: 0000000000000200 RCX: 0000000000000001
> > > > [  344.901716] RDX: 0000000070eb898e RSI: ffffffff82109010 RDI: ffffc900068a7df0
> > > > [  344.901716] RBP: ffffc900068a7d60 R08: ffffffff82ff9fa8 R09: 0000000000000000
> > > > [  344.901716] R10: ffffc900068a7cb0 R11: ffffffff8159b5cc R12: ffffffff82109010
> > > > [  344.901716] R13: 0000000000000000 R14: ffffc900068a7df0 R15: ffff88007da89580
> > >                       ^^^^^^^^^^^^^^^^
> > > 
> > > vmf->page is null.
> > > 
> > > Which means IS_DAX changed half way through a fault, despite us
> > > holding the MMAPLOCK and protecting all the filesystem side of the
> > > fault code from races.
> > > 
> > > Seems to me that even allowing filesystems to switch between
> > > different mapping tree behaviours based on an inode flag is a
> > > fundamentally broken model. The fault action that needs to taken by
> > > the filesystem has already been predetermined by the fault
> > > processing that has already occurred and placed into the contents of
> > > the vmf we've been passed.
> > 
> > I don't think the problem is actually within MM in this particular case.
> > The problem seems to be that xfs_filemap_fault() checks IS_DAX without
> > holding MMAPLOCK and so it can change after that test and before the test
> > in xfs_filemap_page_mkwrite().
> 
> First thing I did was fix that, and it still paniced with vmf->page
> = null in iomap_page_mkwrite, then I realised it was irrelevant

OK, drat.

> because if it can change between xfs_filemap_fault() and
> xfs_filemap_page_mkwrite() it can change anytime in the fault path
> we are not holding the MMAPLOCK....

Agreed but...

> > > Hence I think that if we need to process the fault as a DAX fault
> > > then the vmf needs to tell us that, not require us to look up an
> > > inode flag to determine what to do. ANd if the inode flag changes,
> > > then that needs to be propagated through the mapping and VMAs in a
> > > sane fashion, not just run an invalidation from the filesystem. I
> > > don't know enough about the VM code to say anything useful about how
> > > this needs to be set up, but it's clear that mapping invalidation
> > > and behaviour swaps can't be completely serialised against page
> > > faults from the filesystem side.
> > 
> > But there is no difference in vmf setup from generic MM side. In particular
> > vmf->page is set by the ->fault handler and then it is passed to
> > ->page_mkwrite handler.
> 
> Yes there is. DAX can call the .fault() handler directly from
> handle_pte_fault() for write faults on DAX when there is no pte
> installed. In which case vmf->page is null because we don't go
> through do_wp_page() to install the page we faulted on in the vmf
> before calling .page_mkwrite().
> 
> IOWs, if we fault between the invalidation in the IS_DAX changing
> code and dropping the MMAPLOCK once the transaction inode change is
> complete, then that fault will find an empty pte. It will then call
> ->filemap_fault with vmf->page = NULL, and block on the MMAPLOCK
> until the filesystem transaction is complete. Then it checks
> IS_DAX, finds it's not set, so assumes that we have a page cache
> fault, not a DAX fault....

So if you get to xfs_filemap_fault(), lock MMAPLOCK, then check IS_DAX() -
not set - then you go to filemap_fault() which is perfectly fine with
vmf->page being NULL. What am I missing?

The oops can happen if you call iomap_page_mkwrite() with vmf->page set
to NULL. However I don't see how that is possible after all IS_DAX checks
are under MMAPLOCK - iomap_page_mkwrite() cannot be called through
xfs_filemap_fault() anymore once DAX checks are consistent. And if it gets
called through xfs_filemap_page_mkwrite(), it means that
xfs_filemap_fault() was called before and must have returned
VM_FAULT_LOCKED which means that it has set vmf->page to a correct page.
[sorry, the page lock and entry lock I was speaking about in my previous
email was indeed wrong]

I've tried to reproduce this on my machine but for some reason setting of
per-inode DAX flag does not work. I have (with today's checkout of
xfsprogs):

marvin5:~/:[0]# cat /proc/mounts
...
/dev/ram0 /mnt xfs rw,relatime,attr2,dax,inode64,noquota 0 0

marvin5:~/:[0]# ls /mnt/
fsxfile  fsxfile.fsxgood  fsxfile.fsxlog

marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'lsattr' /mnt/fsxfile
-p-------------- /mnt/fsxfile 

marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'chattr +x' /mnt/fsxfile

marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'lsattr' /mnt/fsxfile
-p-------------- /mnt/fsxfile 

No DAX flag set and no error... What am I doing wrong?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: introduce per-inode DAX flag

[Christoph Hellwig]

On Wed, Aug 30, 2017 at 12:00:59PM +0200, Jan Kara wrote:
> /dev/ram0 /mnt xfs rw,relatime,attr2,dax,inode64,noquota 0 0
> 
> marvin5:~/:[0]# ls /mnt/
> fsxfile  fsxfile.fsxgood  fsxfile.fsxlog
> 
> marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'lsattr' /mnt/fsxfile
> -p-------------- /mnt/fsxfile 
> 
> marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'chattr +x' /mnt/fsxfile
> 
> marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'lsattr' /mnt/fsxfile
> -p-------------- /mnt/fsxfile 
> 
> No DAX flag set and no error... What am I doing wrong?

Odd.  Even on a non-DAX device the flag sticks for me:

root@testvm:~# mount /dev/sda /mnt/
root@testvm:~# touch foo
root@testvm:~# cd /mnt/
root@testvm:/mnt# xfs_io -c 'chattr +x' foo
root@testvm:/mnt# xfs_io -c 'lsattr' foo
--------------x- foo

Re: [PATCH] ext4: introduce per-inode DAX flag

[Theodore Ts'o]

Bringing this discussion back to the "ext4: introduce per-inode DAX
flag" patch, it sounds to me that I should _not_ apply this patch
until we figure out to fix this for reals.  Does this sound fair?

If we think it's completely unfixable, then maybe XFS should consider
(temporarily) withdrawing the per-file DAX support before there is a
userspace and you have to support it forever, but that's between the
NVM/dax and XFS folks to figure out.

					- Ted

Re: [PATCH] ext4: introduce per-inode DAX flag

[Jan Kara]

On Wed 30-08-17 05:34:50, Christoph Hellwig wrote:
> On Wed, Aug 30, 2017 at 12:00:59PM +0200, Jan Kara wrote:
> > /dev/ram0 /mnt xfs rw,relatime,attr2,dax,inode64,noquota 0 0
> > 
> > marvin5:~/:[0]# ls /mnt/
> > fsxfile  fsxfile.fsxgood  fsxfile.fsxlog
> > 
> > marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'lsattr' /mnt/fsxfile
> > -p-------------- /mnt/fsxfile 
> > 
> > marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'chattr +x' /mnt/fsxfile
> > 
> > marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'lsattr' /mnt/fsxfile
> > -p-------------- /mnt/fsxfile 
> > 
> > No DAX flag set and no error... What am I doing wrong?
> 
> Odd.  Even on a non-DAX device the flag sticks for me:
> 
> root@testvm:~# mount /dev/sda /mnt/
> root@testvm:~# touch foo
> root@testvm:~# cd /mnt/
> root@testvm:/mnt# xfs_io -c 'chattr +x' foo
> root@testvm:/mnt# xfs_io -c 'lsattr' foo
> --------------x- foo

OK, tracked this down. I had the XFS filesystem created so that
ip->i_d.di_version < 3 check in xfs_set_diflags() was true and so the flag
got silently ignored. I'd hope to get error in such case but whatever...

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: introduce per-inode DAX flag

[Lukas Czerner]

On Wed, Aug 30, 2017 at 11:00:19AM -0400, Theodore Ts'o wrote:
> Bringing this discussion back to the "ext4: introduce per-inode DAX
> flag" patch, it sounds to me that I should _not_ apply this patch
> until we figure out to fix this for reals.  Does this sound fair?
> 
> If we think it's completely unfixable, then maybe XFS should consider
> (temporarily) withdrawing the per-file DAX support before there is a
> userspace and you have to support it forever, but that's between the
> NVM/dax and XFS folks to figure out.
> 
> 					- Ted

Hi Ted,

I agree, this still has some issues even for ext4. Let's see what we can
do about that and I'll revisit it later.

Thanks!
-Lukas

Re: [PATCH] ext4: introduce per-inode DAX flag

[Jan Kara]

On Wed 30-08-17 12:00:59, Jan Kara wrote:
> On Wed 30-08-17 08:57:17, Dave Chinner wrote:
> > On Tue, Aug 29, 2017 at 05:49:22PM +0200, Jan Kara wrote:
> > > On Mon 28-08-17 20:10:14, Dave Chinner wrote:
> > > > On Mon, Aug 28, 2017 at 12:38:54AM -0700, Christoph Hellwig wrote:
> > > > > On Sat, Aug 26, 2017 at 09:33:58AM +1000, Dave Chinner wrote:
> > > > > > > Nah, -o dax works very well.  It's just the flag instead of the -o dax
> > > > > > > option or rather switching it on a mapped file will probably be very dangerous.
> > > > > > 
> > > > > > In what way is it dangerous, Christoph?
> > > > > 
> > > > > When I run the following script as a normal user:
> > > > > 
> > > > > FSXDIR=~/xfstests/ltp/
> > > > > FILE=/mnt/foo
> > > > > 
> > > > > ${FSXDIR}/fsx $FILE &
> > > > > 
> > > > > while true; do
> > > > >     xfs_io -c 'chattr +x' $FILE
> > > > >     xfs_io -c 'chattr -x' $FILE
> > > > > done
> > > > >
> > > > > I get this nice little crash:
> > > > 
> > > > Can you please package that up into an xfstest?
> > > > 
> > > > > root@testvm:~# sh test.sh
> > > > > skipping zero size read
> > > > > skipping insert range behind EOF
> > > > > truncating to largest ever: 0x3a290
> > > > > zero_range to largest ever: 0x3a8d1
> > > > > zero_range to largest ever: 0x3fe3e
> > > > > zero_range to largest ever: 0x40000
> > > > > [  344.898390] BUG: unable to handle kernel NULL pointer dereference at 0000000000000020
> > > > > [  344.899306] IP: iomap_page_mkwrite+0x17/0xf0
> > > > > [  344.899795] PGD 7db37067
> > > > > [  344.899796] P4D 7db37067
> > > > > [  344.900099] PUD 78c61067
> > > > > [  344.900389] PMD 0
> > > > > [  344.900665]
> > > > > [  344.901075] Oops: 0000 [#1] SMP
> > > > > [  344.901536] Modules linked in:
> > > > > [  344.901716] CPU: 3 PID: 6052 Comm: fsx Not tainted 4.12.0+ #2199
> > > > > [  344.901716] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
> > > > > [  344.901716] task: ffff880079a0da00 task.stack: ffffc900068a4000
> > > > > [  344.901716] RIP: 0010:iomap_page_mkwrite+0x17/0xf0
> > > > > [  344.901716] RSP: 0000:ffffc900068a7d38 EFLAGS: 00010246
> > > > > [  344.901716] RAX: ffff8800798dd0d0 RBX: 0000000000000200 RCX: 0000000000000001
> > > > > [  344.901716] RDX: 0000000070eb898e RSI: ffffffff82109010 RDI: ffffc900068a7df0
> > > > > [  344.901716] RBP: ffffc900068a7d60 R08: ffffffff82ff9fa8 R09: 0000000000000000
> > > > > [  344.901716] R10: ffffc900068a7cb0 R11: ffffffff8159b5cc R12: ffffffff82109010
> > > > > [  344.901716] R13: 0000000000000000 R14: ffffc900068a7df0 R15: ffff88007da89580
> > > >                       ^^^^^^^^^^^^^^^^
> > > > 
> > > > vmf->page is null.
> > > > 
> > > > Which means IS_DAX changed half way through a fault, despite us
> > > > holding the MMAPLOCK and protecting all the filesystem side of the
> > > > fault code from races.
> > > > 
> > > > Seems to me that even allowing filesystems to switch between
> > > > different mapping tree behaviours based on an inode flag is a
> > > > fundamentally broken model. The fault action that needs to taken by
> > > > the filesystem has already been predetermined by the fault
> > > > processing that has already occurred and placed into the contents of
> > > > the vmf we've been passed.
> > > 
> > > I don't think the problem is actually within MM in this particular case.
> > > The problem seems to be that xfs_filemap_fault() checks IS_DAX without
> > > holding MMAPLOCK and so it can change after that test and before the test
> > > in xfs_filemap_page_mkwrite().
> > 
> > First thing I did was fix that, and it still paniced with vmf->page
> > = null in iomap_page_mkwrite, then I realised it was irrelevant
> 
> OK, drat.
> 
> > because if it can change between xfs_filemap_fault() and
> > xfs_filemap_page_mkwrite() it can change anytime in the fault path
> > we are not holding the MMAPLOCK....
> 
> Agreed but...
> 
> > > > Hence I think that if we need to process the fault as a DAX fault
> > > > then the vmf needs to tell us that, not require us to look up an
> > > > inode flag to determine what to do. ANd if the inode flag changes,
> > > > then that needs to be propagated through the mapping and VMAs in a
> > > > sane fashion, not just run an invalidation from the filesystem. I
> > > > don't know enough about the VM code to say anything useful about how
> > > > this needs to be set up, but it's clear that mapping invalidation
> > > > and behaviour swaps can't be completely serialised against page
> > > > faults from the filesystem side.
> > > 
> > > But there is no difference in vmf setup from generic MM side. In particular
> > > vmf->page is set by the ->fault handler and then it is passed to
> > > ->page_mkwrite handler.
> > 
> > Yes there is. DAX can call the .fault() handler directly from
> > handle_pte_fault() for write faults on DAX when there is no pte
> > installed. In which case vmf->page is null because we don't go
> > through do_wp_page() to install the page we faulted on in the vmf
> > before calling .page_mkwrite().
> > 
> > IOWs, if we fault between the invalidation in the IS_DAX changing
> > code and dropping the MMAPLOCK once the transaction inode change is
> > complete, then that fault will find an empty pte. It will then call
> > ->filemap_fault with vmf->page = NULL, and block on the MMAPLOCK
> > until the filesystem transaction is complete. Then it checks
> > IS_DAX, finds it's not set, so assumes that we have a page cache
> > fault, not a DAX fault....
> 
> So if you get to xfs_filemap_fault(), lock MMAPLOCK, then check IS_DAX() -
> not set - then you go to filemap_fault() which is perfectly fine with
> vmf->page being NULL. What am I missing?
> 
> The oops can happen if you call iomap_page_mkwrite() with vmf->page set
> to NULL. However I don't see how that is possible after all IS_DAX checks
> are under MMAPLOCK - iomap_page_mkwrite() cannot be called through
> xfs_filemap_fault() anymore once DAX checks are consistent. And if it gets
> called through xfs_filemap_page_mkwrite(), it means that
> xfs_filemap_fault() was called before and must have returned
> VM_FAULT_LOCKED which means that it has set vmf->page to a correct page.
> [sorry, the page lock and entry lock I was speaking about in my previous
> email was indeed wrong]
> 
> I've tried to reproduce this on my machine but for some reason setting of
> per-inode DAX flag does not work. I have (with today's checkout of
> xfsprogs):
> 
> marvin5:~/:[0]# cat /proc/mounts
> ...
> /dev/ram0 /mnt xfs rw,relatime,attr2,dax,inode64,noquota 0 0
> 
> marvin5:~/:[0]# ls /mnt/
> fsxfile  fsxfile.fsxgood  fsxfile.fsxlog
> 
> marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'lsattr' /mnt/fsxfile
> -p-------------- /mnt/fsxfile 
> 
> marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'chattr +x' /mnt/fsxfile
> 
> marvin5:~/:[0]# ./xfsprogs-dev/io/xfs_io -c 'lsattr' /mnt/fsxfile
> -p-------------- /mnt/fsxfile 
> 
> No DAX flag set and no error... What am I doing wrong?

OK, after fixing this problem I could easily reproduce. And after some
investigation I have to agree it is non-trivial to fix this. I have fixed
races between .fault, .page_mkwrite, and flag change relatively easily.
However also DAX VMAs need to be setup in a special way (VM_MIXEDMAP set)
which obviously does not happen when mmap(2) is called on inode without
S_DAX set and just invalidating the mapping does not change the VMAs. We
could fix this by making sure file is not mapped when switching the flag
but it gets a bit awkward...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR