Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: David Sterba <dsterba@suse.cz>
To: Filipe Manana <fdmanana@kernel.org>
Cc: dsterba@suse.cz, linux-btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH] Btrfs: fix file corruption after snapshotting
Date: Wed, 27 Feb 2019 18:19:23 +0100
Message-ID: <20190227171923.GA24609@twin.jikos.cz> (raw)
In-Reply-To: <CAL3q7H7Ct-r-Vy737YA7jHmyz0R4dJ6M=5wCMnn7Uvz-Z5EASA@mail.gmail.com>

On Wed, Feb 27, 2019 at 01:42:31PM +0000, Filipe Manana wrote:
> > > > So, how about calling writeback_inodes_sb in that case as a fallback?
> > >
> > > Thought about it, but the reason I didn't do it is that if you
> > > fallback to writeback_unodes_sb, you'll never have the error reported
> > > to the user.
> > > It's very unlikely the user will do an fsync on the snapshot version
> > > of the file, specially if it's a RO snapshot, which would be the only
> > > way to report the error.
> > >
> > >
> > > > I'd really like to avoid returning failure from
> > > > btrfs_start_delalloc_flush so the extra overhead of the writeback (in a
> > > > theoretical error case anyway) should be ok.
> > >
> > > Me too, as long as the error is reported (a message in syslog/dmesg is
> > > very likely to be missed).
> >
> > I probably don't understand. EROFS could be ignored and ENOMEM can be
> > worked around. I don't see what needs to be reported to the user.
> 
> For the same reason we don't ignore the error from the initial flush
> done in the ioctl (at create_snapshot()).
> If the flush fails and we ignore the error, we risk getting a snapshot
> with a corrupted version of files,
> without the user knowing about it.
> 
> Yes, I know here, inside the transaction commit it means ending up in
> an abort and turning the fs into RO mode,
> which is very inconvenient.

create_snapshot is quite different, because the error happens outside of
a transaction.  If it fails at btrfs_start_delalloc_snapshot, it goes
directly to a cleanup and back to userspace. Then it can be restarted
if necessary, the filesystem is still operable (unlike the whole system
that could have the memory exhausted if this was the reason of the
failure).

> It's a choice.

So the way I choose is by the overall impact and try to avoid the abort
if possible.

> Anyway, an updated v2 that ignores any error was just sent.
> This is probably something where different people will always have a
> different preference.

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-04 14:28 fdmanana
2019-02-18 17:11 ` David Sterba
2019-02-18 17:27   ` Filipe Manana
2019-02-27 12:47     ` David Sterba
2019-02-27 13:42       ` Filipe Manana
2019-02-27 17:19         ` David Sterba [this message]
2019-02-27 13:42 ` [PATCH v2] Btrfs: fix file corruption after snapshotting due to mix of buffered/DIO writes fdmanana
2019-02-27 17:26   ` David Sterba
2019-02-27 17:32     ` Filipe Manana
2019-02-27 18:31       ` David Sterba
2019-02-27 18:56         ` Filipe Manana
2019-03-12 17:13           ` David Sterba

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190227171923.GA24609@twin.jikos.cz \
    --to=dsterba@suse.cz \
    --cc=fdmanana@kernel.org \
    --cc=linux-btrfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org linux-btrfs@archiver.kernel.org
	public-inbox-index linux-btrfs


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/ public-inbox