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;
Date:   Mon, 23 Nov 2020 22:56:37 -0500
From:   "Theodore Y. Ts'o" <tytso@mit.edu>
To:     Saranya Muruganandam <saranyamohan@google.com>
Cc:     linux-ext4@vger.kernel.org, adilger.kernel@dilger.ca
Subject: Re: [RFC PATCH v3 60/61] e2fsck: propagate number of threads
Message-ID: <20201124035637.GO132317@mit.edu>
References: <20201118153947.3394530-1-saranyamohan@google.com>
 <20201118153947.3394530-61-saranyamohan@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201118153947.3394530-61-saranyamohan@google.com>
Precedence: bulk

On Wed, Nov 18, 2020 at 07:39:46AM -0800, Saranya Muruganandam wrote:
> Sometimes, such as in orphan_inode case, e2fsck_pass1
> is called after reading the block bitmaps. This results in
> reading the block bitmap sequentially and multithreading
> only gets kicked in later. Fix the thread count earlier
> while setting up the file system.

I wonder if read_bitmaps() should be using a different way of
determining how many threads compared to how many threads are used by
e2fsck.

Also, looking more closely, it's not at all clear we should
fs_num_threads should be in the fs structure at all.  It's actually
used in three places.  (a) to influence the default size of a dblist,
if the size is not provided, (b) to influence the default size of an
icount structure, if the size is not provided, (c) to figure out how
many threads are needed in rw_bitmaps.c

(a) and (b) can be dealt with in e2fsck by having e2fsck pass in an
explicit size.  (In fact, for the dblist, we can do a better job by
summing the number of directories in the block group range for each
thread, instead of taking the total number of threads in the group,
and dividing by "the averge number of block groups").

As far as (c) is concerned, perhaps it would be better to make it be
explicit, via a new interface:

errcode_t ext2fs_rw_bitmaps(ext2_filsys fs, int flags, int num_threads);

Where flags is an OR of the following flags:

#define EXT2FS_BITMAPS_WRITE 	      0x0001
#define EXT2FS_BITMAPS_BLOCK 	      0x0002
#define EXT2FS_BITMAPS_INODE 	      0x0004

... and where num_threads is a hint that can be ignored if the
pthreads interface is not available (via a HAVE_PTHREADS autoconf
test).

If we do that, then there's no need to have fs->fs_num_threads at all,
and that can purely be something which is local matter by the
application (e.g., e2fsck, and maybe later other programs like
debugfs's ncheck/icheck commands).

See?  It's really, really important that we have well-defined
interfaces in libext2fs, because that's how we keep tech debt in
e2fsprogs down to a manageable level.

So it might be a good idea to separate out the rw_bitmaps patch() from
the rest of the series, and create a much smaller patch series that
has the autoconf test to see if pthreads are available, and if so, how
it should be linked in (this would also subsume the "fix build for
rpm" patch, except we would make sure that it works for all OS's,
instead of serendiptously when Linux shared libraries were in use, and
then adding a hack when it was noticed that the build configuration
that rpm happened to use wasn't working).

By breaking down the patch series, we can make sure each change is
done cleanly, and portably, and then we can use the individual pieces
to to parallelize e2fsck.  This will probably be better than trying to
deal with this whole patch series as a monolithic thing.

What do folks think?

					- Ted