[RFC] ext2fs: parallel bitmap loading

This is a patch that is part of the parallel e2fsck series that Shilong is working on,
and does not work by itself, but was requested during discussion on the ext4
concall today.


Cheers, Andreas
========================================

From dba9e324999727e6cc2ca158cc01f0053a701db9 Mon Sep 17 00:00:00 2001
From: Wang Shilong <wshilong@ddn.com>
Date: Thu, 3 Sep 2020 10:51:49 +0800
Subject: [PATCH] RFC ext2fs: parallel bitmap loading

In our benchmarking for PiB size filesystem, pass5 takes
10446s to finish and 99.5% of time taken on reading bitmaps.

It makes sense to read the bitmaps using multiple threads,
a quickly benchmark show 10446s to 883s with 64 threads.

Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
 lib/ext2fs/rw_bitmaps.c | 260 ++++++++++++++++++++++++++++++++++------
 1 file changed, 224 insertions(+), 36 deletions(-)

--
2.25.4

Cheers, Andreas

On Fri, Sep 04, 2020 at 03:34:26PM -0600, Andreas Dilger wrote:
> This is a patch that is part of the parallel e2fsck series that Shilong is working on,
> and does not work by itself, but was requested during discussion on the ext4
> concall today.

Andreas, thanks for sending this patch.  (Also available at[1].)

[1] https://lore.kernel.org/linux-ext4/132401FE-6D25-41B3-99D1-50E7BC746237@dilger.ca/

I took look at it, and there are a number of issues with it.  First of
all, there seems to be an assumption that (a) the number of threads is
less than the number of block groups, and (b) the number of threads
can evenly divide the number of block groups.  So for example, if the
number of block groups is prime, or if you are trying to use say, 8 or
16 threads, and the number of block groups is odd, the code in
question will not do the right thing.

(a) meant that attempting to run the e2fsprogs regression test suite
caused most of the test cases to fail with e2fsck crashing due to
buffer overruns.  I fixed this by changing the number of threads to be
16, or if 16 was greater than the number of block groups, to be the
number of block groups, just for debugging purposes.  However, there
were still a few regression test failures.

I also then tried to use a file system that we had been using for
testing fragmentation issues.  The file system was creating a 10GB
virtual disk, and then running these commands:

   DEV=/dev/sdc
   mke2fs -t ext4 $DEV 10G
   mount $DEV /mnt
   pushd /mnt
   for t in $(seq 1 6144) ; do
       for i in $(seq 1 25) ; do
           fallocate tb$t-8mb-$i -l 8M
       done
       for i in $(seq 1 2) ; do
           fallocate tb$t-400mb-$i -l 400M
       done
   done
   popd
   umount /mnt

With the patch applied, all of the threads failed with error code 22
(EINVAL), except for one which failed with a bad block group checksum
error.  I haven't had a chance to dig into further; but I was hoping
that Shilong and/or Saranya might be able to take closer look at that.

But the other thing that we might want to consider is to add
demand-loading of the block (or inode) bitmap.  We got a complaint
that "e2fsck -E journal_only" was super-slow whereas running the
journal by mounting and unmounting the file system was much faster.
The reason, of course, was because the kernel was only reading those
bitmap blocks that are needed to be modified by the orphaned inode
processing, whereas with e2fsprogs, we have to read in all of the
bitmap blocks whether this is necessary or not.

So another idea that we've talked about is teaching libext2fs to be
able to demand load the bitmap, and then when we write out the block
bitmap, we only need to write out those blocks that were loaded.  This
would also speed up running debugfs to examine the file system, as
well as running fuse2fs.  Fortunately, we have abstractions in front
of all of the bitmap accessor functions, and the code paths that would
need to be changed to add demand-loading of bitmaps should be mostly
exclusive of the changes needed for parallel bitmap loading.  So if
Shilong has time to look at making the parallel bitmap loader more
robust, perhaps Saranya could work on the demand-loading idea.

Or if Shilong doesn't have time to try to polish this parallel bitmap
loading changes, we could have Saranya look at clean it up --- since
regardless of whether we implement demand-loading or not, parallel
bitmap reading is going to be useful for some use cases (e.g., a full
fsck, dumpe2fs, or e2image).

What do folks think?

						- Ted

On Sep 16, 2020, at 3:03 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> 
> On Fri, Sep 04, 2020 at 03:34:26PM -0600, Andreas Dilger wrote:
>> This is a patch that is part of the parallel e2fsck series that Shilong
>> is working on, and does not work by itself, but was requested during
>> discussion on the ext4 concall today.
> 
> Andreas, thanks for sending this patch.  (Also available at[1].)
> 
> [1] https://lore.kernel.org/linux-ext4/132401FE-6D25-41B3-99D1-50E7BC746237@dilger.ca/
> 
> I took look at it, and there are a number of issues with it.  First of
> all, there seems to be an assumption that (a) the number of threads is
> less than the number of block groups, and (b) the number of threads
> can evenly divide the number of block groups.  So for example, if the
> number of block groups is prime, or if you are trying to use say, 8 or
> 16 threads, and the number of block groups is odd, the code in
> question will not do the right thing.

Yes, the thread count is checked earlier in the parallel e2fsck patch
series to be <= number of block groups.  However, I wasn't aware of any
requirement for groups = N * threads.  It may be coincidental that we
have never tested that case.

In any case, the patch was never really intended to be used by itself,
only for review and discussion of the general approach.

> (a) meant that attempting to run the e2fsprogs regression test suite
> caused most of the test cases to fail with e2fsck crashing due to
> buffer overruns.  I fixed this by changing the number of threads to be
> 16, or if 16 was greater than the number of block groups, to be the
> number of block groups, just for debugging purposes.  However, there
> were still a few regression test failures.
> 
> I also then tried to use a file system that we had been using for
> testing fragmentation issues.  The file system was creating a 10GB
> virtual disk, and then running these commands:
> 
>   DEV=/dev/sdc
>   mke2fs -t ext4 $DEV 10G
>   mount $DEV /mnt
>   pushd /mnt
>   for t in $(seq 1 6144) ; do
>       for i in $(seq 1 25) ; do
>           fallocate tb$t-8mb-$i -l 8M
>       done
>       for i in $(seq 1 2) ; do
>           fallocate tb$t-400mb-$i -l 400M
>       done
>   done
>   popd
>   umount /mnt
> 
> With the patch applied, all of the threads failed with error code 22
> (EINVAL), except for one which failed with a bad block group checksum
> error.  I haven't had a chance to dig into further; but I was hoping
> that Shilong and/or Saranya might be able to take closer look at that.

There may very well be other issues with the patch that make it not
useful as-is in isolation.  I'd have to let Shilong comment on that.

> But the other thing that we might want to consider is to add
> demand-loading of the block (or inode) bitmap.  We got a complaint
> that "e2fsck -E journal_only" was super-slow whereas running the
> journal by mounting and unmounting the file system was much faster.
> The reason, of course, was because the kernel was only reading those
> bitmap blocks that are needed to be modified by the orphaned inode
> processing, whereas with e2fsprogs, we have to read in all of the
> bitmap blocks whether this is necessary or not.

Forking threads to do on-demand loading may have a high overhead, so
it would be interesting to investigate a libext2fs IO engine that is
using libaio.  That would allow O_DIRECT reading of filesystem metadata
without double caching, as well as avoid blocking threads.  Alternately,
there is already a "readahead" method exported that could be used to
avoid changing the code too much, using posix_fadvise(WILLNEED), but I
have no idea on how that would perform.

> So another idea that we've talked about is teaching libext2fs to be
> able to demand load the bitmap, and then when we write out the block
> bitmap, we only need to write out those blocks that were loaded.  This
> would also speed up running debugfs to examine the file system, as
> well as running fuse2fs.  Fortunately, we have abstractions in front
> of all of the bitmap accessor functions, and the code paths that would
> need to be changed to add demand-loading of bitmaps should be mostly
> exclusive of the changes needed for parallel bitmap loading.  So if
> Shilong has time to look at making the parallel bitmap loader more
> robust, perhaps Saranya could work on the demand-loading idea.
> 
> Or if Shilong doesn't have time to try to polish this parallel bitmap
> loading changes, we could have Saranya look at clean it up --- since
> regardless of whether we implement demand-loading or not, parallel
> bitmap reading is going to be useful for some use cases (e.g., a full
> fsck, dumpe2fs, or e2image).

I don't think Shilong will have time to work on major code changes for
the next few weeks at least, due to internal deadlines, after which we
can finish cleaning up and submitting the pfsck patch series upstream.
If you are interested in the whole 59-patch series, it is available via:

git pull https://review.whamcloud.com/tools/e2fsprogs refs/changes/14/39914/1

or viewable online via Gerrit at:

https://review.whamcloud.com/39914

Getting some high-level review/feedback of that patch series would avoid
spending time to rework/rebase it and finding it isn't in the form that
you would prefer, or if it needs major architectural changes.

Note that this is currently based on top of the Lustre e2fsprogs branch.
While these shouldn't cause any problems with non-Lustre filesystems,
there are other patches in the series that are not necessarily ready
for submission (e.g. dirdata, Lustre xattr decoding, inode badness, etc).

Cheers, Andreas

Hi,

On Thu, Sep 17, 2020 at 9:34 AM Andreas Dilger <adilger@dilger.ca> wrote:
>
> On Sep 16, 2020, at 3:03 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> >
> > On Fri, Sep 04, 2020 at 03:34:26PM -0600, Andreas Dilger wrote:
> >> This is a patch that is part of the parallel e2fsck series that Shilong
> >> is working on, and does not work by itself, but was requested during
> >> discussion on the ext4 concall today.
> >
> > Andreas, thanks for sending this patch.  (Also available at[1].)
> >
> > [1] https://lore.kernel.org/linux-ext4/132401FE-6D25-41B3-99D1-50E7BC746237@dilger.ca/
> >
> > I took look at it, and there are a number of issues with it.  First of
> > all, there seems to be an assumption that (a) the number of threads is
> > less than the number of block groups, and (b) the number of threads
> > can evenly divide the number of block groups.  So for example, if the
> > number of block groups is prime, or if you are trying to use say, 8 or
> > 16 threads, and the number of block groups is odd, the code in
> > question will not do the right thing.
>
> Yes, the thread count is checked earlier in the parallel e2fsck patch
> series to be <= number of block groups.  However, I wasn't aware of any
> requirement for groups = N * threads.  It may be coincidental that we
> have never tested that case.
>
> In any case, the patch was never really intended to be used by itself,
> only for review and discussion of the general approach.
>
> > (a) meant that attempting to run the e2fsprogs regression test suite
> > caused most of the test cases to fail with e2fsck crashing due to
> > buffer overruns.  I fixed this by changing the number of threads to be
> > 16, or if 16 was greater than the number of block groups, to be the
> > number of block groups, just for debugging purposes.  However, there
> > were still a few regression test failures.
> >
> > I also then tried to use a file system that we had been using for
> > testing fragmentation issues.  The file system was creating a 10GB
> > virtual disk, and then running these commands:
> >
> >   DEV=/dev/sdc
> >   mke2fs -t ext4 $DEV 10G
> >   mount $DEV /mnt
> >   pushd /mnt
> >   for t in $(seq 1 6144) ; do
> >       for i in $(seq 1 25) ; do
> >           fallocate tb$t-8mb-$i -l 8M
> >       done
> >       for i in $(seq 1 2) ; do
> >           fallocate tb$t-400mb-$i -l 400M
> >       done
> >   done
> >   popd
> >   umount /mnt
> >

I tested an attachment v2 patch(based on master branch) which used 32
threads locally and it passed the test.

[root@server e2fsprogs]# ./e2fsck/e2fsck -f /dev/sda4
e2fsck 1.46-WIP (20-Mar-2020)
Pass 1: Checking inodes, blocks, and sizes
Pass 2: Checking directory structure
Pass 3: Checking directory connectivity
Pass 4: Checking reference counts
Pass 5: Checking group summary information
Multiple threads triggered to read bitmaps
/dev/sda4: 77963/3145728 files (0.0% non-contiguous), 12559729/12563825 blocks


> > With the patch applied, all of the threads failed with error code 22
> > (EINVAL), except for one which failed with a bad block group checksum
> > error.  I haven't had a chance to dig into further; but I was hoping
> > that Shilong and/or Saranya might be able to take closer look at that.
>
> There may very well be other issues with the patch that make it not
> useful as-is in isolation.  I'd have to let Shilong comment on that.
>
> > But the other thing that we might want to consider is to add
> > demand-loading of the block (or inode) bitmap.  We got a complaint
> > that "e2fsck -E journal_only" was super-slow whereas running the
> > journal by mounting and unmounting the file system was much faster.
> > The reason, of course, was because the kernel was only reading those
> > bitmap blocks that are needed to be modified by the orphaned inode
> > processing, whereas with e2fsprogs, we have to read in all of the
> > bitmap blocks whether this is necessary or not.
>
> Forking threads to do on-demand loading may have a high overhead, so
> it would be interesting to investigate a libext2fs IO engine that is
> using libaio.  That would allow O_DIRECT reading of filesystem metadata
> without double caching, as well as avoid blocking threads.  Alternately,
> there is already a "readahead" method exported that could be used to
> avoid changing the code too much, using posix_fadvise(WILLNEED), but I
> have no idea on how that would perform.
>
> > So another idea that we've talked about is teaching libext2fs to be
> > able to demand load the bitmap, and then when we write out the block
> > bitmap, we only need to write out those blocks that were loaded.  This
> > would also speed up running debugfs to examine the file system, as
> > well as running fuse2fs.  Fortunately, we have abstractions in front
> > of all of the bitmap accessor functions, and the code paths that would
> > need to be changed to add demand-loading of bitmaps should be mostly
> > exclusive of the changes needed for parallel bitmap loading.  So if
> > Shilong has time to look at making the parallel bitmap loader more
> > robust, perhaps Saranya could work on the demand-loading idea.
> >
> > Or if Shilong doesn't have time to try to polish this parallel bitmap
> > loading changes, we could have Saranya look at clean it up --- since
> > regardless of whether we implement demand-loading or not, parallel
> > bitmap reading is going to be useful for some use cases (e.g., a full
> > fsck, dumpe2fs, or e2image).
>
> I don't think Shilong will have time to work on major code changes for
> the next few weeks at least, due to internal deadlines, after which we
> can finish cleaning up and submitting the pfsck patch series upstream.
> If you are interested in the whole 59-patch series, it is available via:
>
> git pull https://review.whamcloud.com/tools/e2fsprogs refs/changes/14/39914/1
>
> or viewable online via Gerrit at:
>
> https://review.whamcloud.com/39914
>
> Getting some high-level review/feedback of that patch series would avoid
> spending time to rework/rebase it and finding it isn't in the form that
> you would prefer, or if it needs major architectural changes.
>
> Note that this is currently based on top of the Lustre e2fsprogs branch.
> While these shouldn't cause any problems with non-Lustre filesystems,
> there are other patches in the series that are not necessarily ready
> for submission (e.g. dirdata, Lustre xattr decoding, inode badness, etc).
>
> Cheers, Andreas
>
>
>
>
>