On Dec 4, 2020, at 9:58 PM, Theodore Ts'o <tytso@xxxxxxx> wrote:
>
> From: Wang Shilong <wshilong@xxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Saranya Muruganandam <saranyamohan@xxxxxxxxxx>
> Signed-off-by: Theodore Ts'o <tytso@xxxxxxx>
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@xxxxxxxxxxxxx>
> @@ -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:
signature.asc
Description: Message signed with OpenPGP
