Message-ID: <20160204233801.GA5365@linux.intel.com>
Date:	Thu, 4 Feb 2016 16:38:01 -0700
From:	Ross Zwisler <ross.zwisler@...ux.intel.com>
To:	Jan Kara <jack@...e.cz>
Cc:	Ross Zwisler <ross.zwisler@...ux.intel.com>,
	Dave Chinner <david@...morbit.com>,
	Dan Williams <dan.j.williams@...el.com>,
	Matthew Wilcox <willy@...ux.intel.com>,
	Christoph Hellwig <hch@...radead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	linux-nvdimm <linux-nvdimm@...1.01.org>
Subject: Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences

On Thu, Feb 04, 2016 at 10:15:58AM +0100, Jan Kara wrote:
<>
> Yes, you are right. filemap_write_and_wait_range() actually doesn't
> guarantee data durability. That function only means all dirty data has been
> sent to storage and the storage has acknowledged them. This is noop for
> PMEM. So we are perfectly fine ignoring calls to
> filemap_write_and_wait_range(). What guarantees data durability are only
> ->fsync() and ->sync_fs() calls. But some code could get upset by seeing
> that filemap_write_and_wait_range() didn't actually get rid of dirty pages
> (in some special cases like inode eviction or similar). That's why I'd
> choose one of the two options for consistency:
> 
> 1) Treat inode indexes to flush as close to dirty pages as we can - this
> means inode is dirty with all the tracking associated with it, radix tree
> entries have dirty tag, we get rid of these in ->writepages(). We are close
> to this currently.

I think we're actually pretty far from this, at least for v4.5.  The issue is
that I don't think we can safely clear radix tree dirty entries during the DAX
code that is called via ->writepages().  To do this correctly we would need to
also mark the PTE as clean so that when the userspace process next writes to
their mmap mapping we would get a new fault to make the page writable. This
would allow us to re-dirty the DAX entry in the radix tree.

I implemented code to do this in v2 of my set, but ripped it out in v3:

https://lkml.org/lkml/2015/11/13/759 (DAX fsync v2)
https://lkml.org/lkml/2015/12/8/583  (DAX fsync v3)

The race that compelled this removal is described here:

https://lists.01.org/pipermail/linux-nvdimm/2016-January/004057.html

(sorry for all the links)

Anyway, for v4.5 I think whatever solution we come up with must be okay with
an ever growing list of dirty radix tree entries, as we currently have.  Are
you aware of a reason why this won't work, or was the cleaning of the radix
tree entries just a good goal to have?  (And I agree it is a good goal, I just
don't know how to do it safely.)

> 2) Completely avoid the dirty tracking and writeback code and reimplement
> everything in DAX code.
> 
> Because some hybrid between these is IMHO bound to provoke weird (and very
> hard to find) bugs.
> 
> > > So revisiting the decision I see two options:
> > > 
> > > 1) Move the DAX flushing code from filemap_write_and_wait() into
> > > ->writepages() fs callback. There the filesystem can provide all the
> > > information it needs including bdev, get_block callback, or whatever.
> > 
> > This seems fine as long as we add it to ->fsync as well since ->writepages is
> > never called in that path, and as long as we are okay with skipping DAX
> > writebacks on hole punch, truncate, and block relocation.
> 
> Look at ext4_sync_file() -> filemap_write_and_wait_range() ->
> __filemap_fdatawrite_range() -> do_writepages(). Except those nrpages > 0
> checks which would need to be changed.

Ah, cool, I missed this path.  Thank you for setting me straight.

Powered by blists - more mailing lists