On Mon, Jul 30, 2018 at 12:08 PM, Filipe Manana <fdmanana@xxxxxxxxx> wrote:
> On Mon, Jul 30, 2018 at 11:21 AM, robbieko <robbieko@xxxxxxxxxxxx> wrote:
>> From: Robbie Ko <robbieko@xxxxxxxxxxxx>
>>
>> Commit e9894fd3e3b3 ("Btrfs: fix snapshot vs nocow writting")
>> modified the nocow writeback mechanism, if you create a snapshot,
>> it will always switch to cow writeback.
>>
>> This will cause data loss when there is no space, because
>> when the space is full, the write will not reserve any space, only
>> check if it can be nocow write.
>
> This is a bit vague.
> You need to mention where space reservation does not happen (at the
> time of the write syscall) and why,
> and that the snapshot happens before flushing IO (running dealloc).
> Then when running dealloc we fallback
> to COW and fail.
>
> You also need to tell that although the write syscall did not return
> an error, the writeback will
> fail but a subsequent fsync on the file will return an error (ENOSPC)
> because the writeback set the error
> on the inode's mapping, so it's not completely a silent data loss, as
> for buffered writes there's no guarantee
> that if write syscall returns 0 the data will be persisted
> successfully (that can only be guaranteed if a subsequent
> fsync call returns 0).
>
>>
>> So fix this by first flush the nocow data, and then switch to the
>> cow write.
I'm also not seeing how what you've done is better then we have now
using the root->will_be_snapshotted atomic,
which is essentially used the same way as the new atomic you are
adding, and forces the writeback code no nocow
writes as well.
>
>
> This seems easy to reproduce using deterministic steps.
> Can you please write a test case for fstests?
>
> Also the subject "Btrfs: fix data lose with snapshot when nospace",
> besides the typo (lose -> loss), should be
> more clear like for example "Btrfs: fix data loss for nocow writes
> after snapshot when low on data space".
Also I'm not even sure if I would call it data loss.
If there was no error returned from a subsequent fsync, I would
definitely call it data loss.
So unless the fsync is not returning ENOSPC, I don't see anything that
needs to be fixed.
>
> Thanks.
>>
>> Fixes: e9894fd3e3b3 ("Btrfs: fix snapshot vs nocow writting")
>> Signed-off-by: Robbie Ko <robbieko@xxxxxxxxxxxx>
>> ---
>> fs/btrfs/ctree.h | 1 +
>> fs/btrfs/disk-io.c | 1 +
>> fs/btrfs/inode.c | 26 +++++---------------------
>> fs/btrfs/ioctl.c | 6 ++++++
>> 4 files changed, 13 insertions(+), 21 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 118346a..663ce05 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -1277,6 +1277,7 @@ struct btrfs_root {
>> int send_in_progress;
>> struct btrfs_subvolume_writers *subv_writers;
>> atomic_t will_be_snapshotted;
>> + atomic_t snapshot_force_cow;
>>
>> /* For qgroup metadata reserved space */
>> spinlock_t qgroup_meta_rsv_lock;
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 205092d..5573916 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -1216,6 +1216,7 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
>> atomic_set(&root->log_batch, 0);
>> refcount_set(&root->refs, 1);
>> atomic_set(&root->will_be_snapshotted, 0);
>> + atomic_set(&root->snapshot_force_cow, 0);
>> root->log_transid = 0;
>> root->log_transid_committed = -1;
>> root->last_log_commit = 0;
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index eba61bc..263b852 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -1275,7 +1275,7 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>> u64 disk_num_bytes;
>> u64 ram_bytes;
>> int extent_type;
>> - int ret, err;
>> + int ret;
>> int type;
>> int nocow;
>> int check_prev = 1;
>> @@ -1407,11 +1407,9 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>> * if there are pending snapshots for this root,
>> * we fall into common COW way.
>> */
>> - if (!nolock) {
>> - err = btrfs_start_write_no_snapshotting(root);
>> - if (!err)
>> - goto out_check;
>> - }
>> + if (!nolock &&
>> + unlikely(atomic_read(&root->snapshot_force_cow)))
>> + goto out_check;
>> /*
>> * force cow if csum exists in the range.
>> * this ensure that csum for a given extent are
>> @@ -1420,9 +1418,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>> ret = csum_exist_in_range(fs_info, disk_bytenr,
>> num_bytes);
>> if (ret) {
>> - if (!nolock)
>> - btrfs_end_write_no_snapshotting(root);
>> -
>> /*
>> * ret could be -EIO if the above fails to read
>> * metadata.
>> @@ -1435,11 +1430,8 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>> WARN_ON_ONCE(nolock);
>> goto out_check;
>> }
>> - if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr)) {
>> - if (!nolock)
>> - btrfs_end_write_no_snapshotting(root);
>> + if (!btrfs_inc_nocow_writers(fs_info, disk_bytenr))
>> goto out_check;
>> - }
>> nocow = 1;
>> } else if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>> extent_end = found_key.offset +
>> @@ -1453,8 +1445,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>> out_check:
>> if (extent_end <= start) {
>> path->slots[0]++;
>> - if (!nolock && nocow)
>> - btrfs_end_write_no_snapshotting(root);
>> if (nocow)
>> btrfs_dec_nocow_writers(fs_info, disk_bytenr);
>> goto next_slot;
>> @@ -1476,8 +1466,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>> end, page_started, nr_written, 1,
>> NULL);
>> if (ret) {
>> - if (!nolock && nocow)
>> - btrfs_end_write_no_snapshotting(root);
>> if (nocow)
>> btrfs_dec_nocow_writers(fs_info,
>> disk_bytenr);
>> @@ -1497,8 +1485,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>> ram_bytes, BTRFS_COMPRESS_NONE,
>> BTRFS_ORDERED_PREALLOC);
>> if (IS_ERR(em)) {
>> - if (!nolock && nocow)
>> - btrfs_end_write_no_snapshotting(root);
>> if (nocow)
>> btrfs_dec_nocow_writers(fs_info,
>> disk_bytenr);
>> @@ -1537,8 +1523,6 @@ static noinline int run_delalloc_nocow(struct inode *inode,
>> EXTENT_CLEAR_DATA_RESV,
>> PAGE_UNLOCK | PAGE_SET_PRIVATE2);
>>
>> - if (!nolock && nocow)
>> - btrfs_end_write_no_snapshotting(root);
>> cur_offset = extent_end;
>>
>> /*
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index b077544..43674ef 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -761,6 +761,7 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>> struct btrfs_pending_snapshot *pending_snapshot;
>> struct btrfs_trans_handle *trans;
>> int ret;
>> + bool snapshot_force_cow = false;
>>
>> if (!test_bit(BTRFS_ROOT_REF_COWS, &root->state))
>> return -EINVAL;
>> @@ -787,6 +788,9 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>> if (ret)
>> goto dec_and_free;
>>
>> + atomic_inc(&root->snapshot_force_cow);
>> + snapshot_force_cow = true;
>> +
>> btrfs_wait_ordered_extents(root, U64_MAX, 0, (u64)-1);
>>
>> btrfs_init_block_rsv(&pending_snapshot->block_rsv,
>> @@ -851,6 +855,8 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir,
>> fail:
>> btrfs_subvolume_release_metadata(fs_info, &pending_snapshot->block_rsv);
>> dec_and_free:
>> + if (snapshot_force_cow)
>> + atomic_dec(&root->snapshot_force_cow);
>> if (atomic_dec_and_test(&root->will_be_snapshotted))
>> wake_up_var(&root->will_be_snapshotted);
>> free_pending:
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Filipe David Manana,
>
> “Whether you think you can, or you think you can't — you're right.”
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
