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.
