Creating gang ABDs for Raidz optional IOs #12333

bwatkinson · 2021-07-07T22:57:15Z

In order to reduce contention on the vq_lock, optional no data blocks
for Raidz are put into gang ABDs. This allows for a reduction on the
number of IO issued down to the children VDEVs reducing contention on
the vq_lock when issuing IO for skip sectors.

Signed-off-by: Brian Atkinson batkinson@lanl.gov

When generating optional IO's for skip sectors with raidz writes they can be placed in a gang ABD along with
the original data. This allows for a single IO to be issued which reduces contention on the child VDEV's vq_lock
when they are placed in the VDEV queues.

Aggregation still takes place in vdev_queue_aggregate() for reads and writes using vq_{read/write}_offset_tree's.

Motivation and Context

Reduces contention on the vq_lock which is a lock than can come under heavy contention as it it is grabbed
for every IO in the VDEV queues.

Description

Before issuing writes down the VDEV children in vdev_raidz_io_start_write() check for the optional skip sectors is check
and placed in gang ABD's with any original data before calling zio_vdev_child_io(). Originally the optional skip sector IO's
were issued in a second loop. However, by only issuing down a single IO to the VDEV children we can reduce contention
on each of the children's vq_locks.

When measuring this with a raidz2 (10+2) the number of times the vq_lock is grabbed in vdev_queue_io() was
reduced by ~7% when writing 128, 16 GB files.

How Has This Been Tested?

Ran zloop.sh for 30 mins
Ran repeated tests with FIO and XDD.

CentOS 8.2
Kernel: 4.18.0-193.el8.x84_84
Server: Gigabyte R72-Z32-00
Single socket AMD EPYC Rome 7502 32c (Hyperthreading 64c)
128 GB RAM, 8x 16 GB 2933 MT/s DDR4

Ran a sweep with PR #10018 rebased off of master from 6-29-2021 with this commit applied on top. By using
O_DIRECT I could directly measure bandwidth improvements because the IO's are issued directly into the
VDEV queues. The following graph shows O_DIRECT with the initial vdev_raidz_io_start_write() (With Gang ABD)
and this commit applied (Without Gang ABD). On average there was around 400 MB/s gained from reducing
contention on the vq_lock. These tests were run using 12 Samsung PM1725a NVMe's in a raidz2 (10+2) VDEV,
recordsize=1M, 1 MB IO requests, and 2 TB of data was written to the ZPool evenly distributed between the
file counts.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

amotin · 2021-07-08T03:07:02Z

I am not very deep in raidz code, but it seems to me that previous code marked optional I/Os as such, and they could be skipped if there is nothing for them to aggregate with. Your code sends all writes as mandatory. In case of large block where every disk get some traffic it is probably not important and can be counted as the price of the optimization, but in case of very small block some disks may get padding writes that may be not needed at all. Am I wrong?

bwatkinson · 2021-07-08T03:46:07Z

I am not very deep in raidz code, but it seems to me that previous code marked optional I/Os as such, and they could be skipped if there is nothing for them to aggregate with. Your code sends all writes as mandatory. In case of large block where every disk get some traffic it is probably not important and can be counted as the price of the optimization, but in case of very small block some disks may get padding writes that may be not needed at all. Am I wrong?

Yes, this is true. So my thought here was to follow the idea of dRAID where skip sectors are just added to the gang ABD. With the O_DIRECT PR #10018, I noticed dRAID was out performing raidz. Looking at flame graphs, raidz IO's were waiting on the children VDEV's vq_lock in vdev_queue_io(). These gang ABD's can still be aggregated in vdev_queue_aggregate() as well.

However, you make a good point and I went ahead and update the case of:
rc->rc_size == 0 which implies rc->rc_abd == NULL
With ZIO_FLAG_OPTIONAL. That now allows these gang ABDs to possibly be skipped as was done previously.

module/zfs/vdev_raidz.c

amotin

3 more nits and I'll be satisfied. ;)

module/zfs/vdev_raidz.c

amotin · 2021-07-14T13:42:36Z

@bwatkinson Thanks. But just to be sure, I've just noticed that vdev_raidz_child_done() is also used by draid, which also uses gangs during write. Have you checked that this change won't affect draid?

bwatkinson · 2021-07-14T17:53:29Z

@bwatkinson Thanks. But just to be sure, I've just noticed that vdev_raidz_child_done() is also used by draid, which also uses gangs during write. Have you checked that this change won't affect draid?

Good point. I went ahead and updated this again. I created a vdev_draid_child_done() callback that does not check for or free the gang ABD.

sempervictus · 2021-08-01T23:20:29Z

This seems pretty handy - @amotin has it as approved, but it doesn't carry the relevant tag yet. @behlendorf: are there any blockers/concerns for merging?

mmaybee · 2021-08-17T17:34:03Z

I would like to see a second review, by someone familiar with the RAIDZ and dRAID code paths (probably @behlendorf or me).

module/zfs/vdev_raidz.c

bwatkinson · 2021-10-06T22:18:02Z

@ahrens would you be willing to review this as well? I wasn't sure how this might affect the raidz expansion code.

behlendorf · 2021-11-05T20:14:11Z

@bwatkinson a rebase of this is probably in order to a fresh test run. @mmaybee since you're familiar with this part of the code can you review this.

bwatkinson · 2021-11-05T22:32:32Z

@bwatkinson a rebase of this is probably in order to a fresh test run. @mmaybee since you're familiar with this part of the code can you review this.

@behlendorf I went ahead and did a rebase.

mmaybee

Looks good. I have a few nit comments.

mmaybee · 2021-11-08T13:46:45Z

module/zfs/vdev_raidz.c

+		/*
+		 * Generate optional I/O for single skip sector to improve
+		 * aggregation contiguity.
+		 */
+		if (rc->rc_size == 0) {
+			ASSERT3P(rc->rc_abd, ==, NULL);
+			zio_nowait(zio_vdev_child_io(zio, NULL, cvd,
+			    rc->rc_offset, NULL, 1ULL << ashift,
+			    zio->io_type, zio->io_priority,
+			    ZIO_FLAG_NODATA | ZIO_FLAG_OPTIONAL, NULL,
+			    NULL));
+		} else {
+			ASSERT3P(rc->rc_abd, !=, NULL);
+			zio_nowait(zio_vdev_child_io(zio, NULL, cvd,
+			    rc->rc_offset, rc->rc_abd,
+			    abd_get_size(rc->rc_abd), zio->io_type,
+			    zio->io_priority, 0, vdev_raidz_child_done, rc));
+		}


I think the ordering here is a little confusing. The primary purpose of this loop is to issue the IOs for the "normal" raid children, and then there is a "special case" if the child is a skip sector. Could we write this like:

if (rc->rc_size > 0) { ...assert and nowait the child io... } else { /* Generate optional write for skip sector */ ...assert and nowait child io... }

I agree. I went ahead and updated this.

mmaybee · 2021-11-08T13:49:23Z

module/zfs/vdev_raidz.c

+	ASSERT3U(rm->rm_nrows, ==, 1);
+
+	/*
+	 * We will pad any parity columns with additional space to account for


nit: drop the "We will": Pad any parity columns with additional space to account for

I updated this.

mmaybee · 2021-11-08T14:22:33Z

module/zfs/vdev_raidz.c

+		 * a single I/O that contains the data and skip sector.
+		 *
+		 * It is important to make sure that rc_size is not updated
+		 * even though we are adding a skip sector to the ABD. With


nit: With -> When

I updated this.

mmaybee · 2021-11-08T14:23:19Z

module/zfs/vdev_raidz.c

+		 * It is important to make sure that rc_size is not updated
+		 * even though we are adding a skip sector to the ABD. With
+		 * calculating the parity in vdev_raidz_generate_parity_row()
+		 * the rc_size is used for iterating through the ABD's. We


nit: for iterating -> to iterate

I updated this.

In order to reduce contention on the vq_lock, optional skip sectors for Raidz writes can be placed into a single IO request. This is done by padding out the linear ABD for a parity column to contain the skip sector and by creating gang ABD to contain the data and skip sector for data columns. The vdev_raidz_map_alloc() function now contains specific functions for both reads and write to allocate the ABD's that will be issued down to the VDEV chldren. Signed-off-by: Brian Atkinson <batkinson@lanl.gov>

In order to reduce contention on the vq_lock, optional skip sectors for Raidz writes can be placed into a single IO request. This is done by padding out the linear ABD for a parity column to contain the skip sector and by creating gang ABD to contain the data and skip sector for data columns. The vdev_raidz_map_alloc() function now contains specific functions for both reads and write to allocate the ABD's that will be issued down to the VDEV chldren. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-By: Mark Maybee <mark.maybee@delphix.com> Signed-off-by: Brian Atkinson <batkinson@lanl.gov> Closes openzfs#12333

behlendorf requested review from behlendorf and ahrens July 7, 2021 23:34

bwatkinson force-pushed the raidz_nodata_gang_abd branch from 8849c6d to 2a4025d Compare July 8, 2021 03:02

bwatkinson force-pushed the raidz_nodata_gang_abd branch from 2a4025d to 848d07a Compare July 8, 2021 04:30

behlendorf added the Status: Code Review Needed Ready for review and testing label Jul 8, 2021

bwatkinson force-pushed the raidz_nodata_gang_abd branch 2 times, most recently from 43ff258 to db51013 Compare July 8, 2021 17:40

amotin reviewed Jul 8, 2021

View reviewed changes

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

bwatkinson force-pushed the raidz_nodata_gang_abd branch from db51013 to 097ad0a Compare July 8, 2021 22:25

amotin reviewed Jul 8, 2021

View reviewed changes

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

bwatkinson force-pushed the raidz_nodata_gang_abd branch from 097ad0a to c4948fe Compare July 9, 2021 00:49

amotin approved these changes Jul 9, 2021

View reviewed changes

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

bwatkinson force-pushed the raidz_nodata_gang_abd branch from c4948fe to dccc659 Compare July 9, 2021 01:52

ahrens assigned mmaybee Jul 9, 2021

behlendorf reviewed Jul 13, 2021

View reviewed changes

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

bwatkinson force-pushed the raidz_nodata_gang_abd branch from dccc659 to b1faf12 Compare July 14, 2021 00:25

amotin reviewed Jul 14, 2021

View reviewed changes

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

bwatkinson force-pushed the raidz_nodata_gang_abd branch from b1faf12 to d6d71d5 Compare July 14, 2021 02:35

bwatkinson force-pushed the raidz_nodata_gang_abd branch from d6d71d5 to f0723f0 Compare July 14, 2021 17:51

amotin approved these changes Jul 14, 2021

View reviewed changes

behlendorf reviewed Aug 23, 2021

View reviewed changes

module/zfs/vdev_raidz.c Outdated Show resolved Hide resolved

bwatkinson force-pushed the raidz_nodata_gang_abd branch 3 times, most recently from 4ac8918 to 2c09b66 Compare September 14, 2021 23:19

bwatkinson force-pushed the raidz_nodata_gang_abd branch from 2c09b66 to 50a4fea Compare September 15, 2021 20:59

bwatkinson requested a review from amotin September 16, 2021 20:50

bwatkinson force-pushed the raidz_nodata_gang_abd branch from 50a4fea to 7dc7156 Compare September 17, 2021 00:33

bwatkinson requested a review from behlendorf September 17, 2021 00:34

amotin approved these changes Sep 20, 2021

View reviewed changes

behlendorf approved these changes Sep 20, 2021

View reviewed changes

bwatkinson force-pushed the raidz_nodata_gang_abd branch from 7dc7156 to 1b0091f Compare November 5, 2021 22:32

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 5, 2021

mmaybee approved these changes Nov 8, 2021

View reviewed changes

bwatkinson force-pushed the raidz_nodata_gang_abd branch from 1b0091f to de3ed2d Compare November 9, 2021 00:30

mmaybee merged commit 345196b into openzfs:master Nov 9, 2021

jumbi77 mentioned this pull request Feb 7, 2022

zfs-2.1.3 patchset #13063

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating gang ABDs for Raidz optional IOs #12333

Creating gang ABDs for Raidz optional IOs #12333

bwatkinson commented Jul 7, 2021 •

edited

Loading

amotin commented Jul 8, 2021 •

edited

Loading

bwatkinson commented Jul 8, 2021 •

edited

Loading

amotin left a comment

amotin commented Jul 14, 2021

bwatkinson commented Jul 14, 2021

sempervictus commented Aug 1, 2021

mmaybee commented Aug 17, 2021

bwatkinson commented Oct 6, 2021

behlendorf commented Nov 5, 2021

bwatkinson commented Nov 5, 2021

mmaybee left a comment

mmaybee Nov 8, 2021

bwatkinson Nov 9, 2021

mmaybee Nov 8, 2021

bwatkinson Nov 9, 2021

mmaybee Nov 8, 2021

bwatkinson Nov 9, 2021

mmaybee Nov 8, 2021

bwatkinson Nov 9, 2021

Creating gang ABDs for Raidz optional IOs #12333

Creating gang ABDs for Raidz optional IOs #12333

Conversation

bwatkinson commented Jul 7, 2021 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

amotin commented Jul 8, 2021 • edited Loading

bwatkinson commented Jul 8, 2021 • edited Loading

amotin left a comment

Choose a reason for hiding this comment

amotin commented Jul 14, 2021

bwatkinson commented Jul 14, 2021

sempervictus commented Aug 1, 2021

mmaybee commented Aug 17, 2021

bwatkinson commented Oct 6, 2021

behlendorf commented Nov 5, 2021

bwatkinson commented Nov 5, 2021

mmaybee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwatkinson commented Jul 7, 2021 •

edited

Loading

amotin commented Jul 8, 2021 •

edited

Loading

bwatkinson commented Jul 8, 2021 •

edited

Loading