linux-ext4 - Re: [PATCH RFC 2/5] libext2fs: add threading support to the I/O manager abstraction

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <2EFCA6FE-60CC-47BA-A403-592122D5FBCB@dilger.ca>
Date:   Mon, 7 Dec 2020 11:15:30 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        Saranya Muruganandam <saranyamohan@...gle.com>,
        Wang Shilong <wshilong@....com>
Subject: Re: [PATCH RFC 2/5] libext2fs: add threading support to the I/O
 manager abstraction

On Dec 4, 2020, at 9:58 PM, Theodore Ts'o <tytso@....edu> wrote:
> 
> 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.
> 
> Applications using libext2fs can pass in EXT2_FLAG_THREADS to
> ext2fs_open() or ext2fs_open2() to request threading support.
> 
> Signed-off-by: Theodore Ts'o <tytso@....edu>
> ---
> 
> 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 = read(data->dev, data->bounce, channel->block_size);
> 		if (actual != channel->block_size) {
> +			mutex_unlock(data, BOUNCE_MTX);
> 			actual = really_read;
> 			buf -= really_read;
> 			size += really_read;
> @@ -246,6 +303,7 @@ bounce_read:
> 		really_read += actual;
> 		size -= actual;
> 		buf += 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 != 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 = read(data->dev, data->bounce,
> 				      channel->block_size);
> 			if (actual != channel->block_size) {
> 				if (actual < 0) {
> +					mutex_unlock(data, BOUNCE_MTX);
> 					retval = errno;
> 					goto error_out;
> 				}
> @@ -362,6 +424,7 @@ bounce_write:
> 			goto error_out;
> 		}
> 		actual = write(data->dev, data->bounce, channel->block_size);
> +		mutex_unlock(data, BOUNCE_MTX);
> 		if (actual < 0) {
> 			retval = 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 |= CHANNEL_FLAGS_THREADS;
> +		retval = pthread_mutex_init(&data->cache_mutex, NULL);
> +		if (retval)
> +			goto cleanup;
> +		retval = pthread_mutex_init(&data->bounce_mutex, NULL);
> +		if (retval) {
> +			pthread_mutex_destroy(&data->cache_mutex);
> +			goto cleanup;
> +		}
> +		retval = 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






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)