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: <4F169AE8-BFD2-4EE3-8741-7C75B8764583@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_FB2FB66E-266E-4B42-9615-DD34F3568521";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH RFC 4/5] ext2fs: parallel bitmap loading
Date:   Thu, 10 Dec 2020 17:12:09 -0700
In-Reply-To: <20201205045856.895342-5-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-5-tytso@mit.edu>
Precedence: bulk


--Apple-Mail=_FB2FB66E-266E-4B42-9615-DD34F3568521
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
> From: Wang Shilong <wshilong@ddn.com>
>=20
> In our benchmarking for PiB size filesystem, pass5 takes
> 10446s to finish and 99.5% of time takes on reading bitmaps.
>=20
> It makes sense to reading bitmaps using multiple threads,
> a quickly benchmark show 10446s to 626s with 64 threads.
>=20
> [ 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. ]
>=20
> 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 |=3D =
EXT2_FLAG_BBITMAP_TAIL_PROBLEM;
> +					*tail_flags |=3D =
EXT2_FLAG_BBITMAP_TAIL_PROBLEM;
> 			} else
> 				memset(block_bitmap, 0, block_nbytes);
> 			cnt =3D block_nbytes << 3;
> +#ifdef HAVE_PTHREAD
> +			if (mutex)
> +				pthread_mutex_lock(mutex);
> +#endif
> 			retval =3D =
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 =3D inode_nbytes << 3;
> +			if (mutex)
> +				pthread_mutex_lock(mutex);
> 			retval =3D =
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) =3D=3D 0) ||
> +	    (num_threads =3D=3D 1) || (fs->flags & =
EXT2_FLAG_IMAGE_FILE))
> +		goto fallback;
> +
> +	if (num_threads < 0) {
> +#if defined(HAVE_SYSCONF) && defined(_SC_NPROCESSORS_CONF)
> +		num_threads =3D 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 =3D 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 <=3D 1)
> +			goto fallback;
> +	}

[snip]

> +	/* XXX should save and restore cache setting */
> +	io_channel_set_options(fs->io, "cache=3Don");
> 	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


--Apple-Mail=_FB2FB66E-266E-4B42-9615-DD34F3568521
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/SuVkACgkQcqXauRfM
H+BnkRAArPq+YRXBUn6SNbFhNAf3abdbzT6AOednBotzcSwKK6qdqUA12lXS0BH8
AbN0Nee72UaHJ33LVj9QOtNBtK782LanWm4dpm3x3zoWg63QhcFA8VxZ82yObEGC
PQDa4/v46qlnV7eXfKc+QhZu3Xc5g9iCSgkIgFKP5Xd1xqZ+CYsAXLd2Mojx8d+V
x/HTZJSDK4H2IMZQy5W631W2pAZKONcVVbBbGC2C97tVRcFLPFdnvuGmrvhBkh8p
z2XIcc6ZK9Vn9gk/FstBp4++zN+kvd2Aljk682DI3RgsugfVdkPKGOjzGPjVIYe8
Z2POrP+22HrOL7d0AY+IIkVIQic322IOcTmsLJcsrknkQHtE3NNfoyi3X9dfpxWE
auLK4AtYjB+YHAdXBLiojnWF+UPlgVR/oLSpWG35uNza6e3jChzFhR17tyyCH5LP
aWby5S5rQLNbGuLv9ynclk6iBHulgGeidREgaYOxu1AAx99eaQxjzUrSkIqu5arF
KzBdJwUuw5K2kri3EzIi6RumkBqvDAV5K1resrD+RakVWGrEFdIPc29inRpTP1Ks
izpgxDno51elXEOGHZuMqclMJm00TWGufKmzmw3aStg4rWFdDpk/28pb/nPHpwW4
HErUxGE8DVn0zyUIqO1GTzRJGK8xuojH5c08n9KSMB+zzog08SU=
=VUFx
-----END PGP SIGNATURE-----

--Apple-Mail=_FB2FB66E-266E-4B42-9615-DD34F3568521--