On Wed, Feb 27, 2019 at 5:25 PM David Sterba <dsterba@xxxxxxx> wrote:
>
> On Wed, Feb 27, 2019 at 01:42:30PM +0000, fdmanana@xxxxxxxxxx wrote:
> > @@ -1897,15 +1899,45 @@ static inline int btrfs_start_delalloc_flush(struct btrfs_fs_info *fs_info)
> > * from already being in a transaction and our join_transaction doesn't
> > * have to re-take the fs freeze lock.
> > */
> > - if (btrfs_test_opt(fs_info, FLUSHONCOMMIT))
> > + if (btrfs_test_opt(fs_info, FLUSHONCOMMIT)) {
> > writeback_inodes_sb(fs_info->sb, WB_REASON_SYNC);
> > + } else {
> > + struct btrfs_pending_snapshot *pending;
> > + struct list_head *head = &trans->transaction->pending_snapshots;
> > +
> > + /*
> > + * Flush dellaloc for any root that is going to be snapshotted.
> > + * This is done to avoid a corrupted version of files, in the
> > + * snapshots, that had both buffered and direct IO writes (even
> > + * if they were done sequentially) due to an unordered update of
> > + * the inode's size on disk.
> > + */
> > + list_for_each_entry(pending, head, list)
> > + btrfs_start_delalloc_snapshot(pending->root);
> > + }
>
> A diff would be better than words, incremental on top of your patch:
You mean, better than a full 2nd version patch I suppose.
>
> @@ -1912,8 +1912,15 @@ static inline int btrfs_start_delalloc_flush(struct btrfs_trans_handle *trans)
> * if they were done sequentially) due to an unordered update of
> * the inode's size on disk.
> */
> - list_for_each_entry(pending, head, list)
> - btrfs_start_delalloc_snapshot(pending->root);
> + list_for_each_entry(pending, head, list) {
> + int ret;
> +
> + ret = btrfs_start_delalloc_snapshot(pending->root);
> + if (ret < 0) {
> + writeback_inodes_sb(fs_info->sb, WB_REASON_SYNC);
> + break;
> + }
What do you expect by falling back to writeback_inodes_sb()?
It all ends up going through the same btrfs writeback path.
And as I left it, if an error happens for one root, it still tries to
flush writeback for all the remaining roots as well, so I don't get it
why you fallback to writeback_inodes_sb().
> + }
> }
> return 0;
> }
> ---
>
> Making a switch by the exact error is probably not necessary and wouldn't be
> future proof anyway.
Not sure I understood that sentence.
Anyway, it's not clear to me whether as it is it's fine, or do you
want an incremental patch, or something else.
thanks
