From: David Sterba <firstname.lastname@example.org> To: Filipe Manana <email@example.com> Cc: firstname.lastname@example.org, linux-btrfs <email@example.com> 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.
next prev parent 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 \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.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 \ email@example.com firstname.lastname@example.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