DAX data corruption for mmaped and written files

DAX data corruption for mmaped and written files

[Jan Kara]

Hello,

yesterday I have been stress-testing mmap code with my new fault locking
patches and I have found a data corruption issue when file is written both
via mmap and standard write(2). The problem is following:

CPU1					CPU2
dax_io()				dax_fault()
  get_block() - allocates block
  ...					  get_block() - finds allocated block
					    - zeroes it inside fs
					  fault completese

  if (buffer_unwritten(bh) || buffer_new(bh)) -> new buffer
    dax_new_buf() -> zeroes buffer which may
		     overwrite user data

In some cases the race can also go the other way around and we lose data
written by write.

So either we need to do the zeroing inside fs also for write(2) path (but
that would essentially mean we would write the block twice for each
allocating write) or we would need dax_io() to also use radix tree locking
to serialize against page faults (in the same way page cache does this with
page lock). Any opinion on what would be better?

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

Re: DAX data corruption for mmaped and written files

[Ross Zwisler]

On Thu, Mar 24, 2016 at 02:12:23PM +0100, Jan Kara wrote:
> Hello,
> 
> yesterday I have been stress-testing mmap code with my new fault locking
> patches and I have found a data corruption issue when file is written both
> via mmap and standard write(2). The problem is following:
> 
> CPU1					CPU2
> dax_io()				dax_fault()
>   get_block() - allocates block
>   ...					  get_block() - finds allocated block
> 					    - zeroes it inside fs
> 					  fault completese
> 
>   if (buffer_unwritten(bh) || buffer_new(bh)) -> new buffer
>     dax_new_buf() -> zeroes buffer which may
> 		     overwrite user data
> 
> In some cases the race can also go the other way around and we lose data
> written by write.
> 
> So either we need to do the zeroing inside fs also for write(2) path (but
> that would essentially mean we would write the block twice for each
> allocating write) or we would need dax_io() to also use radix tree locking
> to serialize against page faults (in the same way page cache does this with
> page lock). Any opinion on what would be better?

If the radix tree locking is essentially trying to be analogous to the page
lock, and the page lock is already used to serialize I/O vs page faults for
the page cache case, I guess doing that same serialization using our new DAX
locking seems like a natural fit.

Re: DAX data corruption for mmaped and written files

[Dave Chinner]

On Thu, Mar 24, 2016 at 02:12:23PM +0100, Jan Kara wrote:
> Hello,
> 
> yesterday I have been stress-testing mmap code with my new fault locking
> patches and I have found a data corruption issue when file is written both
> via mmap and standard write(2). The problem is following:
> 
> CPU1					CPU2
> dax_io()				dax_fault()
>   get_block() - allocates block
>   ...					  get_block() - finds allocated block
> 					    - zeroes it inside fs
> 					  fault completese
> 
>   if (buffer_unwritten(bh) || buffer_new(bh)) -> new buffer
>     dax_new_buf() -> zeroes buffer which may
> 		     overwrite user data

Which filesystem? XFS should be, in both cases, zeroing the
block entirely inside get_block() when it is first allocated. i.e we
should see:

CPU1					CPU2
dax_io()				dax_fault()
  get_block() - allocates block
              - zeroes it inside fs
  ...					  get_block() - finds allocated block
					  fault completes

  buffer returned is not new or unwritten.


> In some cases the race can also go the other way around and we lose data
> written by write.

It shouldn't:

CPU1					CPU2
dax_io()				dax_fault()
					  get_block() - allocates block
						      - zeroes it inside fs
					  ....
  get_block() - finds allocated block
  buffer returned is not new or unwritten.
					  fault completes

> So either we need to do the zeroing inside fs also for write(2) path (but
> that would essentially mean we would write the block twice for each
> allocating write)

Yup, that's what XFS is supposed to be doing right now...

> or we would need dax_io() to also use radix tree locking
> to serialize against page faults (in the same way page cache does this with
> page lock). Any opinion on what would be better?

Don't care, as long as data is not corrupted ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: DAX data corruption for mmaped and written files

[Jan Kara]

On Tue 29-03-16 09:39:16, Dave Chinner wrote:
> On Thu, Mar 24, 2016 at 02:12:23PM +0100, Jan Kara wrote:
> > Hello,
> > 
> > yesterday I have been stress-testing mmap code with my new fault locking
> > patches and I have found a data corruption issue when file is written both
> > via mmap and standard write(2). The problem is following:
> > 
> > CPU1					CPU2
> > dax_io()				dax_fault()
> >   get_block() - allocates block
> >   ...					  get_block() - finds allocated block
> > 					    - zeroes it inside fs
> > 					  fault completese
> > 
> >   if (buffer_unwritten(bh) || buffer_new(bh)) -> new buffer
> >     dax_new_buf() -> zeroes buffer which may
> > 		     overwrite user data
> 
> Which filesystem? XFS should be, in both cases, zeroing the
> block entirely inside get_block() when it is first allocated. i.e we
> should see:
> 
> CPU1					CPU2
> dax_io()				dax_fault()
>   get_block() - allocates block
>               - zeroes it inside fs
>   ...					  get_block() - finds allocated block
> 					  fault completes
> 
>   buffer returned is not new or unwritten.
> 
> 
> > In some cases the race can also go the other way around and we lose data
> > written by write.

Yeah, correct. This is ext4 specific issue because ext4 doesn't return
prezeroed blocks from get_block() callback used for dax_io().

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