Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <A8AFE573-798C-4E07-AD66-A369B3B1CC51@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_FF56063C-3D76-4C8B-AFE0-29BD43C4F7E4";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH 3/4] ext4: Speedup ext4 orphan inode handling
Date:   Thu, 17 Jun 2021 01:44:13 -0600
In-Reply-To: <20210616105655.5129-4-jack@suse.cz>
Cc:     Ted Tso <tytso@mit.edu>, linux-ext4@vger.kernel.org
To:     Jan Kara <jack@suse.cz>
References: <20210616105655.5129-1-jack@suse.cz>
 <20210616105655.5129-4-jack@suse.cz>
Precedence: bulk


--Apple-Mail=_FF56063C-3D76-4C8B-AFE0-29BD43C4F7E4
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Jun 16, 2021, at 4:56 AM, Jan Kara <jack@suse.cz> wrote:
>=20
> Ext4 orphan inode handling is a bottleneck for workloads which heavily
> truncate / unlink small files since it contends on the global
> s_orphan_mutex lock (and generally it's difficult to improve =
scalability
> of the ondisk linked list of orphaned inodes).
>=20
> This patch implements new way of handling orphan inodes. Instead of
> linking orphaned inode into a linked list, we store it's inode number =
in
> a new special file which we call "orphan file". Currently we still
> protect the orphan file with a spinlock for simplicity but even in =
this
> setting we can substantially reduce the length of the critical section
> and thus speedup some workloads.

Is it a single spinlock for the whole file?  Did you consider using
a per-page lock or grouplock?  With a page in the orphan file for each
CPU core, it would basically be lockless.

> Note that the change is backwards compatible when the filesystem is
> clean - the existence of the orphan file is a compat feature, we set
> another ro-compat feature indicating orphan file needs scanning for
> orphaned inodes when mounting filesystem read-write. This ro-compat
> feature gets cleared on unmount / remount read-only.
>=20
> Some performance data from 80 CPU Xeon Server with 512 GB of RAM,
> filesystem located on SSD, average of 5 runs:
>=20
> stress-orphan (microbenchmark truncating files byte-by-byte from N
> processes in parallel)
>=20
> Threads Time            Time
>        Vanilla         Patched
>  1       1.057200        0.945600
>  2       1.680400        1.331800
>  4       2.547000        1.995000
>  8       7.049400        6.424200
> 16      14.827800       14.937600
> 32      40.948200       33.038200
> 64      87.787400       60.823600
> 128     206.504000      122.941400
>=20
> So we can see significant wins all over the board.
>=20
> Signed-off-by: Jan Kara <jack@suse.cz>
>=20
> +static int ext4_orphan_file_add(handle_t *handle, struct inode =
*inode)
> +{
> 	spin_lock(&oi->of_lock);
> +	for (i =3D 0; i < oi->of_blocks && =
!oi->of_binfo[i].ob_free_entries; i++);
> +	if (i =3D=3D oi->of_blocks) {
> +		spin_unlock(&oi->of_lock);
> +		/*
> +		 * For now we don't grow or shrink orphan file. We just =
use
> +		 * whatever was allocated at mke2fs time. The additional
> +		 * credits we would have to reserve for each orphan =
inode
> +		 * operation just don't seem worth it.
> +		 */
> +		return -ENOSPC;
> +	}
> +	oi->of_binfo[i].ob_free_entries--;
> +	spin_unlock(&oi->of_lock);

How do we know how large to make the orphan file at mkfs time?  What if =
it
becomes full during use?  It seems like reserving a fixed number of =
blocks
will invariably be incorrect for the actual workload on the filesystem.

> @@ -49,6 +95,16 @@ int ext4_orphan_add(handle_t *handle, struct inode =
*inode)
> 	ASSERT((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> 		  S_ISLNK(inode->i_mode)) || inode->i_nlink =3D=3D 0);
>=20
> +	if (sbi->s_orphan_info.of_blocks) {
> +		err =3D ext4_orphan_file_add(handle, inode);
> +		/*
> +		 * Fallback to normal orphan list of orphan file is
> +		 * out of space
> +		 */
> +		if (err !=3D -ENOSPC)
> +			return err;
> +	}

This could schedule a task on a workqueue to allocate a few more blocks?
That could easily reserve more credits for this action, without making
the common case more expensive.  Even if it isn't used with the current
mount, it would be available for the next mount (which presumably would
also need additional blocks).

Whether it is worth the complexity to make this fully dynamic, at least
it would auto-tune for the workload placed on this filesystem, and would
not initially be worse than the old single-linked list.

Cheers, Andreas


--Apple-Mail=_FF56063C-3D76-4C8B-AFE0-29BD43C4F7E4
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIyBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAmDK/U0ACgkQcqXauRfM
H+CMMQ/3YRd/Nh7hzHsVmI22OASlhEa/wsTc6yXLjJTvsKJYfESihcAo5tvTwK9C
4cRh7yyIOS461JL9FOgUCu+8SALGaSzlgiugXy9f2vDQP3QH1rilXqeMPTECPnGv
NrmC4GXoK/4xwYNJvFXsJULJlQLpYhEkmla9QNJ/CvSYJ+Yoq//mW71ggmHwZuHd
515aVY92PDu+8tCE0oOSVOo1122nEoE2vaS5sJSi5UMlK16XxMbU5jEQV2OuD/uI
+B8TItmO8F/uK5IMOfkMmPpeVcIVMKGwJ7prJBMaY2YBJpz1yiU70+NF3nvGevTi
3x/x6VECrB0MUiZFYIxuCUs3cWco1MtNSyIFHwHQdD2KiA+rTho2ckQvtfdB23B5
sq4CX59Quhc/CrWejywQjzICL1z91T3TPOloUuQ43Th7S2tAKUfugGgNpuBbYsef
+2ay/R+8E+5HSYRIbw+4IJ97JX38e5LoNpXOAOF8lMz/2yewavZ+XCKNxv/Pmn0b
If7kEAEUBV8LrjAAOqVSzzEnbpSsgFqHJCgZqdoKaf/sIzX4RK4rUGYlGrMfw5kP
EZqx5F9XwblB4oQzVbRpoUgh261X0g/O2K2b5yT3pv+VtYt3MhALrW6LvqOfDs5f
9/NsKqhH8adqNqrAGayFOyfnTqDa1H+gNotyPe08KtfR21KSXg==
=+JV8
-----END PGP SIGNATURE-----

--Apple-Mail=_FF56063C-3D76-4C8B-AFE0-29BD43C4F7E4--