Re: [PATCH RFC 4/5] ext2fs: parallel bitmap loading

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 4046 bytes --]

On Dec 4, 2020, at 9:58 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> 
> From: Wang Shilong <wshilong@ddn.com>
> 
> In our benchmarking for PiB size filesystem, pass5 takes
> 10446s to finish and 99.5% of time takes on reading bitmaps.
> 
> It makes sense to reading bitmaps using multiple threads,
> a quickly benchmark show 10446s to 626s with 64 threads.
> 
> [ This has all of many bug fixes for rw_bitmaps.c from the original
>  luster patch set collapsed into a single commit.   In addition it has
>  the new ext2fs_rw_bitmaps() api proposed by Ted. ]
> 
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> Signed-off-by: Saranya Muruganandam <saranyamohan@google.com>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

The patch looks generally good.  Unfortunately, I don't have a large
system available to verify the performance at this time, but it looks
close enough to the original code that I don't think there is much
risk of breakage.

One potential cross-platform build issue below, and some cosmetic
suggestions, but you could add:

Reviewed-by: Andreas Dilger <adilger@whamcloud.com>

> @@ -329,12 +369,20 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode, int do_block)
> 				}
> 				if (!bitmap_tail_verify((unsigned char *) block_bitmap,
> 							block_nbytes, fs->blocksize - 1))
> -					tail_flags |= EXT2_FLAG_BBITMAP_TAIL_PROBLEM;
> +					*tail_flags |= EXT2_FLAG_BBITMAP_TAIL_PROBLEM;
> 			} else
> 				memset(block_bitmap, 0, block_nbytes);
> 			cnt = block_nbytes << 3;
> +#ifdef HAVE_PTHREAD
> +			if (mutex)
> +				pthread_mutex_lock(mutex);
> +#endif
> 			retval = ext2fs_set_block_bitmap_range2(fs->block_map,
> 					       blk_itr, cnt, block_bitmap);
> +#ifdef HAVE_PTHREAD
> +			if (mutex)
> +				pthread_mutex_unlock(mutex);
> +#endif

(style) It wouldn't be terrible to have wrappers around these functions
instead of inline #ifdef in the few places they are used, like:

#ifdef HAVE_PTHREAD
static void unix_pthread_mutex_lock(pthread_mutex_t *mutex)
{
	if (mutex)
		pthread_mutex_lock(mutex);
}
static void unix_pthread_mutex_unlock(pthread_mutex_t *mutex)
{
	if (mutex)
		pthread_mutex_unlock(mutex);
}
#else
#define unix_pthread_mutex_lock(mutex) do {} while (0)
#define unix_pthread_mutex_unlock(mutex) do {} while (0)
#endif


> @@ -365,63 +413,229 @@ static errcode_t read_bitmaps(ext2_filsys fs, int do_inode,					memset(inode_bitmap, 0, inode_nbytes);
> 			cnt = inode_nbytes << 3;
> +			if (mutex)
> +				pthread_mutex_lock(mutex);
> 			retval = ext2fs_set_inode_bitmap_range2(fs->inode_map,
> 					       ino_itr, cnt, inode_bitmap);
> +			if (mutex)
> +				pthread_mutex_unlock(mutex);

(minor) These two pthread calls need #ifdef HAVE_PTHREAD or use wrappers.

> +errcode_t ext2fs_rw_bitmaps(ext2_filsys fs, int flags, int num_threads)
> +{
> +#ifdef HAVE_PTHREAD
> +	if (((fs->io->flags & CHANNEL_FLAGS_THREADS) == 0) ||
> +	    (num_threads == 1) || (fs->flags & EXT2_FLAG_IMAGE_FILE))
> +		goto fallback;
> +
> +	if (num_threads < 0) {
> +#if defined(HAVE_SYSCONF) && defined(_SC_NPROCESSORS_CONF)
> +		num_threads = sysconf(_SC_NPROCESSORS_CONF);
> +#else
> +		/*
> +		 * Guess for now; eventually we should probably define
> +		 * ext2fs_get_num_cpus() and teach it how to get this info on
> +		 * MacOS, FreeBSD, etc.
> +		 * ref: https://stackoverflow.com/questions/150355
> +		 */
> +		num_threads = 4;
> +#endif

(style) this #endif wouldn't hurt to have a /* HAVE_SYSCONF */ comment,
but currently isn't too far from the #ifdef though it looks like it may
become further away and harder to track in the future.

> +		if (num_threads <= 1)
> +			goto fallback;
> +	}

[snip]

> +	/* XXX should save and restore cache setting */
> +	io_channel_set_options(fs->io, "cache=on");
> 	return retval;
> +fallback:
> +#endif

(style) this would definitely benefit from a /* HAVE_PTHREAD */ comment,
since it is so far from the original #ifdef.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]