On Thu, Jan 16, 2020 at 08:49:00AM -0800, Christoph Hellwig wrote:
> On Wed, Jan 15, 2020 at 10:15:58PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > In the previous patch, we solved a stale disk contents exposure problem
> > by forcing the delalloc write path to create unwritten extents, write
> > the data, and convert the extents to written after writeback completes.
> >
> > This is a pretty huge hammer to use, so we'll relax the delalloc write
> > strategy to go straight to written extents (as we once did) if someone
> > tells us to write the entire file to disk. This reopens the exposure
> > window slightly, but we'll only be affected if writeback completes out
> > of order and the system crashes during writeback.
> >
> > Because once again we can map written extents past EOF, we also
> > enlarge the writepages window downward if the window is beyond the
> > on-disk size and there are written extents after the EOF block. This
> > ensures that speculative post-EOF preallocations are not left uncovered.
>
> This does sound really sketchy. Do you have any performance numbers
> justifying something this nasty?
Nope! :D
IIRC Dave also expressed interested in performance impacts the last time
I sent this series, albeit more from the perspective of quantifying how
much pain we'd incur from forcing all writes to perform an unwritten
extent conversion at the end.
FWIW after months of running this on my internal systems, I haven't been
able to quantify any significant difference before and after, even with
rmap enabled. There's slightly more log traffic from the extra
bmbt/rmapbt/inode core updates, but even then the log is fairly good at
deduping repeated updates. Both transactions usually commit before the
log checkpoints.
Frankly I wouldn't apply this patch (or 'xfs: extend the range of
flush_unmap ranges') on the grounds that re-opening potential disclosure
flaws is never worth the risk. I'm also pretty sure that being careful
to convert delalloc data fork extents to unwritten extents fixes the
stale disclosure flaw that Ritesh wrote about in ('iomap: direct-io:
Move inode_dio_begin before filemap_write_and_wait_range').
(As far as ext4 goes, I talked to Jan and Ted this morning and they
seemed to think that they could solve the race on their end by retaining
the unwritten state in the incore extent cache because ext4 apparently
doesn't commit the extent map update transaction until after writeback
completes.)
--D
