Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331480 > unrolled thread

[PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-10 21:50 +0100
    Re: [PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS Jan Kara <jack@suse.cz> - 2016-02-11 13:50 +0100
      Re: [PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-11 20:50 +0100
        Re: [PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS Dave Chinner <david@fromorbit.com> - 2016-02-11 22:00 +0100
      Re: [PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-12 20:10 +0100
        Re: [PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS Dave Chinner <david@fromorbit.com> - 2016-02-13 03:50 +0100
          Re: [PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-02-13 06:10 +0100

#1331480 — [PATCH v2 0/2] DAX bdev fixes - move flushing calls to FS

During testing of raw block devices + DAX I noticed that the struct
block_device that we were using for DAX operations was incorrect.  For the
fault handlers, etc. we can just get the correct bdev via get_block(),
which is passed in as a function pointer, but for the *sync code and for
sector zeroing we don't have access to get_block().  This is also an issue
for XFS real-time devices, whenever we get those working.

Patch one of this series fixes the DAX sector zeroing code by explicitly
passing in a valid struct block_device.

Patch two of this series fixes DAX *sync support by moving calls to
dax_writeback_mapping_range() out of filemap_write_and_wait_range() and
into the filesystem/block device ->writepages function so that it can
supply us with a valid block device. This also fixes DAX code to properly
flush caches in response to sync(2).

Thanks to Jan Kara for his initial draft of patch 2:
https://lkml.org/lkml/2016/2/9/485

Here are the changes that I've made to that patch:

1) For DAX mappings, only return after calling
dax_writeback_mapping_range() if we encountered an error.  In the non-error
case we still need to write back normal pages, else we lose metadata
updates. 

2) In dax_writeback_mapping_range(), move the new check for 
        if (!mapping->nrexceptional || wbc->sync_mode != WB_SYNC_ALL)
above the i_blkbits check.  In my testing I found cases where
dax_writeback_mapping_range() was called for inodes with i_blkbits !=
PAGE_SHIFT - I'm assuming these are internal metadata inodes?  They have no
exceptional DAX entries to flush, so we have no work to do, but if we
return error from the i_blkbits check we will fail the overall writeback
operation.  Please let me know if it seems wrong for us to be seeing inodes
set to use DAX but with i_blkbits != PAGE_SHIFT and I'll get more info.

3) In filemap_write_and_wait() and filemap_write_and_wait_range(), continue
the writeback in the case that DAX is enabled but we only have a nonzero
mapping->nrpages.  As with 1) and 2), I believe this is necessary to
properly writeback metadata changes.  If this sounds wrong, please let me
know and I'll get more info.

A working tree can be found here:
https://git.kernel.org/cgit/linux/kernel/git/zwisler/linux.git/log/?h=fsync_bdev_v2

Ross Zwisler (2):
  dax: supply DAX clearing code with correct bdev
  dax: move writeback calls into the filesystems

 fs/block_dev.c         | 16 +++++++++++++++-
 fs/dax.c               | 22 ++++++++++++----------
 fs/ext2/inode.c        | 17 +++++++++++++++--
 fs/ext4/inode.c        |  7 +++++++
 fs/xfs/xfs_aops.c      | 11 ++++++++++-
 fs/xfs/xfs_aops.h      |  1 +
 fs/xfs/xfs_bmap_util.c |  3 ++-
 include/linux/dax.h    |  8 +++++---
 mm/filemap.c           | 12 ++++--------
 9 files changed, 71 insertions(+), 26 deletions(-)

-- 
2.5.0

[toc] | [next] | [standalone]

#1331923

On Wed 10-02-16 13:48:54, Ross Zwisler wrote:
> During testing of raw block devices + DAX I noticed that the struct
> block_device that we were using for DAX operations was incorrect.  For the
> fault handlers, etc. we can just get the correct bdev via get_block(),
> which is passed in as a function pointer, but for the *sync code and for
> sector zeroing we don't have access to get_block().  This is also an issue
> for XFS real-time devices, whenever we get those working.
> 
> Patch one of this series fixes the DAX sector zeroing code by explicitly
> passing in a valid struct block_device.
> 
> Patch two of this series fixes DAX *sync support by moving calls to
> dax_writeback_mapping_range() out of filemap_write_and_wait_range() and
> into the filesystem/block device ->writepages function so that it can
> supply us with a valid block device. This also fixes DAX code to properly
> flush caches in response to sync(2).
> 
> Thanks to Jan Kara for his initial draft of patch 2:
> https://lkml.org/lkml/2016/2/9/485
> 
> Here are the changes that I've made to that patch:
> 
> 1) For DAX mappings, only return after calling
> dax_writeback_mapping_range() if we encountered an error.  In the non-error
> case we still need to write back normal pages, else we lose metadata
> updates. 
> 
> 2) In dax_writeback_mapping_range(), move the new check for 
>         if (!mapping->nrexceptional || wbc->sync_mode != WB_SYNC_ALL)
> above the i_blkbits check.  In my testing I found cases where
> dax_writeback_mapping_range() was called for inodes with i_blkbits !=
> PAGE_SHIFT - I'm assuming these are internal metadata inodes?  They have no
> exceptional DAX entries to flush, so we have no work to do, but if we
> return error from the i_blkbits check we will fail the overall writeback
> operation.  Please let me know if it seems wrong for us to be seeing inodes
> set to use DAX but with i_blkbits != PAGE_SHIFT and I'll get more info.

So I'm wondering - how come S_DAX flag got set for inode where i_blkbis !=
PAGE_SHIFT? That would seem to be a bug? I specifically ordered the checks
like this to catch such issues.

> 3) In filemap_write_and_wait() and filemap_write_and_wait_range(), continue
> the writeback in the case that DAX is enabled but we only have a nonzero
> mapping->nrpages.  As with 1) and 2), I believe this is necessary to
> properly writeback metadata changes.  If this sounds wrong, please let me
> know and I'll get more info.

And I'm surprised here as well. If there are dax_mapping() inodes that have
pagecache pages, then we have issues with radix tree handling as well. So
how come dax_mapping() inodes have pages attached? If it is about block
device inodes, then I find it buggy, that S_DAX gets set for such inodes
when filesystem is mounted on them because in such cases we are IMO asking
for data corruption sooner rather than later...

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

[toc] | [prev] | [next] | [standalone]

#1332339

On Thu, Feb 11, 2016 at 01:43:04PM +0100, Jan Kara wrote:
> On Wed 10-02-16 13:48:54, Ross Zwisler wrote:
> > During testing of raw block devices + DAX I noticed that the struct
> > block_device that we were using for DAX operations was incorrect.  For the
> > fault handlers, etc. we can just get the correct bdev via get_block(),
> > which is passed in as a function pointer, but for the *sync code and for
> > sector zeroing we don't have access to get_block().  This is also an issue
> > for XFS real-time devices, whenever we get those working.
> > 
> > Patch one of this series fixes the DAX sector zeroing code by explicitly
> > passing in a valid struct block_device.
> > 
> > Patch two of this series fixes DAX *sync support by moving calls to
> > dax_writeback_mapping_range() out of filemap_write_and_wait_range() and
> > into the filesystem/block device ->writepages function so that it can
> > supply us with a valid block device. This also fixes DAX code to properly
> > flush caches in response to sync(2).
> > 
> > Thanks to Jan Kara for his initial draft of patch 2:
> > https://lkml.org/lkml/2016/2/9/485
> > 
> > Here are the changes that I've made to that patch:
> > 
> > 1) For DAX mappings, only return after calling
> > dax_writeback_mapping_range() if we encountered an error.  In the non-error
> > case we still need to write back normal pages, else we lose metadata
> > updates. 
> > 
> > 2) In dax_writeback_mapping_range(), move the new check for 
> >         if (!mapping->nrexceptional || wbc->sync_mode != WB_SYNC_ALL)
> > above the i_blkbits check.  In my testing I found cases where
> > dax_writeback_mapping_range() was called for inodes with i_blkbits !=
> > PAGE_SHIFT - I'm assuming these are internal metadata inodes?  They have no
> > exceptional DAX entries to flush, so we have no work to do, but if we
> > return error from the i_blkbits check we will fail the overall writeback
> > operation.  Please let me know if it seems wrong for us to be seeing inodes
> > set to use DAX but with i_blkbits != PAGE_SHIFT and I'll get more info.
> 
> So I'm wondering - how come S_DAX flag got set for inode where i_blkbis !=
> PAGE_SHIFT? That would seem to be a bug? I specifically ordered the checks
> like this to catch such issues.

I've isolated this one - this happens for all three filesystems (ext2, ext4 &
XFS), and does indeed have to do with the fact that S_DAX is set for
bdev->bd_inode.

Here is one failure path:

[  102.866637]  [<ffffffff81576d93>] dump_stack+0x85/0xc2
[  102.867101]  [<ffffffff812b9ee0>] dax_writeback_mapping_range+0x60/0xe0
[  102.867738]  [<ffffffff812a1d4f>] blkdev_writepages+0x3f/0x50
[  102.868272]  [<ffffffff811db011>] do_writepages+0x21/0x30
[  102.868784]  [<ffffffff811cb6a6>] __filemap_fdatawrite_range+0xc6/0x100
[  102.869378]  [<ffffffff811cb75a>] filemap_write_and_wait+0x4a/0xa0
[  102.869933]  [<ffffffff812a15e0>] set_blocksize+0x70/0xd0
[  102.870424]  [<ffffffff812a273d>] sb_set_blocksize+0x1d/0x50
[  102.870933]  [<ffffffff8132ac9b>] ext4_fill_super+0x75b/0x3360
[  102.871487]  [<ffffffff81583381>] ? vsnprintf+0x201/0x4c0
[  102.872005]  [<ffffffff815836d9>] ? snprintf+0x49/0x60
[  102.872499]  [<ffffffff81263010>] mount_bdev+0x180/0x1b0
[  102.872981]  [<ffffffff8132a540>] ? ext4_calculate_overhead+0x370/0x370
[  102.873580]  [<ffffffff8131ad95>] ext4_mount+0x15/0x20
[  102.874042]  [<ffffffff81263908>] mount_fs+0x38/0x170
[  102.874524]  [<ffffffff812839db>] vfs_kern_mount+0x6b/0x150
[  102.875041]  [<ffffffff8128670f>] do_mount+0x24f/0xe90
[  102.875508]  [<ffffffff81284444>] ? mntput+0x24/0x40
[  102.875958]  [<ffffffff812399ba>] ? __kmalloc_track_caller+0xea/0x240
[  102.876542]  [<ffffffff812862bc>] ? copy_mount_options+0x2c/0x210
[  102.877087]  [<ffffffff81287695>] SyS_mount+0x95/0xe0
[  102.877573]  [<ffffffff81a6af72>] entry_SYSCALL_64_fastpath+0x12/0x76

In set_blocksize() we are actually updating bdev->bd_inode->i_blkbits to be
12, but before that happens we do a sync_blockdev() with i_blkbits at 10,
which causes the failure.  This can be reproduced easily just by mounting an
ext2 or ext4 filesystem.

I think the plan of unsetting S_DAX on bdev->bd_inode when we mount will save
us from this, as long as we do it super early in the mount process.

[toc] | [prev] | [next] | [standalone]

#1332362

On Thu, Feb 11, 2016 at 12:49:22PM -0700, Ross Zwisler wrote:
> I think the plan of unsetting S_DAX on bdev->bd_inode when we mount will save
> us from this, as long as we do it super early in the mount process.

I think that S_DAX should not be set on the block device by default
in the first place. If we've been surprised by unexpected behaviour,
then I'm sure there are going to be other surprises waiting for us.
DAX default policy should be opt-in, not opt-out.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1333046

On Thu, Feb 11, 2016 at 01:43:04PM +0100, Jan Kara wrote:
> On Wed 10-02-16 13:48:54, Ross Zwisler wrote:
> > 3) In filemap_write_and_wait() and filemap_write_and_wait_range(), continue
> > the writeback in the case that DAX is enabled but we only have a nonzero
> > mapping->nrpages.  As with 1) and 2), I believe this is necessary to
> > properly writeback metadata changes.  If this sounds wrong, please let me
> > know and I'll get more info.
> 
> And I'm surprised here as well. If there are dax_mapping() inodes that have
> pagecache pages, then we have issues with radix tree handling as well. So
> how come dax_mapping() inodes have pages attached? If it is about block
> device inodes, then I find it buggy, that S_DAX gets set for such inodes
> when filesystem is mounted on them because in such cases we are IMO asking
> for data corruption sooner rather than later...

I think I've figured this one out, at least partially.

For ext2 the issues I was seeing were due to the fact that directory inodes
have S_DAX set, but have dirty page cache pages.   In testing with
generic/002, I see two ext2 inodes with S_DAX trying to do a writeback while
they have dirty page cache pages.  The first has i_ino=2, which is the
EXT2_ROOT_INO.

The second inode changes from run to run, but for my last run was 155649.  The
test failed because that directory inode was found to be corrupt by fsck.ext2:

	*** fsck.ext2 output ***
	fsck from util-linux 2.26.2
	e2fsck 1.42.12 (29-Aug-2014)
	Pass 1: Checking inodes, blocks, and sizes
	Pass 2: Checking directory structure
	Directory inode 155649, block #0, offset 0: directory corrupted

If I change the code in ext2_writepages() so that it does the
mpage_writepages() even for DAX inodes, all my xfstests pass.  I'm not sure
this is the right fix, though - should it instead be that ext2 directory
inodes don't have S_DAX set?

A similar problem occurs with ext4, though I haven't yet tracked it down to an
inode type.  It could be that ext4 directory inodes have the same issue, and
Eric Sandeen suggested we might also have an issue with XATTRS attached to
inodes.

As with ext2, if I allow the normal writeback to occur in ext4_writepages()
even for DAX inodes, the issues go away, but I'm not sure whether or not this
is the correct fix.

As far as I can see, XFS does not have these issues - returning immediately
having done just the DAX writeback in xfs_vm_writepages() lets all my xfstests
pass.

For v4.5 should I send out an updated version of this series that does the
regular page writeback for ext2 & ext4, or should we work to clear S_DAX for
regular filesystem inodes that have dirty page cache data?

[toc] | [prev] | [next] | [standalone]

#1333254

On Fri, Feb 12, 2016 at 12:03:20PM -0700, Ross Zwisler wrote:
> On Thu, Feb 11, 2016 at 01:43:04PM +0100, Jan Kara wrote:
> > On Wed 10-02-16 13:48:54, Ross Zwisler wrote:
> > > 3) In filemap_write_and_wait() and filemap_write_and_wait_range(), continue
> > > the writeback in the case that DAX is enabled but we only have a nonzero
> > > mapping->nrpages.  As with 1) and 2), I believe this is necessary to
> > > properly writeback metadata changes.  If this sounds wrong, please let me
> > > know and I'll get more info.
> > 
> > And I'm surprised here as well. If there are dax_mapping() inodes that have
> > pagecache pages, then we have issues with radix tree handling as well. So
> > how come dax_mapping() inodes have pages attached? If it is about block
> > device inodes, then I find it buggy, that S_DAX gets set for such inodes
> > when filesystem is mounted on them because in such cases we are IMO asking
> > for data corruption sooner rather than later...
> 
> I think I've figured this one out, at least partially.
> 
> For ext2 the issues I was seeing were due to the fact that directory inodes
> have S_DAX set, but have dirty page cache pages.   In testing with
> generic/002, I see two ext2 inodes with S_DAX trying to do a writeback while
> they have dirty page cache pages.  The first has i_ino=2, which is the
> EXT2_ROOT_INO.
....
> As far as I can see, XFS does not have these issues - returning immediately
> having done just the DAX writeback in xfs_vm_writepages() lets all my xfstests
> pass.

XFS will not have issues because it does not dirty directory inodes
at the VFS level, nor does it use the page cache for directory data.
However, looking at the code I think it does still set S_DAX on
directory inodes, which it shouldn't be doing.

I've got a couple of fixes I need to do in this area - hopefully
I'll get it done on Monday.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [next] | [standalone]

#1333276

On Sat, Feb 13, 2016 at 01:38:49PM +1100, Dave Chinner wrote:
> On Fri, Feb 12, 2016 at 12:03:20PM -0700, Ross Zwisler wrote:
> > On Thu, Feb 11, 2016 at 01:43:04PM +0100, Jan Kara wrote:
> > > On Wed 10-02-16 13:48:54, Ross Zwisler wrote:
> > > > 3) In filemap_write_and_wait() and filemap_write_and_wait_range(), continue
> > > > the writeback in the case that DAX is enabled but we only have a nonzero
> > > > mapping->nrpages.  As with 1) and 2), I believe this is necessary to
> > > > properly writeback metadata changes.  If this sounds wrong, please let me
> > > > know and I'll get more info.
> > > 
> > > And I'm surprised here as well. If there are dax_mapping() inodes that have
> > > pagecache pages, then we have issues with radix tree handling as well. So
> > > how come dax_mapping() inodes have pages attached? If it is about block
> > > device inodes, then I find it buggy, that S_DAX gets set for such inodes
> > > when filesystem is mounted on them because in such cases we are IMO asking
> > > for data corruption sooner rather than later...
> > 
> > I think I've figured this one out, at least partially.
> > 
> > For ext2 the issues I was seeing were due to the fact that directory inodes
> > have S_DAX set, but have dirty page cache pages.   In testing with
> > generic/002, I see two ext2 inodes with S_DAX trying to do a writeback while
> > they have dirty page cache pages.  The first has i_ino=2, which is the
> > EXT2_ROOT_INO.
> ....
> > As far as I can see, XFS does not have these issues - returning immediately
> > having done just the DAX writeback in xfs_vm_writepages() lets all my xfstests
> > pass.
> 
> XFS will not have issues because it does not dirty directory inodes
> at the VFS level, nor does it use the page cache for directory data.
> However, looking at the code I think it does still set S_DAX on
> directory inodes, which it shouldn't be doing.
> 
> I've got a couple of fixes I need to do in this area - hopefully
> I'll get it done on Monday.

Cool.  I've got a quick patch that stops S_DAX from being set on everything
but regular inodes for ext2 and ext4.  This solved a lot of my xfstests
failures.

Even after that I'm seeing two last failures with ext4 - I'll keep working on
those.

- Ross

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web