Thread (11 messages) 11 messages, 5 authors, 2021-02-22

Re: [PATCH 5.10.x] btrfs: fix crash after non-aligned direct IO write with O_DSYNC

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-02-22 11:07:51
Also in: stable

On Tue, Feb 16, 2021 at 06:52:21PM +0100, David Sterba wrote:
On Tue, Feb 16, 2021 at 04:34:27PM +0100, Greg KH wrote:
quoted
On Tue, Feb 16, 2021 at 04:15:46PM +0100, David Sterba wrote:
quoted
On Tue, Feb 16, 2021 at 03:50:36PM +0100, Greg KH wrote:
quoted
On Tue, Feb 16, 2021 at 02:40:31PM +0000, fdmanana@kernel.org wrote:
As this is a one-off patch, I need the btrfs maintainers to ack this and
really justify why we can't take the larger patch or patch series here
instead, as that is almost always the correct thing to do instead.
Acked-by: David Sterba <dsterba@suse.com>

The full backport would be patches

ecfdc08b8cc6 btrfs: remove dio iomap DSYNC workaround
a42fa643169d btrfs: call iomap_dio_complete() without inode_lock
502756b38093 btrfs: remove btrfs_inode::dio_sem
e9adabb9712e btrfs: use shared lock for direct writes within EOF
c35237063340 btrfs: push inode locking and unlocking into buffered/direct write
a14b78ad06ab btrfs: introduce btrfs_inode_lock()/unlock()
b8d8e1fd570a btrfs: introduce btrfs_write_check()

and maybe more.

$ git diff b8d8e1fd570a^..ecfdc08b8cc6 | diffstat
 btrfs_inode.h |   10 -
 ctree.h       |    8 +
 file.c        |  338 +++++++++++++++++++++++++++-------------------------------
 inode.c       |   96 +++++++---------
 transaction.h |    1 
 5 files changed, 213 insertions(+), 240 deletions(-)

That seems too much for a backport, the fix Filipe implemented is
simpler and IMO qualifies as the exceptional stable-only patch.
Why is that too much?  For 7 patches that's a small overall diffstat.
And you match identically what is upstream in Linus's tree.  That means
over time, backporting fixing is much easier, and understanding the code
for everyone is simpler.
The changes are not trivial and touch eg. inode locking and other
subsystems (iomap), so they're not self contained inside btrfs. And the
list of possibly related patches is not entirely known at this moment,
the above is an example that was obvious, but Filipe has expressed
doubts that it's complete and I agree.

Backporting them to 5.10.x would need same amount of testing and
validation that the 5.11 version got during the whole development cycle.
quoted
It's almost always better to track what is in Linus's tree than to do
one-off patches as 95% of the time we do one-off patches they are buggy
and cause problems as no one else is running them.
While I understand that concern in general, in this case it's trading
changes by lots of code with a targeted fix with a reproducer, basically
fixing the buggy error handling path.
quoted
So how about sending the above backported series instead please.
Considering the risk I don't want to do that.
Ok, thanks, now queued up.

greg k-h
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help