From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from synology.com ([59.124.61.242]:43534 "EHLO synology.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726143AbeHFEj5 (ORCPT ); Mon, 6 Aug 2018 00:39:57 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Date: Mon, 06 Aug 2018 10:33:04 +0800 From: robbieko To: fdmanana@gmail.com Cc: linux-btrfs Subject: Re: [PATCH] Btrfs: optimization to avoid ENOSPC for nocow writes after snapshot when low on data space In-Reply-To: References: <1533287623-2720-1-git-send-email-robbieko@synology.com> Message-ID: <82d6f5328d3a122eca4a02609f195f4f@synology.com> Sender: linux-btrfs-owner@vger.kernel.org List-ID: Filipe Manana 於 2018-08-03 18:22 寫到: > On Fri, Aug 3, 2018 at 10:13 AM, robbieko > wrote: >> From: Robbie Ko >> >> Commit e9894fd3e3b3 ("Btrfs: fix snapshot vs nocow writting") >> forced writeback fallback to COW when subvolume is snapshotted. > > Commit e9894fd3e3b3 ("Btrfs: fix snapshot vs nocow writting") forced > nocow writes to fallback > to COW, during writeback, when a snapshot is created. This resulted in > writes made before creating > the snapshot to unexpectedly fail with ENOSPC during writeback when > success (0) was returned > to user space through the write system call. > > The steps leading to this problem are: > >> >> 1. When the space is full, write syscall will check if can >> nocow, and space reservation will not happen. > > 1. When it's not possible to allocate data space for a write, the > buffered write path checks if > a NOCOW write is possible. If it is, it will not reserve space and > success (0) is returned to > user space. > >> >> 2. Then snapshot happens before flushing IO (running dealloc), >> we will increase will_be_snapshotted, and then when running >> dealloc we fallback to COW and fail (ENOSPC). > > 2. Then when a snapshot is created, the root's will_be_snapshotted > atomic is incremented and writeback > is triggered for all inode's that belong to the root being > snapshotted. Incrementing that atomic forces > all previous writes to fallback to COW during writeback (running > delalloc). > > 3. This results in the writeback for the inodes to fail and therefore > setting the ENOSPC error in their mappings, > so that a subsequent fsync on them will report the error to user > space. So it's not a completely silent data loss > (since fsync will report ENOSPC) but it's a very unexpected and > undesirable behaviour, because if a clean > shutdown/unmount of the filesystem happens without previous calls to > fsync, it is expected to have the data > present in the files after mounting the filesystem again. > >> >> So fix this by we add a snapshot_force_cow, this is used to >> distinguish between write and writeback. > > So fix this by adding a new atomic named snapshot_force_cow to the > root structure which prevents > this behaviour and works the following way: > >> >> 1. Increase will_be_snapshotted, so that write force to the cow, >> always need space reservation. > > 1. It is incremented when we start to create a snapshot after > triggering writeback and > before waiting for writeback to finish. > >> >> 2. Flushing all dirty pages (running dealloc), then now writeback >> is still flushed in nocow mode, make sure all ditry pages that might >> not reserve space previously have flushed this time otherwise they >> will fallback to cow mode and fail due to no space. > > 2. This new atomic is now what is used by writeback (running delalloc) > to decide whether we need to > fallback to COW or not. Because we incremented this new atomic after > triggering writeback in the snapshot > creation ioctl, we ensure that all buffered writes that happened > before snapshot creation will succeed and > not fallback to COW (which would make them fail with ENOSPC). > >> >> 3. Increase snapshot_force_cow, since all new dirty pages are >> guaranteed space reservation, when running dealloc we can safely >> fallback to COW. > > 3. The existing atomic, will_be_snapshotted, is kept because it is > used to force new buffered writes, that > start after we started snapshotting, to reserve data space even when > NOCOW is possible. > This makes these writes fail early with ENOSPC when there's no > available space to allocate, preventing the > unexpected behaviour of writeback later failing with ENOSPC due to a > fallback to COW mode. > >> >> Fixes: e9894fd3e3b3 ("Btrfs: fix snapshot vs nocow writting") >> Signed-off-by: Robbie Ko >> --- >> fs/btrfs/ctree.h | 1 + >> fs/btrfs/disk-io.c | 1 + >> fs/btrfs/inode.c | 26 +++++--------------------- >> fs/btrfs/ioctl.c | 14 ++++++++++++++ >> 4 files changed, 21 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..42af06b 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; >> @@ -777,6 +778,10 @@ static int create_snapshot(struct btrfs_root >> *root, struct inode *dir, >> goto free_pending; >> } >> >> + /* >> + * We force a new write to reserve space to >> + * avoid the space being full since they'll fallback to cow. >> + */ > > > Force new buffered writes to reserve space even when NOCOW is > possible. This is to > avoid later writeback (running dealloc) to fallback to COW mode and > unexpectedly fail > with ENOSPC. > > >> atomic_inc(&root->will_be_snapshotted); >> smp_mb__after_atomic(); >> /* wait for no snapshot writes */ >> @@ -787,6 +792,13 @@ static int create_snapshot(struct btrfs_root >> *root, struct inode *dir, >> if (ret) >> goto dec_and_free; >> >> + /* >> + * When all previous writes are finished, >> + * we can safely convert writeback to cow. >> + */ > > All previous writes have started writeback in NOCOW mode, so now we > force future writes to > fallback to COW mode during snapshot creation. > > > I would also change the subject from: > > "Btrfs: optimization to avoid ENOSPC for nocow writes after snapshot > when low on data space" > > to: > > "Btrfs: fix unexpected failure of nocow buffered writes after > snapshotting when low on space" > > > With that you can have my: > Reviewed-by: Filipe Manana > > Thanks, great work! I send a new patch with new title "Btrfs: fix unexpected failure of nocow buffered writes after snapshotting when low on space". Thanks > > >> + 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 +863,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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html