Re: [PATCH 05/10] btrfs: defer I/O completion based on the btrfs_raid_bio

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/4/29 22:30, Christoph Hellwig wrote:
> Instead of attaching a an extra allocation an indirect call to each
> low-level bio issued by the RAID code, add a work_struct to struct
> btrfs_raid_bio and only defer the per-rbio completion action.  The
> per-bio action for all the I/Os are trivial and can be safely done
> from interrupt context.
>
> As a nice side effect this also allows sharing the boilerplate code
> for the per-bio completions
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

It looks like this patch is causing test failure in btrfs/027, at least
for subapge (64K page size, 4K sectorsize) cases.

Reproducibility is 100% (4/4 tried).

The hanging sub-test case is the repalcing of a missing device in raid5.

The involved dmesg (including the hanging thread dump) is:

[  276.672541] BTRFS warning (device dm-1): read-write for sector size
4096 with page size 65536 is experimental
[  276.744316] BTRFS info (device dm-1): checking UUID tree
[  277.387701] BTRFS info (device dm-1): allowing degraded mounts
[  277.390314] BTRFS info (device dm-1): using free space tree
[  277.392108] BTRFS info (device dm-1): has skinny extents
[  277.393890] BTRFS warning (device dm-1): read-write for sector size
4096 with page size 65536 is experimental
[  277.420922] BTRFS warning (device dm-1): devid 2 uuid
4b67464d-e851-4a88-8765-67b043d4680f is missing
[  277.432694] BTRFS warning (device dm-1): devid 2 uuid
4b67464d-e851-4a88-8765-67b043d4680f is missing
[  277.648326] BTRFS info (device dm-1): dev_replace from <missing disk>
(devid 2) to /dev/mapper/test-scratch5 started
[  297.264371] task:btrfs           state:D stack:    0 pid: 7158 ppid:
  6493 flags:0x0000000c
[  297.280744] Call trace:
[  297.282351]  __switch_to+0xfc/0x160
[  297.284525]  __schedule+0x260/0x61c
[  297.286959]  schedule+0x54/0xc4
[  297.288980]  scrub_enumerate_chunks+0x610/0x760 [btrfs]
[  297.292504]  btrfs_scrub_dev+0x1a0/0x530 [btrfs]
[  297.306738]  btrfs_dev_replace_start+0x2a4/0x2d0 [btrfs]
[  297.310418]  btrfs_dev_replace_by_ioctl+0x48/0x84 [btrfs]
[  297.314026]  btrfs_ioctl_dev_replace+0x1b8/0x210 [btrfs]
[  297.328014]  btrfs_ioctl+0xa48/0x1a70 [btrfs]
[  297.330705]  __arm64_sys_ioctl+0xb4/0x100
[  297.333037]  invoke_syscall+0x50/0x120
[  297.343237]  el0_svc_common.constprop.0+0x4c/0x100
[  297.345716]  do_el0_svc+0x34/0xa0
[  297.347242]  el0_svc+0x34/0xb0
[  297.348763]  el0t_64_sync_handler+0xa8/0x130
[  297.350870]  el0t_64_sync+0x18c/0x190

Mind to take a look on that hang?

Thanks,
Qu

> ---
>   fs/btrfs/ctree.h   |   2 +-
>   fs/btrfs/disk-io.c |  12 ++---
>   fs/btrfs/disk-io.h |   1 -
>   fs/btrfs/raid56.c  | 111 ++++++++++++++++++---------------------------
>   4 files changed, 49 insertions(+), 77 deletions(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 40a6f61559348..4dd0d4a2e7757 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -853,7 +853,7 @@ struct btrfs_fs_info {
>   	struct btrfs_workqueue *flush_workers;
>   	struct btrfs_workqueue *endio_workers;
>   	struct btrfs_workqueue *endio_meta_workers;
> -	struct btrfs_workqueue *endio_raid56_workers;
> +	struct workqueue_struct *endio_raid56_workers;
>   	struct workqueue_struct *rmw_workers;
>   	struct btrfs_workqueue *endio_meta_write_workers;
>   	struct btrfs_workqueue *endio_write_workers;
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 73e12ecc81be1..3c6137734d28c 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -753,14 +753,10 @@ static void end_workqueue_bio(struct bio *bio)
>   			wq = fs_info->endio_meta_write_workers;
>   		else if (end_io_wq->metadata == BTRFS_WQ_ENDIO_FREE_SPACE)
>   			wq = fs_info->endio_freespace_worker;
> -		else if (end_io_wq->metadata == BTRFS_WQ_ENDIO_RAID56)
> -			wq = fs_info->endio_raid56_workers;
>   		else
>   			wq = fs_info->endio_write_workers;
>   	} else {
> -		if (end_io_wq->metadata == BTRFS_WQ_ENDIO_RAID56)
> -			wq = fs_info->endio_raid56_workers;
> -		else if (end_io_wq->metadata)
> +		if (end_io_wq->metadata)
>   			wq = fs_info->endio_meta_workers;
>   		else
>   			wq = fs_info->endio_workers;
> @@ -2274,7 +2270,8 @@ static void btrfs_stop_all_workers(struct btrfs_fs_info *fs_info)
>   	btrfs_destroy_workqueue(fs_info->hipri_workers);
>   	btrfs_destroy_workqueue(fs_info->workers);
>   	btrfs_destroy_workqueue(fs_info->endio_workers);
> -	btrfs_destroy_workqueue(fs_info->endio_raid56_workers);
> +	if (fs_info->endio_raid56_workers)
> +		destroy_workqueue(fs_info->endio_raid56_workers);
>   	if (fs_info->rmw_workers)
>   		destroy_workqueue(fs_info->rmw_workers);
>   	btrfs_destroy_workqueue(fs_info->endio_write_workers);
> @@ -2477,8 +2474,7 @@ static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info)
>   		btrfs_alloc_workqueue(fs_info, "endio-meta-write", flags,
>   				      max_active, 2);
>   	fs_info->endio_raid56_workers =
> -		btrfs_alloc_workqueue(fs_info, "endio-raid56", flags,
> -				      max_active, 4);
> +		alloc_workqueue("btrfs-endio-raid56", flags, max_active);
>   	fs_info->rmw_workers = alloc_workqueue("btrfs-rmw", flags, max_active);
>   	fs_info->endio_write_workers =
>   		btrfs_alloc_workqueue(fs_info, "endio-write", flags,
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 9340e3266e0ac..97255e3d7e524 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -21,7 +21,6 @@ enum btrfs_wq_endio_type {
>   	BTRFS_WQ_ENDIO_DATA,
>   	BTRFS_WQ_ENDIO_METADATA,
>   	BTRFS_WQ_ENDIO_FREE_SPACE,
> -	BTRFS_WQ_ENDIO_RAID56,
>   };
>
>   static inline u64 btrfs_sb_offset(int mirror)
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index a5b623ee6facd..1a3c1a9b10d0b 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -164,6 +164,9 @@ struct btrfs_raid_bio {
>   	atomic_t stripes_pending;
>
>   	atomic_t error;
> +
> +	struct work_struct end_io_work;
> +
>   	/*
>   	 * these are two arrays of pointers.  We allocate the
>   	 * rbio big enough to hold them both and setup their
> @@ -1552,15 +1555,7 @@ static void set_bio_pages_uptodate(struct btrfs_raid_bio *rbio, struct bio *bio)
>   	}
>   }
>
> -/*
> - * end io for the read phase of the rmw cycle.  All the bios here are physical
> - * stripe bios we've read from the disk so we can recalculate the parity of the
> - * stripe.
> - *
> - * This will usually kick off finish_rmw once all the bios are read in, but it
> - * may trigger parity reconstruction if we had any errors along the way
> - */
> -static void raid_rmw_end_io(struct bio *bio)
> +static void raid56_bio_end_io(struct bio *bio)
>   {
>   	struct btrfs_raid_bio *rbio = bio->bi_private;
>
> @@ -1571,23 +1566,34 @@ static void raid_rmw_end_io(struct bio *bio)
>
>   	bio_put(bio);
>
> -	if (!atomic_dec_and_test(&rbio->stripes_pending))
> -		return;
> +	if (atomic_dec_and_test(&rbio->stripes_pending))
> +		queue_work(rbio->bioc->fs_info->endio_raid56_workers,
> +			   &rbio->end_io_work);
> +}
>
> -	if (atomic_read(&rbio->error) > rbio->bioc->max_errors)
> -		goto cleanup;
> +/*
> + * End io handler for the read phase of the rmw cycle.  All the bios here are
> + * physical stripe bios we've read from the disk so we can recalculate the
> + * parity of the stripe.
> + *
> + * This will usually kick off finish_rmw once all the bios are read in, but it
> + * may trigger parity reconstruction if we had any errors along the way
> + */
> +static void raid56_rmw_end_io_work(struct work_struct *work)
> +{
> +	struct btrfs_raid_bio *rbio =
> +		container_of(work, struct btrfs_raid_bio, end_io_work);
> +
> +	if (atomic_read(&rbio->error) > rbio->bioc->max_errors) {
> +		rbio_orig_end_io(rbio, BLK_STS_IOERR);
> +		return;
> +	}
>
>   	/*
> -	 * this will normally call finish_rmw to start our write
> -	 * but if there are any failed stripes we'll reconstruct
> -	 * from parity first
> +	 * This will normally call finish_rmw to start our write but if there
> +	 * are any failed stripes we'll reconstruct from parity first.
>   	 */
>   	validate_rbio_for_rmw(rbio);
> -	return;
> -
> -cleanup:
> -
> -	rbio_orig_end_io(rbio, BLK_STS_IOERR);
>   }
>
>   /*
> @@ -1662,11 +1668,9 @@ static int raid56_rmw_stripe(struct btrfs_raid_bio *rbio)
>   	 * touch it after that.
>   	 */
>   	atomic_set(&rbio->stripes_pending, bios_to_read);
> +	INIT_WORK(&rbio->end_io_work, raid56_rmw_end_io_work);
>   	while ((bio = bio_list_pop(&bio_list))) {
> -		bio->bi_end_io = raid_rmw_end_io;
> -
> -		btrfs_bio_wq_end_io(rbio->bioc->fs_info, bio, BTRFS_WQ_ENDIO_RAID56);
> -
> +		bio->bi_end_io = raid56_bio_end_io;
>   		submit_bio(bio);
>   	}
>   	/* the actual write will happen once the reads are done */
> @@ -2108,25 +2112,13 @@ static void __raid_recover_end_io(struct btrfs_raid_bio *rbio)
>   }
>
>   /*
> - * This is called only for stripes we've read from disk to
> - * reconstruct the parity.
> + * This is called only for stripes we've read from disk to reconstruct the
> + * parity.
>    */
> -static void raid_recover_end_io(struct bio *bio)
> +static void raid_recover_end_io_work(struct work_struct *work)
>   {
> -	struct btrfs_raid_bio *rbio = bio->bi_private;
> -
> -	/*
> -	 * we only read stripe pages off the disk, set them
> -	 * up to date if there were no errors
> -	 */
> -	if (bio->bi_status)
> -		fail_bio_stripe(rbio, bio);
> -	else
> -		set_bio_pages_uptodate(rbio, bio);
> -	bio_put(bio);
> -
> -	if (!atomic_dec_and_test(&rbio->stripes_pending))
> -		return;
> +	struct btrfs_raid_bio *rbio =
> +		container_of(work, struct btrfs_raid_bio, end_io_work);
>
>   	if (atomic_read(&rbio->error) > rbio->bioc->max_errors)
>   		rbio_orig_end_io(rbio, BLK_STS_IOERR);
> @@ -2209,11 +2201,9 @@ static int __raid56_parity_recover(struct btrfs_raid_bio *rbio)
>   	 * touch it after that.
>   	 */
>   	atomic_set(&rbio->stripes_pending, bios_to_read);
> +	INIT_WORK(&rbio->end_io_work, raid_recover_end_io_work);
>   	while ((bio = bio_list_pop(&bio_list))) {
> -		bio->bi_end_io = raid_recover_end_io;
> -
> -		btrfs_bio_wq_end_io(rbio->bioc->fs_info, bio, BTRFS_WQ_ENDIO_RAID56);
> -
> +		bio->bi_end_io = raid56_bio_end_io;
>   		submit_bio(bio);
>   	}
>
> @@ -2582,8 +2572,7 @@ static noinline void finish_parity_scrub(struct btrfs_raid_bio *rbio,
>   	atomic_set(&rbio->stripes_pending, nr_data);
>
>   	while ((bio = bio_list_pop(&bio_list))) {
> -		bio->bi_end_io = raid_write_end_io;
> -
> +		bio->bi_end_io = raid56_bio_end_io;
>   		submit_bio(bio);
>   	}
>   	return;
> @@ -2671,24 +2660,14 @@ static void validate_rbio_for_parity_scrub(struct btrfs_raid_bio *rbio)
>    * This will usually kick off finish_rmw once all the bios are read in, but it
>    * may trigger parity reconstruction if we had any errors along the way
>    */
> -static void raid56_parity_scrub_end_io(struct bio *bio)
> +static void raid56_parity_scrub_end_io_work(struct work_struct *work)
>   {
> -	struct btrfs_raid_bio *rbio = bio->bi_private;
> -
> -	if (bio->bi_status)
> -		fail_bio_stripe(rbio, bio);
> -	else
> -		set_bio_pages_uptodate(rbio, bio);
> -
> -	bio_put(bio);
> -
> -	if (!atomic_dec_and_test(&rbio->stripes_pending))
> -		return;
> +	struct btrfs_raid_bio *rbio =
> +		container_of(work, struct btrfs_raid_bio, end_io_work);
>
>   	/*
> -	 * this will normally call finish_rmw to start our write
> -	 * but if there are any failed stripes we'll reconstruct
> -	 * from parity first
> +	 * This will normally call finish_rmw to start our write, but if there
> +	 * are any failed stripes we'll reconstruct from parity first
>   	 */
>   	validate_rbio_for_parity_scrub(rbio);
>   }
> @@ -2758,11 +2737,9 @@ static void raid56_parity_scrub_stripe(struct btrfs_raid_bio *rbio)
>   	 * touch it after that.
>   	 */
>   	atomic_set(&rbio->stripes_pending, bios_to_read);
> +	INIT_WORK(&rbio->end_io_work, raid56_parity_scrub_end_io_work);
>   	while ((bio = bio_list_pop(&bio_list))) {
> -		bio->bi_end_io = raid56_parity_scrub_end_io;
> -
> -		btrfs_bio_wq_end_io(rbio->bioc->fs_info, bio, BTRFS_WQ_ENDIO_RAID56);
> -
> +		bio->bi_end_io = raid56_bio_end_io;
>   		submit_bio(bio);
>   	}
>   	/* the actual write will happen once the reads are done */