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: <2EFCA6FE-60CC-47BA-A403-592122D5FBCB@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_FCF84F7C-E5F6-4437-9128-B519BF773F79";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH RFC 2/5] libext2fs: add threading support to the I/O
 manager abstraction
Date:   Mon, 7 Dec 2020 11:15:30 -0700
In-Reply-To: <20201205045856.895342-3-tytso@mit.edu>
Cc:     Ext4 Developers List <linux-ext4@vger.kernel.org>,
        Saranya Muruganandam <saranyamohan@google.com>,
        Wang Shilong <wshilong@ddn.com>
To:     Theodore Ts'o <tytso@mit.edu>
References: <20201205045856.895342-1-tytso@mit.edu>
 <20201205045856.895342-3-tytso@mit.edu>
Precedence: bulk


--Apple-Mail=_FCF84F7C-E5F6-4437-9128-B519BF773F79
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Dec 4, 2020, at 9:58 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>=20
> Add initial implementation support for the unix_io manager.
> Applications which want to use threading should pass in
> IO_FLAG_THREADS when opening the channel.  Channels which support
> threading (which as of this commit is unix_io and test_io if the
> backing io_manager supports threading) will set the
> CHANNEL_FLAGS_THREADS bit in io->flags.  Library code or applications
> can test if threading is enabled by checking this flag.
>=20
> Applications using libext2fs can pass in EXT2_FLAG_THREADS to
> ext2fs_open() or ext2fs_open2() to request threading support.
>=20
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
>=20
> diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
> index 628e60c39..9385487d9 100644
> --- a/lib/ext2fs/unix_io.c
> +++ b/lib/ext2fs/unix_io.c
> @@ -232,8 +287,10 @@ static errcode_t raw_read_blk(io_channel channel,
> bounce_read:
> 	while (size > 0) {
> +		mutex_lock(data, BOUNCE_MTX);
> 		actual =3D read(data->dev, data->bounce, =
channel->block_size);
> 		if (actual !=3D channel->block_size) {
> +			mutex_unlock(data, BOUNCE_MTX);
> 			actual =3D really_read;
> 			buf -=3D really_read;
> 			size +=3D really_read;
> @@ -246,6 +303,7 @@ bounce_read:
> 		really_read +=3D actual;
> 		size -=3D actual;
> 		buf +=3D actual;
> +		mutex_unlock(data, BOUNCE_MTX);
> 	}
> 	return 0;

Do you know how often we get into the "bounce_read" IO path?  It seems =
like
locking around the read would kill parallelism, but this code path also
looks like a fallback, but maybe 100% used for blocksize !=3D PAGE_SIZE?

> @@ -341,11 +401,13 @@ static errcode_t raw_write_blk(io_channel =
channel,
> 	 */
> bounce_write:
> 	while (size > 0) {
> +		mutex_lock(data, BOUNCE_MTX);
> 		if (size < channel->block_size) {
> 			actual =3D read(data->dev, data->bounce,
> 				      channel->block_size);
> 			if (actual !=3D channel->block_size) {
> 				if (actual < 0) {
> +					mutex_unlock(data, BOUNCE_MTX);
> 					retval =3D errno;
> 					goto error_out;
> 				}
> @@ -362,6 +424,7 @@ bounce_write:
> 			goto error_out;
> 		}
> 		actual =3D write(data->dev, data->bounce, =
channel->block_size);
> +		mutex_unlock(data, BOUNCE_MTX);
> 		if (actual < 0) {
> 			retval =3D errno;
> 			goto error_out;

Ditto.

> @@ -703,6 +773,25 @@ static errcode_t unix_open_channel(const char =
*name, int fd,
> 			setrlimit(RLIMIT_FSIZE, &rlim);
> 		}
> 	}
> +#endif
> +#ifdef HAVE_PTHREAD
> +	if (flags & IO_FLAG_THREADS) {
> +		io->flags |=3D CHANNEL_FLAGS_THREADS;
> +		retval =3D pthread_mutex_init(&data->cache_mutex, NULL);
> +		if (retval)
> +			goto cleanup;
> +		retval =3D pthread_mutex_init(&data->bounce_mutex, =
NULL);
> +		if (retval) {
> +			pthread_mutex_destroy(&data->cache_mutex);
> +			goto cleanup;
> +		}
> +		retval =3D pthread_mutex_init(&data->stats_mutex, NULL);
> +		if (retval) {
> +			pthread_mutex_destroy(&data->cache_mutex);
> +			pthread_mutex_destroy(&data->bounce_mutex);
> +			goto cleanup;
> +		}
> +	}
> #endif

At one point you talked about using dlopen() or similar to link in the
pthread library only if it is actually needed?  Or is the linkage of
the pthread library avoided by these functions not being called unless
IO_FLAG_THREADS is set?

Cheers, Andreas


--Apple-Mail=_FCF84F7C-E5F6-4437-9128-B519BF773F79
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

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAl/OcUIACgkQcqXauRfM
H+C3Rg//TkCu3KUHGX8dPFy4ehyYDoHRlCpf8hPd8Z8Xoj41gdUp22FXVN58PRUZ
rBBMGw6kulqJTv/ZVT08zjoIgs4TNrlrdma8CNAj6ZGkHT1yUOV9EtTOJcCtl0fg
otR3ObTzfcELTwbgN33WrogrVTvIEFmN4nmK6tn9LTNf+ZhjK/2dRMSVQhmSHqa1
MJ/im8z4ROQ4Vd1W72QAV11+kphUNvXYTEjzLkxPw7xZ6ajp6IO+3Ef4nrw63ztV
1j6tEUOom+RuewNP1ol8peI6uOjQmsqDNCb9n1kKgTwp+sjpZhAbuU5M8oLG+Mji
oda6F5FVtRxkQG1GwNInc3jWPLZ902vE/Aj6ASs8nOlW3SDiQnPp7j4cw5eczR7y
1NmSh9EO5OQdgFCc9b4BSsmhC4WC8XBIOelwvH6YqVhX5Op3jlSLsgQ71lSGaTbi
wcOcsYLNcdUELxokKTsBQAcDFU1vPUkORdcY5X9aN71MSQP+fzTBiTo6j6FE+PTe
wkTVNrG5ofA8vTXGlLqZIW8Ii13AXXawKXORVLPRNWJWnumm1EpFCrAM74CYlwDZ
0OfO9UuIvXKDcn3Hcvn7aryWVZ1RwzAo3RupxnZ9WPlN2bm1JCxsnxTfGOhRZ3I1
i63JM41fqTTM7x9o55BMCdUwpCAQ3ZbtOB1d7Pmv7oyyAp+TbS0=
=y5I3
-----END PGP SIGNATURE-----

--Apple-Mail=_FCF84F7C-E5F6-4437-9128-B519BF773F79--