[PATCH v2 0/4] btrfs: prepare and move async csum generation out of experimental feautres

[PATCH v2 0/4] btrfs: prepare and move async csum generation out of experimental feautres

[Qu Wenruo]

[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(-)

-- 
2.56.0

[PATCH v2 1/4] btrfs: do not allow fast fsync if there is any async dio write

[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 blocks

To prevent such problem, disable fast fsync for async direct writes.

Fixes: 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")
Link: https://sashiko.dev/#/bug/linux-ee047626-2fa7-46d5-b543-861136993fc7
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/direct-io.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
index ed3914257d39..6e796163015e 100644
--- a/fs/btrfs/direct-io.c
+++ b/fs/btrfs/direct-io.c
@@ -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
-- 
2.56.0

[PATCH v2 2/4] btrfs: fix an ASSERT() race with async csum generation

[Qu Wenruo]

If 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.

Fixes: dd57c78aec39 ("btrfs: introduce btrfs_bio::async_csum")
Link: https://sashiko.dev/#/patchset/cover.1794261158.git.wqu%40suse.com
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file-item.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index ff8f8cad00fc..e19d4c1c754a 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -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);
-- 
2.56.0

[PATCH v2 3/4] btrfs: make btrfs_fs_info::workers to be a regular workqueue

[Qu Wenruo]

The 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.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 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(-)

diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 2e12a6d7e16f..508b44841768 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -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;
 }
 
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 35a43e7cb937..e69055d84606 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -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
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index e19d4c1c754a..af3c73189bc1 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -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;
 }
 
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index 2dd167c32d61..abf1db539c49 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -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;
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index b424cc836bd4..c554391aeb5a 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -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);
-- 
2.56.0

[PATCH v2 4/4] btrfs: move async data csum generation out of experimental features

[Qu Wenruo]

The 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.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 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(-)

diff --git a/fs/btrfs/Kconfig b/fs/btrfs/Kconfig
index 4b10d78ed99b..47faa08d9059 100644
--- a/fs/btrfs/Kconfig
+++ b/fs/btrfs/Kconfig
@@ -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.
diff --git a/fs/btrfs/bio.c b/fs/btrfs/bio.c
index 508b44841768..1b3de0ecb3ec 100644
--- a/fs/btrfs/bio.c
+++ b/fs/btrfs/bio.c
@@ -562,120 +562,7 @@ static int btrfs_bio_csum(struct btrfs_bio *bbio)
 {
 	if (bbio->bio.bi_opf & REQ_META)
 		return btree_csum_one_bio(bbio);
-#ifdef CONFIG_BTRFS_EXPERIMENTAL
-	return btrfs_csum_one_bio(bbio, true);
-#else
-	return btrfs_csum_one_bio(bbio, false);
-#endif
-}
-
-/*
- * Async submit bios are used to offload expensive checksumming onto the worker
- * threads.
- */
-struct async_submit_bio {
-	struct btrfs_bio *bbio;
-	struct btrfs_io_context *bioc;
-	struct btrfs_io_stripe smap;
-	int mirror_num;
-	struct work_struct work;
-};
-
-/*
- * 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.
- */
-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;
-
-	kfree(async);
-
-	/* If an error occurred we just want to clean up the bio and move on. */
-	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;
-	}
-
-	/*
-	 * All of the bios that pass through here are from async helpers.
-	 * Use REQ_BTRFS_CGROUP_PUNT to issue them from the owning cgroup's
-	 * context.  This changes nothing when cgroups aren't in use.
-	 */
-	bio->bi_opf |= REQ_BTRFS_CGROUP_PUNT;
-	btrfs_submit_bio(bio, bioc, &smap, mirror_num);
-}
-
-static bool should_async_write(struct btrfs_bio *bbio)
-{
-	struct btrfs_fs_info *fs_info = bbio->inode->root->fs_info;
-	bool auto_csum_mode = true;
-
-#ifdef CONFIG_BTRFS_EXPERIMENTAL
-	/*
-	 * Write bios will calculate checksum and submit bio at the same time.
-	 * Unless explicitly required don't offload serial csum calculate and bio
-	 * submit into a workqueue.
-	 */
-	return false;
-#endif
-
-	/* Submit synchronously if the checksum implementation is fast. */
-	if (auto_csum_mode && test_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags))
-		return false;
-
-	/*
-	 * Try to defer the submission to a workqueue to parallelize the
-	 * checksum calculation unless the I/O is issued synchronously.
-	 */
-	if (op_is_sync(bbio->bio.bi_opf))
-		return false;
-
-	/* Zoned devices require I/O to be submitted in order. */
-	if ((bbio->bio.bi_opf & REQ_META) && btrfs_is_zoned(fs_info))
-		return false;
-
-	return true;
-}
-
-/*
- * Submit bio to an async queue.
- *
- * Return true if the work has been successfully submitted, else false.
- */
-static bool btrfs_wq_submit_bio(struct btrfs_bio *bbio,
-				struct btrfs_io_context *bioc,
-				struct btrfs_io_stripe *smap, int mirror_num)
-{
-	struct btrfs_fs_info *fs_info = bbio->inode->root->fs_info;
-	struct async_submit_bio *async;
-
-	async = kmalloc_obj(*async, GFP_NOFS);
-	if (!async)
-		return false;
-
-	async->bbio = bbio;
-	async->bioc = bioc;
-	async->smap = *smap;
-	async->mirror_num = mirror_num;
-
-	INIT_WORK(&async->work, run_one_async_submit);
-	queue_work(fs_info->workers, &async->work);
-	return true;
+	return btrfs_csum_one_bio(bbio);
 }
 
 static u64 btrfs_append_map_length(struct btrfs_bio *bbio, u64 map_length)
@@ -784,10 +671,6 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
 		if (!(inode->flags & BTRFS_INODE_NODATASUM) &&
 		    !test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) &&
 		    !btrfs_is_data_reloc_root(inode->root) && !bbio->is_remap) {
-			if (should_async_write(bbio) &&
-			    btrfs_wq_submit_bio(bbio, bioc, &smap, mirror_num))
-				goto done;
-
 			ret = btrfs_bio_csum(bbio);
 			status = errno_to_blk_status(ret);
 			if (status)
@@ -802,7 +685,6 @@ static bool btrfs_submit_chunk(struct btrfs_bio *bbio, int mirror_num)
 	}
 
 	btrfs_submit_bio(bio, bioc, &smap, mirror_num);
-done:
 	return map_length == length;
 
 fail:
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index e69055d84606..14389e7c22b4 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1829,6 +1829,10 @@ 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);
+	/*
+	 * The async csum work on @workers can queue work on the endio queues
+	 * destroyed below, so @workers must be destroyed before them.
+	 */
 	if (fs_info->workers)
 		destroy_workqueue(fs_info->workers);
 	if (fs_info->endio_workers)
@@ -2087,25 +2091,6 @@ static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info)
 	return 0;
 }
 
-static void btrfs_init_csum_hash(struct btrfs_fs_info *fs_info, u16 csum_type)
-{
-	/* Check if the checksum implementation is a fast accelerated one. */
-	switch (csum_type) {
-	case BTRFS_CSUM_TYPE_CRC32:
-		if (crc32_optimizations() & CRC32C_OPTIMIZATION)
-			set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
-		break;
-	case BTRFS_CSUM_TYPE_XXHASH:
-		set_bit(BTRFS_FS_CSUM_IMPL_FAST, &fs_info->flags);
-		break;
-	default:
-		break;
-	}
-
-	btrfs_info(fs_info, "using %s checksum algorithm",
-		   btrfs_super_csum_name(csum_type));
-}
-
 static int btrfs_replay_log(struct btrfs_fs_info *fs_info,
 			    struct btrfs_fs_devices *fs_devices)
 {
@@ -3476,7 +3461,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	fs_info->csum_size = btrfs_super_csum_size(disk_super);
 	fs_info->csum_type = csum_type;
 
-	btrfs_init_csum_hash(fs_info, csum_type);
+	btrfs_info(fs_info, "using %s checksum algorithm",
+		   btrfs_super_csum_name(csum_type));
 
 	/*
 	 * We want to check superblock checksum, the type is stored inside.
@@ -4466,19 +4452,18 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info)
 	btrfs_flush_workqueue(fs_info->delalloc_workers);
 
 	/*
-	 * 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_submit() which calls btrfs_bio_end_io() in case the bio
-	 * has an error, and that later function can do the final
+	 * The fs_info->workers queue runs the async data checksum work, queued
+	 * at btrfs_csum_one_bio(), which calls bio_endio() on the write bio
+	 * after the checksums are calculated. If the device has already
+	 * finished the bio, this is the final bio_endio() which ends up in
+	 * btrfs_finish_ordered_extent(), and that 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.
+	 * That final bio_endio() can also queue work on the endio queues
+	 * flushed below, so this queue must be flushed before them.
 	 */
 	flush_workqueue(fs_info->workers);
 
diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index af3c73189bc1..109806482733 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -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);
diff --git a/fs/btrfs/file-item.h b/fs/btrfs/file-item.h
index 6c678787c770..60a0eb17b3f6 100644
--- a/fs/btrfs/file-item.h
+++ b/fs/btrfs/file-item.h
@@ -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,
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index abf1db539c49..298ebbe71ccf 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -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;
-- 
2.56.0

Re: [PATCH v2 1/4] btrfs: do not allow fast fsync if there is any async dio write

[Qu Wenruo]

在 2026/11/10 16:42, 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 blocks
> 
> To prevent such problem, disable fast fsync for async direct writes.
> 
> Fixes: 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")
> Link: https://sashiko.dev/#/bug/linux-ee047626-2fa7-46d5-b543-861136993fc7
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/direct-io.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
> index ed3914257d39..6e796163015e 100644
> --- a/fs/btrfs/direct-io.c
> +++ b/fs/btrfs/direct-io.c
> @@ -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));

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
>   	} 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

[Filipe Manana]

On Tue, Nov 10, 2026 at 7:06 AM Qu Wenruo <wqu@suse.com> wrote:
>
>
>
> 在 2026/11/10 16:42, 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 blocks
> >
> > To prevent such problem, disable fast fsync for async direct writes.
> >
> > Fixes: 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")
> > Link: https://sashiko.dev/#/bug/linux-ee047626-2fa7-46d5-b543-861136993fc7
> > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > ---
> >   fs/btrfs/direct-io.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
> > index ed3914257d39..6e796163015e 100644
> > --- a/fs/btrfs/direct-io.c
> > +++ b/fs/btrfs/direct-io.c
> > @@ -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));
>
> 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.

How can it race? An fsync takes the inode lock in exclusive mode.

>
> 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?

>
> Or should I change fsync() to wait for direct IOs instead of using
> btrfs_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?


>
> Thanks,
> Qu
> >       } 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

[Filipe Manana]

On Tue, Nov 10, 2026 at 10:01 AM Filipe Manana <fdmanana@kernel.org> wrote:
>
> On Tue, Nov 10, 2026 at 7:06 AM Qu Wenruo <wqu@suse.com> wrote:
> >
> >
> >
> > 在 2026/11/10 16:42, 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 blocks
> > >
> > > To prevent such problem, disable fast fsync for async direct writes.
> > >
> > > Fixes: 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")
> > > Link: https://sashiko.dev/#/bug/linux-ee047626-2fa7-46d5-b543-861136993fc7
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > > ---
> > >   fs/btrfs/direct-io.c | 11 +++++++++++
> > >   1 file changed, 11 insertions(+)
> > >
> > > diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
> > > index ed3914257d39..6e796163015e 100644
> > > --- a/fs/btrfs/direct-io.c
> > > +++ b/fs/btrfs/direct-io.c
> > > @@ -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));
> >
> > 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.
>
> How can it race? An fsync takes the inode lock in exclusive mode.
>
> >
> > 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?
>
> >
> > Or should I change fsync() to wait for direct IOs instead of using
> > btrfs_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.

And Josef's pending patch actually solves this issue already:

https://lore.kernel.org/linux-btrfs/20261103-b4-btrfs-data-integrity-v2-3-577dc2690061@toxicpanda.com/

It ensures we wait for IO completion for all ordered extents.

>
> 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?
>
>
> >
> > Thanks,
> > Qu
> > >       } 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

[Qu Wenruo]

在 2026/11/10 20:31, Filipe Manana 写道:
> On Tue, Nov 10, 2026 at 7:06 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>>
>>
>> 在 2026/11/10 16:42, 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 blocks
>>>
>>> To prevent such problem, disable fast fsync for async direct writes.
>>>
>>> Fixes: 487781796d30 ("btrfs: make fast fsyncs wait only for writeback")
>>> Link: https://sashiko.dev/#/bug/linux-ee047626-2fa7-46d5-b543-861136993fc7
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>    fs/btrfs/direct-io.c | 11 +++++++++++
>>>    1 file changed, 11 insertions(+)
>>>
>>> diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
>>> index ed3914257d39..6e796163015e 100644
>>> --- a/fs/btrfs/direct-io.c
>>> +++ b/fs/btrfs/direct-io.c
>>> @@ -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));
>>
>> 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.
> 
> 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 */

> 
>>
>> 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.

> 
>>
>> Or should I change fsync() to wait for direct IOs instead of using
>> btrfs_set_inode_full_sync()?
> 
> Under which conditions is  -EIOCBQUEUED returned?
> Are these async writes (like io_uring)?

Yes, AIO/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?

At least that's the newest commit that doesn't wait for async direct 
writes nor encoded writes.

Thanks,
Qu

> 
> 
>>
>> Thanks,
>> Qu
>>>        } else {
>>>                /*
>>>                 * If we have a synchronous write, we must make sure the fsync
>>
>>
>