Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences

From: Dave Chinner <david@fromorbit.com>
To: Jan Kara <jack@suse.cz>
Cc: Ross Zwisler <ross.zwisler@linux.intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	Matthew Wilcox <willy@linux.intel.com>,
	Christoph Hellwig <hch@infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-nvdimm <linux-nvdimm@ml01.01.org>
Subject: Re: [PATCH 2/2] dax: fix bdev NULL pointer dereferences
Date: Sun, 7 Feb 2016 10:15:12 +1100	[thread overview]
Message-ID: <20160206231512.GF31407@dastard> (raw)
In-Reply-To: <20160204091558.GB4956@quack.suse.cz>

On Thu, Feb 04, 2016 at 10:15:58AM +0100, Jan Kara wrote:
> On Wed 03-02-16 13:13:28, Ross Zwisler wrote:
> > Here is the comment from Dave Chinner that had me move to having the calls to
> > dax_writeback_mapping_range() into the generic mm code:
> > 
> >    > Lastly, this flushing really needs to be inside
> >    > filemap_write_and_wait_range(), because we call the writeback code
> >    > from many more places than just fsync to ensure ordering of various
> >    > operations such that files are in known state before proceeding
> >    > (e.g. hole punch).
> > https://lkml.org/lkml/2015/11/16/847
.....
> > > 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.

Just to be clear: this is pretty much what I was implying was
necessary when I said that the DAX flushing needed to be "inside
filemap_write_and_wait_range".  And that's what I thought Ross was
planning on doing after that round discussion.  i.e. Ross said:

"If the race described above isn't an issue then I agree moving this
call out of the filesystems and down into the generic page writeback
code is probably the right thing to do."

https://lkml.org/lkml/2015/11/17/718

Realistically, what Jan is saying in this thread is exactly what I
said we needed to do way back when I first pointed out that fsync
was broken and dirty tracking in the mapping radix tree was still
needed for fsync to work effectively.

Clearly, haven't been following recent developments in this patchset
as closely as I should have - I did not think that such
micro-management was going to be necessary after the discussion taht
was had back in November.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com