btrfs: prepare and move async csum generation out of experimental feautres
v2 cover btrfs: prepare and move async csum generation out of experimental feautres
[CHANGELOG]
v2:
- Introduce a patch to fix a potential data loss with async direct writes
This is an existing bug, not only affecting the async data csum generation. Introduce a dedicated fix for it.
- Introduce a patch to fixx an ASSERT() false alert race Again an existing bug.
Sashiko exposed several problems related to previous default async csum generation, one fix related to encoded writes is already merged.
The remaining problems are:
- Possible data loss due to fast fsync with async direct writes Fixed by the first patch, not only affecting async csum generation. Although async csum generation will make it more obvious.
- A possible ASSERT() race which can trigger false alert Fixed by the second patch, by moving the ASSERT() out of the workqueue workload.
- Potential workqueue flush problems related to endio_workers Fixed by the 3rd patch, also as a preparation for the incoming default async data csum generation.
Then we can finally enable async data csum generation by default to improve the data write performance, and completely get rid of the old csum offloading behavior.
Qu Wenruo (4):
btrfs: do not allow fast fsync if there is any async dio write
btrfs: fix an ASSERT() race with async csum generation
btrfs: make btrfs_fs_info::workers to be a regular workqueue
btrfs: move async data csum generation out of experimental features
fs/btrfs/Kconfig | 2 - fs/btrfs/bio.c | 139 +------------------------------------------ fs/btrfs/direct-io.c | 11 ++++ fs/btrfs/disk-io.c | 47 ++++++--------- fs/btrfs/file-item.c | 15 ++--- fs/btrfs/file-item.h | 2 +- fs/btrfs/fs.h | 17 +----- fs/btrfs/super.c | 2 +- 8 files changed, 43 insertions(+), 192 deletions(-)
Signature · 1 line
2.56.0
PATCH v2 1/4 btrfs: do not allow fast fsync if there is any async dio write
Sashiko reported a possible race where an async direct write happened, a fast fsync() can return before the write reaches the storage:
CPU0 (io_uring/AIO direct write)| CPU1
--------------------------------+-----------------------------------
btrfs_direct_write()
|-btrfs_dio_write()
| \-__iomap_dio_rw()
| |-btrfs_dio_iomap_begin()
| | |-btrfs_new_extent_direct()
| | ^^^ new extent + ordered extent, data not written yet
| |
| |-btrfs_dio_submit_io()
| | |-btrfs_submit_bbio()
| | ^^^ bio in flight, not finished
| |
| \- return -EIOCBQUEUED
\-btrfs_inode_unlock()
btrfs_sync_file()
|-btrfs_get_ordered_extents_for_logging()
| ^^^grabs the incomplete OE
|
|-filemap_fdatawait_range()
| ^^^no folio under writeback, returns
| immediately
|
|-btrfs_log_dentry_safe()
| | ...
| \-btrfs_log_changed_extents()
| ^^^ logs file extent item for the new extent
| OE only marked PENDING, nobody waits for its IO
|
\-btrfs_sync_log()
^^^ log tree is on disk, data bio still in flight
fsync() returnspower loss, the data bio never reached disk
mount
|-btrfs_recover_log_trees()
^^^ replays the extent item, file now points to unwritten blocks
To prevent such problem, disable fast fsync for async direct writes.
--- fs/btrfs/direct-io.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
fs/btrfs/direct-io.c +11 -0
@@ -974,6 +974,17 @@ ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)if (IS_ERR_OR_NULL (dio)) { ret =PTR_ERR_OR_ZERO (dio); +/* +* The async direct write is not waiting for its IO to finish. +* +* If fsync() went through the fast path, the direct write OE +* will be logged, but the data may not yet reach disks. +* And fsync() returned without waiting for the data. +* +* So for async direct writes, we can not go to fast sync path. +*/ +if (ret == -EIOCBQUEUED ) +btrfs_set_inode_full_sync (BTRFS_I (inode)); }else {/* * If we have a synchronous write, we must make sure the fsync
Signature · 1 line
2.56.0
Re: [PATCH v2 1/4] btrfs: do not allow fast fsync if there is any async dio write
Quoted 60 lines · Qu Wenruo
Sashiko reported a possible race where an async direct write happened, a fast
fsync()can return before the write reaches the storage:CPU0 (io_uring/AIO direct write)| CPU1 --------------------------------+----------------------------------- btrfs_direct_write() |-btrfs_dio_write() | \-__iomap_dio_rw() | |-btrfs_dio_iomap_begin() | | |-btrfs_new_extent_direct() | | ^^^ new extent + ordered extent, data not written yet | | | |-btrfs_dio_submit_io() | | |-btrfs_submit_bbio() | | ^^^ bio in flight, not finished | | | \- return -EIOCBQUEUED \-btrfs_inode_unlock() btrfs_sync_file() |-btrfs_get_ordered_extents_for_logging() | ^^^grabs the incomplete OE | |-filemap_fdatawait_range() | ^^^no folio under writeback, returns | immediately | |-btrfs_log_dentry_safe() | | ... | \-btrfs_log_changed_extents() | ^^^ logs file extent item for the new extent | OE only marked PENDING, nobody waits for its IO | \-btrfs_sync_log() ^^^ log tree is on disk, data bio still in flight fsync() returns power loss, the data bio never reached disk mount |-btrfs_recover_log_trees() ^^^ replays the extent item, file now points to unwritten blocksTo prevent such problem, disable fast fsync for async direct writes.
Fixes 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/direct-io.c | 11 +++++++++++ 1 file changed, 11 insertions(+)fs/btrfs/direct-io.c +1
-0@@ -974,6 +974,17 @@ ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)if (IS_ERR_OR_NULL (dio)) { ret =PTR_ERR_OR_ZERO (dio); +/*
fs/btrfs/direct-io.c ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) · line 987
Quoted 10 lines
fs/btrfs/direct-io.c +10
-0+* The async direct write is not waiting for its IO to finish. +* +* If fsync() went through the fast path, the direct write OE +* will be logged, but the data may not yet reach disks. +* And fsync() returned without waiting for the data. +* +* So for async direct writes, we can not go to fast sync path. +*/ +if (ret == -EIOCBQUEUED ) +btrfs_set_inode_full_sync (BTRFS_I (inode));
Unfortunately btrfs_set_inode_full_sync() is not safe if the caller is only holding a shared inode lock.
So it can still race with fsync.
The shared inode lock for dio writes is introduced by commit e9adabb9712e ("btrfs: use shared lock for direct writes within EOF").
This is making me wonder if the shared inode lock for direct IO is a good idea in the first place.
Or should I change fsync() to wait for direct IOs instead of using btrfs_set_inode_full_sync()?
Thanks,
Qu
Quoted 3 lines
fs/btrfs/direct-io.c +0
-0}else {/* * If we have a synchronous write, we must make sure the fsync
Re: [PATCH v2 1/4] btrfs: do not allow fast fsync if there is any async dio write
Quoted 78 lines · Qu Wenruo
Quoted 60 lines · Qu Wenruo
Sashiko reported a possible race where an async direct write happened, a fast
fsync()can return before the write reaches the storage:CPU0 (io_uring/AIO direct write)| CPU1 --------------------------------+----------------------------------- btrfs_direct_write() |-btrfs_dio_write() | \-__iomap_dio_rw() | |-btrfs_dio_iomap_begin() | | |-btrfs_new_extent_direct() | | ^^^ new extent + ordered extent, data not written yet | | | |-btrfs_dio_submit_io() | | |-btrfs_submit_bbio() | | ^^^ bio in flight, not finished | | | \- return -EIOCBQUEUED \-btrfs_inode_unlock() btrfs_sync_file() |-btrfs_get_ordered_extents_for_logging() | ^^^grabs the incomplete OE | |-filemap_fdatawait_range() | ^^^no folio under writeback, returns | immediately | |-btrfs_log_dentry_safe() | | ... | \-btrfs_log_changed_extents() | ^^^ logs file extent item for the new extent | OE only marked PENDING, nobody waits for its IO | \-btrfs_sync_log() ^^^ log tree is on disk, data bio still in flight fsync() returns power loss, the data bio never reached disk mount |-btrfs_recover_log_trees() ^^^ replays the extent item, file now points to unwritten blocksTo prevent such problem, disable fast fsync for async direct writes.
Fixes 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/direct-io.c | 11 +++++++++++ 1 file changed, 11 insertions(+)fs/btrfs/direct-io.c +1
-0@@ -974,6 +974,17 @@ ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)if (IS_ERR_OR_NULL (dio)) { ret =PTR_ERR_OR_ZERO (dio); +/* fs/btrfs/direct-io.c ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) · line 987
Quoted 10 lines
fs/btrfs/direct-io.c +10
-0+* The async direct write is not waiting for its IO to finish. +* +* If fsync() went through the fast path, the direct write OE +* will be logged, but the data may not yet reach disks. +* And fsync() returned without waiting for the data. +* +* So for async direct writes, we can not go to fast sync path. +*/ +if (ret == -EIOCBQUEUED ) +btrfs_set_inode_full_sync (BTRFS_I (inode));Unfortunately
btrfs_set_inode_full_sync()is not safe if the caller is only holding a shared inode lock.
Quoted 1 line
So it can still race with fsync.
How can it race? An fsync takes the inode lock in exclusive mode.
Quoted 6 lines
The shared inode lock for dio writes is introduced by commit
e9adabb9712e("btrfs: use shared lock for direct writes within EOF").This is making me wonder if the shared inode lock for direct IO is a good idea in the first place.
Why would it not be a good idea?
Quoted 3 lines
Or should I change
fsync()to wait for direct IOs instead of usingbtrfs_set_inode_full_sync()?
Under which conditions is -EIOCBQUEUED returned?
Are these async writes (like io_uring)?
If that's common, then forcing full fsyncs badly affects performance.
We had fast fsync long before 487781796d30.
It got introduced back in 2012 in commit 5dc562c541e1, then removed in b5e6c3e170b770 and then added back (in a slightly different form) in 487781796d30.
So are you sure 487781796d30 is the culprit?
Quoted 8 lines
Thanks,
QuQuoted 3 lines
fs/btrfs/direct-io.c +0
-0}else {/* * If we have a synchronous write, we must make sure the fsync
Re: [PATCH v2 1/4] btrfs: do not allow fast fsync if there is any async dio write
Quoted 94 lines · Filipe Manana
Quoted 78 lines · Qu Wenruo
Quoted 60 lines · Qu Wenruo
Sashiko reported a possible race where an async direct write happened, a fast
fsync()can return before the write reaches the storage:CPU0 (io_uring/AIO direct write)| CPU1 --------------------------------+----------------------------------- btrfs_direct_write() |-btrfs_dio_write() | \-__iomap_dio_rw() | |-btrfs_dio_iomap_begin() | | |-btrfs_new_extent_direct() | | ^^^ new extent + ordered extent, data not written yet | | | |-btrfs_dio_submit_io() | | |-btrfs_submit_bbio() | | ^^^ bio in flight, not finished | | | \- return -EIOCBQUEUED \-btrfs_inode_unlock() btrfs_sync_file() |-btrfs_get_ordered_extents_for_logging() | ^^^grabs the incomplete OE | |-filemap_fdatawait_range() | ^^^no folio under writeback, returns | immediately | |-btrfs_log_dentry_safe() | | ... | \-btrfs_log_changed_extents() | ^^^ logs file extent item for the new extent | OE only marked PENDING, nobody waits for its IO | \-btrfs_sync_log() ^^^ log tree is on disk, data bio still in flight fsync() returns power loss, the data bio never reached disk mount |-btrfs_recover_log_trees() ^^^ replays the extent item, file now points to unwritten blocksTo prevent such problem, disable fast fsync for async direct writes.
Fixes 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/direct-io.c | 11 +++++++++++ 1 file changed, 11 insertions(+)fs/btrfs/direct-io.c +1
-0@@ -974,6 +974,17 @@ ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)if (IS_ERR_OR_NULL (dio)) { ret =PTR_ERR_OR_ZERO (dio); +/* fs/btrfs/direct-io.c ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) · line 987
Quoted 10 lines
fs/btrfs/direct-io.c +10
-0+* The async direct write is not waiting for its IO to finish. +* +* If fsync() went through the fast path, the direct write OE +* will be logged, but the data may not yet reach disks. +* And fsync() returned without waiting for the data. +* +* So for async direct writes, we can not go to fast sync path. +*/ +if (ret == -EIOCBQUEUED ) +btrfs_set_inode_full_sync (BTRFS_I (inode));Unfortunately
btrfs_set_inode_full_sync()is not safe if the caller is only holding a shared inode lock.Quoted 1 line
So it can still race with fsync.
How can it race? An fsync takes the inode lock in exclusive mode.
Quoted 6 lines
The shared inode lock for dio writes is introduced by commit
e9adabb9712e("btrfs: use shared lock for direct writes within EOF").This is making me wonder if the shared inode lock for direct IO is a good idea in the first place.
Why would it not be a good idea?
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
Or should I change
fsync()to wait for direct IOs instead of usingbtrfs_set_inode_full_sync()?Under which conditions is
-EIOCBQUEUEDreturned?
Are these async writes (likeio_uring)?If that's common, then forcing full fsyncs badly affects performance.
And Josef's pending patch actually solves this issue already:
It ensures we wait for IO completion for all ordered extents.
Quoted 16 lines
We had fast fsync long before
487781796d30.
It got introduced back in 2012 in commit5dc562c541e1, then removed inb5e6c3e170b770and then added back (in a slightly different form) in487781796d30.
So are you sure487781796d30is the culprit?Quoted 8 lines
Thanks,
QuQuoted 3 lines
fs/btrfs/direct-io.c +0
-0}else {/* * If we have a synchronous write, we must make sure the fsync
Re: [PATCH v2 1/4] btrfs: do not allow fast fsync if there is any async dio write
In reply to Filipe MananaQuoted 79 lines · Filipe Manana
Quoted 78 lines · Qu Wenruo
Quoted 60 lines · Qu Wenruo
Sashiko reported a possible race where an async direct write happened, a fast
fsync()can return before the write reaches the storage:CPU0 (io_uring/AIO direct write)| CPU1 --------------------------------+----------------------------------- btrfs_direct_write() |-btrfs_dio_write() | \-__iomap_dio_rw() | |-btrfs_dio_iomap_begin() | | |-btrfs_new_extent_direct() | | ^^^ new extent + ordered extent, data not written yet | | | |-btrfs_dio_submit_io() | | |-btrfs_submit_bbio() | | ^^^ bio in flight, not finished | | | \- return -EIOCBQUEUED \-btrfs_inode_unlock() btrfs_sync_file() |-btrfs_get_ordered_extents_for_logging() | ^^^grabs the incomplete OE | |-filemap_fdatawait_range() | ^^^no folio under writeback, returns | immediately | |-btrfs_log_dentry_safe() | | ... | \-btrfs_log_changed_extents() | ^^^ logs file extent item for the new extent | OE only marked PENDING, nobody waits for its IO | \-btrfs_sync_log() ^^^ log tree is on disk, data bio still in flight fsync() returns power loss, the data bio never reached disk mount |-btrfs_recover_log_trees() ^^^ replays the extent item, file now points to unwritten blocksTo prevent such problem, disable fast fsync for async direct writes.
Fixes 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/direct-io.c | 11 +++++++++++ 1 file changed, 11 insertions(+)fs/btrfs/direct-io.c +1
-0@@ -974,6 +974,17 @@ ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from)if (IS_ERR_OR_NULL (dio)) { ret =PTR_ERR_OR_ZERO (dio); +/* fs/btrfs/direct-io.c ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) · line 987
Quoted 10 lines
fs/btrfs/direct-io.c +10
-0+* The async direct write is not waiting for its IO to finish. +* +* If fsync() went through the fast path, the direct write OE +* will be logged, but the data may not yet reach disks. +* And fsync() returned without waiting for the data. +* +* So for async direct writes, we can not go to fast sync path. +*/ +if (ret == -EIOCBQUEUED ) +btrfs_set_inode_full_sync (BTRFS_I (inode));Unfortunately
btrfs_set_inode_full_sync()is not safe if the caller is only holding a shared inode lock.
Quoted 3 lines
Quoted 1 line
So it can still race with fsync.
How can it race? An fsync takes the inode lock in exclusive mode.
CPU0 (sync DIO write within EOF) CPU1 (async DIO write within EOF)btrfs_direct_write() btrfs_direct_write()
btrfs_inode_lock(..., SHARED) btrfs_inode_lock(..., SHARED)
iomap_dio_complete() btrfs_dio_write()
... ...
btrfs_sync_file() btrfs_dio_iomap_begin()
down_write(&i_mmap_lock) btrfs_dio_submit_io()
full_sync = test_bit(...)
/* full_sync is false, skips
btrfs_wait_ordered_range() */
btrfs_set_inode_full_sync()
btrfs_log_dentry_safe()
...
btrfs_log_inode()
test_bit(NEEDS_FULL_SYNC) <-- now true!
clear_bit(NEEDS_FULL_SYNC)
copy_inode_items_to_log()
/* skips btrfs_log_changed_extents()
and purges modified_extents */Quoted 9 lines
Quoted 7 lines
The shared inode lock for dio writes is introduced by commit
e9adabb9712e("btrfs: use shared lock for direct writes within EOF").This is making me wonder if the shared inode lock for direct IO is a good idea in the first place.
Why would it not be a good idea?
OK, then I guess encoded write can be fixed using the same wait, other than using that FULL_SYNC flag.
Quoted 7 lines
Quoted 4 lines
Or should I change
fsync()to wait for direct IOs instead of usingbtrfs_set_inode_full_sync()?Under which conditions is
-EIOCBQUEUEDreturned?
Are these async writes (likeio_uring)?
Yes, AIO/io_uring.
Quoted 8 lines
If that's common, then forcing full fsyncs badly affects performance.
We had fast fsync long before
487781796d30.
It got introduced back in 2012 in commit5dc562c541e1, then removed inb5e6c3e170b770and then added back (in a slightly different form) in487781796d30.
So are you sure487781796d30is the culprit?
At least that's the newest commit that doesn't wait for async direct writes nor encoded writes.
Thanks,
Qu
Quoted 11 lines
Quoted 10 lines
Thanks,
QuQuoted 3 lines
fs/btrfs/direct-io.c +0
-0}else {/* * If we have a synchronous write, we must make sure the fsync
PATCH v2 2/4 btrfs: fix an ASSERT() race with async csum generation
In reply to Qu WenruoIf CONFIG_BTRFS_EXPERIMENTAL is enabled, btrfs will submit a data write bio before its checksum can be generated.
However this introduce a race with zoned devices that can trigger the ASSERT() inside csum_one_bio_work():
CPU0 | CPU1 (kworker)
--------------------------------+-----------------------------------
btrfs_submit_chunk()
|-btrfs_bio_csum()
| \-btrfs_csum_one_bio()
| \-queue_work(..., &bbio->csum_work)
\-btrfs_submit_bio()
\-btrfs_submit_dev_bio()
|-bio->bi_opf &= ~REQ_OP_WRITE;
| csum_one_bio_work()
| \-ASSERT(btrfs_op(&bbio->bio) ==
| BTRFS_MAP_WRITE);
| ^^^ sees REQ_OP_READ (0), BUG()
\-bio->bi_opf |= REQ_OP_ZONE_APPEND;Fix it by moving the ASSERT() into btrfs_csum_one_bio(), so at that time the bi_opf is not yet updated and can always be trusted.
--- fs/btrfs/file-item.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/btrfs/file-item.c +2 -1
@@ -807,7 +807,6 @@ static void csum_one_bio_work(struct work_struct *work) { struct btrfs_bio *bbio = container_of(work, struct btrfs_bio, csum_work); -ASSERT (btrfs_op (&bbio->bio) ==BTRFS_MAP_WRITE );csum_one_bio (bbio);bio_endio (&bbio->bio); }
@@ -824,6 +823,8 @@ int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async)struct btrfs_ordered_sum *sums;unsigned nofs_flag; +ASSERT (btrfs_op (&bbio->bio) ==BTRFS_MAP_WRITE ); + nofs_flag =memalloc_nofs_save (); sums =kvzalloc (btrfs_ordered_sum_size (fs_info, bio->bi_iter.bi_size),GFP_KERNEL );
Signature · 1 line
2.56.0
PATCH v2 3/4 btrfs: make btrfs_fs_info::workers to be a regular workqueue
In reply to Qu WenruoThe workers workqueue is only utilized by csum generation offloading path, which has quite some requirements to meet:
- Non-experimental builds
Experimental builds are already submitting bios before csum generation is finished, which has more concurrency than this method, and still ensures the submission sequence is followed.
- Slow csum algorithms
- Non-sync IOs
Otherwise offloading into a workqueue is only to cause more context switch and overhead.
- Not metadata zoned writes
The biggest one that requires sequential submission is already excluded.
So there is not really much need to use btrfs workqueue for the sequential submission.
Change from a btrfs workqueue to a regular workqueue.
This will make some bios submission out of their original order, but the order will be back after the async csum generation feature is out of experimental, which is very soon.
The main objective of this change is to prepare for the incomine async csum generation as default.
As the previous endio_workers sharing can still cause problems related to the workqueue flush.
--- fs/btrfs/bio.c | 53 ++++++++++++++------------------------------ fs/btrfs/disk-io.c | 14 +++++++----- fs/btrfs/file-item.c | 2 +- fs/btrfs/fs.h | 2 +- fs/btrfs/super.c | 2 +- 5 files changed, 29 insertions(+), 44 deletions(-)
fs/btrfs/bio.c +17 -36
@@ -10,7 +10,6 @@#include "ctree.h" #include "volumes.h" #include "raid56.h" -#include "async-thread.h" #include "dev-replace.h" #include "zoned.h" #include "file-item.h"
@@ -579,7 +578,7 @@ struct async_submit_bio {struct btrfs_io_context *bioc;struct btrfs_io_stripe smap;int mirror_num; -struct btrfs_work work; +struct work_struct work; };/*
@@ -588,45 +587,27 @@ struct async_submit_bio {* sums are attached onto the ordered extent record. * * At IO completion time the csums attached on the ordered extent record are -* inserted into the btree. +* inserted into the tree. */ -static void run_one_async_start (struct btrfs_work *work) +static void run_one_async_submit (struct work_struct *work) {struct async_submit_bio *async = container_of(work, struct async_submit_bio, work); +struct btrfs_bio *bbio = async->bbio; +struct btrfs_io_context *bioc = async->bioc; +struct btrfs_io_stripe smap = async->smap; +const int mirror_num = async->mirror_num; +struct bio *bio = &bbio->bio;int ret; - ret =btrfs_bio_csum (async->bbio); -if (ret) - async->bbio->bio.bi_status =errno_to_blk_status (ret); -} - -/* -* In order to insert checksums into the metadata in large chunks, we wait -* until bio submission time. All the pages in the bio are checksummed and -* sums are attached onto the ordered extent record. -* -* At IO completion time the csums attached on the ordered extent record are -* inserted into the tree. -* -* If called with @do_free == true, then it will free the work struct. -*/ -static void run_one_async_done (struct btrfs_work *work,bool do_free) -{ -struct async_submit_bio *async = - container_of(work, struct async_submit_bio, work); -struct bio *bio = &async->bbio->bio; - -if (do_free) { -kfree (container_of(work ,struct async_submit_bio ,work )); -return ; - } +kfree (async);/* If an error occurred we just want to clean up the bio and move on. */ -if (bio->bi_status) { -btrfs_put_bioc (async->bioc); -btrfs_bio_counter_dec (async->bbio->inode->root->fs_info); -btrfs_bio_end_io (async->bbio, bio->bi_status); + ret =btrfs_bio_csum (bbio); +if (ret) { +btrfs_put_bioc (bioc); +btrfs_bio_counter_dec (bbio->inode->root->fs_info); +btrfs_bio_end_io (bbio,errno_to_blk_status (ret));return ; }
@@ -636,7 +617,7 @@ static void run_one_async_done(struct btrfs_work *work, bool do_free)* context. This changes nothing when cgroups aren't in use. */ bio->bi_opf |=REQ_BTRFS_CGROUP_PUNT ; -btrfs_submit_bio (bio, async->bioc, &async->smap, async->mirror_num); +btrfs_submit_bio (bio, bioc, &smap, mirror_num); }static bool should_async_write (struct btrfs_bio *bbio)
@@ -692,8 +673,8 @@ static bool btrfs_wq_submit_bio(struct btrfs_bio *bbio, async->smap = *smap; async->mirror_num = mirror_num; -btrfs_init_work (&async->work, run_one_async_start, run_one_async_done); -btrfs_queue_work (fs_info->workers, &async->work); +INIT_WORK (&async->work, run_one_async_submit); +queue_work (fs_info->workers, &async->work);return true; }
fs/btrfs/disk-io.c +9 -5
@@ -1829,7 +1829,8 @@ static void btrfs_stop_all_workers(struct btrfs_fs_info *fs_info)if (fs_info->fixup_workers)destroy_workqueue (fs_info->fixup_workers);btrfs_destroy_workqueue (fs_info->delalloc_workers); -btrfs_destroy_workqueue (fs_info->workers); +if (fs_info->workers) +destroy_workqueue (fs_info->workers);if (fs_info->endio_workers)destroy_workqueue (fs_info->endio_workers);if (fs_info->rmw_workers)
@@ -2040,8 +2041,7 @@ static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info)unsigned int flags =WQ_MEM_RECLAIM |WQ_FREEZABLE |WQ_UNBOUND ;unsigned int ordered_flags =WQ_MEM_RECLAIM |WQ_FREEZABLE ; - fs_info->workers = -btrfs_alloc_workqueue (fs_info,"worker" , flags, max_active,16 ); + fs_info->workers =alloc_workqueue ("btrfs-worker" , flags, max_active); fs_info->delalloc_workers = btrfs_alloc_workqueue(fs_info, "delalloc",
@@ -4469,14 +4469,18 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)* We can have ordered extents getting their last reference dropped from * the fs_info->workers queue because for async writes for data bios we * queue a work for that queue, at btrfs_wq_submit_bio(), that runs -* run_one_async_done() which calls btrfs_bio_end_io() in case the bio +* run_one_async_submit() which calls btrfs_bio_end_io() in case the bio * has an error, and that later function can do the final * btrfs_put_ordered_extent() on the ordered extent attached to the bio, * which adds a delayed iput for the inode. So we must flush the queue * so that we don't have delayed iputs after committing the current * transaction below and stopping the cleaner and transaction kthreads. +* +* That queue also runs the async checksum work, csum_one_bio_work(), +* whose bio_endio() can lead to work being queued on the endio queues +* flushed below, so it must be flushed before them. */ -btrfs_flush_workqueue (fs_info->workers); +flush_workqueue (fs_info->workers);/* * When finishing a compressed write bio we schedule a work queue item
fs/btrfs/file-item.c +1 -1
@@ -846,7 +846,7 @@ int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async) }bio_inc_remaining (bio);INIT_WORK (&bbio->csum_work, csum_one_bio_work); -queue_work (fs_info->endio_workers, &bbio->csum_work); +queue_work (fs_info->workers, &bbio->csum_work);return 0 ; }
fs/btrfs/fs.h +1 -1
@@ -704,7 +704,7 @@ struct btrfs_fs_info {* * A third pool does submit_bio to avoid deadlocking with the other two. */ -struct btrfs_workqueue *workers; +struct workqueue_struct *workers;struct btrfs_workqueue *delalloc_workers;struct btrfs_workqueue *flush_workers;struct workqueue_struct *endio_workers;
fs/btrfs/super.c +1 -1
@@ -1235,7 +1235,7 @@ static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info,btrfs_info (fs_info,"resize thread pool %d -> %d" , old_pool_size, new_pool_size); -btrfs_workqueue_set_max (fs_info->workers, new_pool_size); +workqueue_set_max_active (fs_info->workers, new_pool_size);btrfs_workqueue_set_max (fs_info->delalloc_workers, new_pool_size);btrfs_workqueue_set_max (fs_info->caching_workers, new_pool_size);workqueue_set_max_active (fs_info->endio_workers, new_pool_size);
Signature · 1 line
2.56.0
PATCH v2 4/4 btrfs: move async data csum generation out of experimental features
In reply to Qu WenruoThe feature has been under experimental features for a long time, it's time to enable it by default for end users.
This also means we can completely get rid of the current csum offload for slow checksum algorithms, as there is no difference between slow and fast csum generation anyway.
There is a minor behavior change for metadata writes, previously if the checksum algorithm is slow, we will offload the metadata checksum generation into a workqueue, and wait for the csum generation to finish before submission.
That brings no concurrency improvement, and will only introduce extra context switches as overhead. Now metadata checksum generation is always inlined.
--- fs/btrfs/Kconfig | 2 - fs/btrfs/bio.c | 120 +------------------------------------------ fs/btrfs/disk-io.c | 41 +++++---------- fs/btrfs/file-item.c | 10 ++-- fs/btrfs/file-item.h | 2 +- fs/btrfs/fs.h | 15 +----- 6 files changed, 22 insertions(+), 168 deletions(-)
fs/btrfs/Kconfig +0 -2
@@ -111,8 +111,6 @@ config BTRFS_EXPERIMENTAL - huge folios for data - folios can be as large as 2MiB now - - asynchronous checksum generation for data writes - - remap-tree - logical address remapping tree If unsure, say N.
fs/btrfs/bio.c +1 -119
138 lines not on this page · raw message
fs/btrfs/disk-io.c +13 -28
70 lines not on this page · raw message
fs/btrfs/file-item.c +5 -5
@@ -813,8 +813,12 @@ static void csum_one_bio_work(struct work_struct *work)/* * Calculate checksums of the data contained inside a bio. +* +* The checksums are calculated by a work queued on fs_info->workers, so they +* are not valid on return. This must be called before the bio is submitted, as +* the extra bio_inc_remaining() makes the bio completion wait for the work. */ -int btrfs_csum_one_bio (struct btrfs_bio *bbio,bool async) +int btrfs_csum_one_bio (struct btrfs_bio *bbio) {struct btrfs_ordered_extent *ordered = bbio->ordered;struct btrfs_inode *inode = bbio->inode;
@@ -840,10 +844,6 @@ int btrfs_csum_one_bio(struct btrfs_bio *bbio, bool async)btrfs_add_ordered_sum (ordered, sums); bbio->csum_saved_iter = bio->bi_iter; -if (!async) { -csum_one_bio (bbio); -return 0 ; - }bio_inc_remaining (bio);INIT_WORK (&bbio->csum_work, csum_one_bio_work);queue_work (fs_info->workers, &bbio->csum_work);
fs/btrfs/file-item.h +1 -1
@@ -64,7 +64,7 @@ int btrfs_lookup_file_extent(struct btrfs_trans_handle *trans,int btrfs_insert_data_csums (struct btrfs_trans_handle *trans,struct btrfs_root *root,struct btrfs_ordered_sum *sums); -int btrfs_csum_one_bio (struct btrfs_bio *bbio,bool async); +int btrfs_csum_one_bio (struct btrfs_bio *bbio);int btrfs_alloc_dummy_sum (struct btrfs_bio *bbio); int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end, struct list_head *list, int search_commit,
fs/btrfs/fs.h +2 -13
@@ -189,12 +189,6 @@ enum {/* Indicate that the cleaner thread is awake and doing something. */ BTRFS_FS_CLEANER_RUNNING , -/* -* The checksumming has an optimized version and is considered fast, -* so we don't need to offload checksums to workqueues. -*/ -BTRFS_FS_CSUM_IMPL_FAST , -/* Indicate that the discard workqueue can service discards. */ BTRFS_FS_DISCARD_RUNNING,
@@ -696,13 +690,8 @@ struct btrfs_fs_info {struct list_head delalloc_roots;/* -* There is a pool of worker threads for checksumming during writes and -* a pool for checksumming after reads. This is because readers can -* run with FS locks held, and the writers may be waiting for those -* locks. We don't want ordering in the pending list to cause -* deadlocks, and so the two are serviced separately. -* -* A third pool does submit_bio to avoid deadlocking with the other two. +* @workers is the pool of worker threads for checksumming during +* data writes. */ struct workqueue_struct *workers;struct btrfs_workqueue *delalloc_workers;
Signature · 1 line
2.56.0