Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <130596B4-5BA6-4377-B5CE-0D59FB79878F@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_FF22BD80-4F5F-4BF3-B7A4-F1027C38FA83";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH v4 4/8] e2fsck: add fast commit scan pass
Date:   Mon, 18 Sep 2023 14:39:32 -0600
In-Reply-To: <20210122054504.1498532-5-user@harshads-520.kir.corp.google.com>
Cc:     Ext4 Developers List <linux-ext4@vger.kernel.org>, tytso@mit.edu
To:     Harshad Shirwadkar <harshadshirwadkar@gmail.com>
References: <20210122054504.1498532-1-user@harshads-520.kir.corp.google.com>
 <20210122054504.1498532-5-user@harshads-520.kir.corp.google.com>
Precedence: bulk


--Apple-Mail=_FF22BD80-4F5F-4BF3-B7A4-F1027C38FA83
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Jan 21, 2021, at 10:45 PM, Harshad Shirwadkar =
<harshadshirwadkar@gmail.com> wrote:
>=20
> From: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
>=20
> Add fast commit scan pass. Scan pass is responsible for following
> things:
>=20
> * Count total number of fast commit tags that need to be replayed
>  during the replay phase.
>=20
> * Validate whether the fast commit area is valid for a given
>  transaction ID.
>=20
> * Verify the CRC of fast commit area.
>=20
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com>

I was making a fix to debugfs/journal.c today to improve performance
of the revoke hashtable, since it was performing very badly with a
large journal and lots of revokes (separate patch to be submitted).

I noticed that e2fsck/journal.c is totally missing any of the fast
commit handling that was added to in this patch.

This seems dangerous since there are some cases where debugfs and =
tune2fs
will do journal recovery in userspace, but it appears possible that they
would totally miss any fast commit transaction handling.

It isn't great that we have two nearly identical copies of the same code
in e2fsprogs, but looks is difficult to make them totally identical.
We could potentially play some tricks here (e.g. use a common variable
name for both "ctx" and "fs" in the code, unify copies of =
e2fsck_journal_*
and ext2fs_journal_* into a single file, and potentially abstract out
some of the differences (mainly from e2fsck/journal.c fixing errors
during journal recovery) into helper functions that are no-ops on the
debugfs/journal.c side.

There would still be two different *builds* of the code, with a lot of
macro expansions to hide the differences, but I think it looks possible
to at least bring these two copies more into sync.  I have some =
cleanups,
but I don't know much about fast commits and what should be done there.

Cheers, Andreas

> ---
> e2fsck/journal.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 109 insertions(+)
>=20
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index 2c8e3441..f1aa0fd6 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -278,6 +278,108 @@ static int process_journal_block(ext2_filsys fs,
> 	return 0;
> }
>=20
> +static int ext4_fc_replay_scan(journal_t *j, struct buffer_head *bh,
> +				int off, tid_t expected_tid)
> +{
> +	e2fsck_t ctx =3D j->j_fs_dev->k_ctx;
> +	struct e2fsck_fc_replay_state *state;
> +	int ret =3D JBD2_FC_REPLAY_CONTINUE;
> +	struct ext4_fc_add_range *ext;
> +	struct ext4_fc_tl *tl;
> +	struct ext4_fc_tail *tail;
> +	__u8 *start, *end;
> +	struct ext4_fc_head *head;
> +	struct ext2fs_extent ext2fs_ex;
> +
> +	state =3D &ctx->fc_replay_state;
> +
> +	start =3D (__u8 *)bh->b_data;
> +	end =3D (__u8 *)bh->b_data + j->j_blocksize - 1;
> +
> +	jbd_debug(1, "Scan phase starting, expected %d", expected_tid);
> +	if (state->fc_replay_expected_off =3D=3D 0) {
> +		memset(state, 0, sizeof(*state));
> +		/* Check if we can stop early */
> +		if (le16_to_cpu(((struct ext4_fc_tl *)start)->fc_tag)
> +			!=3D EXT4_FC_TAG_HEAD) {
> +			jbd_debug(1, "Ending early!, not a head tag");
> +			return 0;
> +		}
> +	}
> +
> +	if (off !=3D state->fc_replay_expected_off) {
> +		ret =3D -EFSCORRUPTED;
> +		goto out_err;
> +	}
> +
> +	state->fc_replay_expected_off++;
> +	fc_for_each_tl(start, end, tl) {
> +		jbd_debug(3, "Scan phase, tag:%s, blk %lld\n",
> +			  tag2str(le16_to_cpu(tl->fc_tag)), =
bh->b_blocknr);
> +		switch (le16_to_cpu(tl->fc_tag)) {
> +		case EXT4_FC_TAG_ADD_RANGE:
> +			ext =3D (struct ext4_fc_add_range =
*)ext4_fc_tag_val(tl);
> +			ret =3D ext2fs_decode_extent(&ext2fs_ex, (void =
*)&ext->fc_ex,
> +						   sizeof(ext->fc_ex));
> +			if (ret)
> +				ret =3D JBD2_FC_REPLAY_STOP;
> +			else
> +				ret =3D JBD2_FC_REPLAY_CONTINUE;
> +		case EXT4_FC_TAG_DEL_RANGE:
> +		case EXT4_FC_TAG_LINK:
> +		case EXT4_FC_TAG_UNLINK:
> +		case EXT4_FC_TAG_CREAT:
> +		case EXT4_FC_TAG_INODE:
> +		case EXT4_FC_TAG_PAD:
> +			state->fc_cur_tag++;
> +			state->fc_crc =3D jbd2_chksum(j, state->fc_crc, =
tl,
> +					sizeof(*tl) + =
ext4_fc_tag_len(tl));
> +			break;
> +		case EXT4_FC_TAG_TAIL:
> +			state->fc_cur_tag++;
> +			tail =3D (struct ext4_fc_tail =
*)ext4_fc_tag_val(tl);
> +			state->fc_crc =3D jbd2_chksum(j, state->fc_crc, =
tl,
> +						sizeof(*tl) +
> +						offsetof(struct =
ext4_fc_tail,
> +						fc_crc));
> +			jbd_debug(1, "tail tid %d, expected %d\n",
> +					le32_to_cpu(tail->fc_tid),
> +					expected_tid);
> +			if (le32_to_cpu(tail->fc_tid) =3D=3D =
expected_tid &&
> +				le32_to_cpu(tail->fc_crc) =3D=3D =
state->fc_crc) {
> +				state->fc_replay_num_tags =3D =
state->fc_cur_tag;
> +			} else {
> +				ret =3D state->fc_replay_num_tags ?
> +					JBD2_FC_REPLAY_STOP : =
-EFSBADCRC;
> +			}
> +			state->fc_crc =3D 0;
> +			break;
> +		case EXT4_FC_TAG_HEAD:
> +			head =3D (struct ext4_fc_head =
*)ext4_fc_tag_val(tl);
> +			if (le32_to_cpu(head->fc_features) &
> +				~EXT4_FC_SUPPORTED_FEATURES) {
> +				ret =3D -EOPNOTSUPP;
> +				break;
> +			}
> +			if (le32_to_cpu(head->fc_tid) !=3D expected_tid) =
{
> +				ret =3D -EINVAL;
> +				break;
> +			}
> +			state->fc_cur_tag++;
> +			state->fc_crc =3D jbd2_chksum(j, state->fc_crc, =
tl,
> +					sizeof(*tl) + =
ext4_fc_tag_len(tl));
> +			break;
> +		default:
> +			ret =3D state->fc_replay_num_tags ?
> +				JBD2_FC_REPLAY_STOP : -ECANCELED;
> +		}
> +		if (ret < 0 || ret =3D=3D JBD2_FC_REPLAY_STOP)
> +			break;
> +	}
> +
> +out_err:
> +	return ret;
> +}
> /*
>  * Main recovery path entry point. This function returns =
JBD2_FC_REPLAY_CONTINUE
>  * to indicate that it is expecting more fast commit blocks. It =
returns
> @@ -286,6 +388,13 @@ static int process_journal_block(ext2_filsys fs,
> static int ext4_fc_replay(journal_t *journal, struct buffer_head *bh,
> 				enum passtype pass, int off, tid_t =
expected_tid)
> {
> +	e2fsck_t ctx =3D journal->j_fs_dev->k_ctx;
> +	struct e2fsck_fc_replay_state *state =3D &ctx->fc_replay_state;
> +
> +	if (pass =3D=3D PASS_SCAN) {
> +		state->fc_current_pass =3D PASS_SCAN;
> +		return ext4_fc_replay_scan(journal, bh, off, =
expected_tid);
> +	}
> 	return JBD2_FC_REPLAY_STOP;
> }
>=20
> --
> 2.30.0.280.ga3ce27912f-goog
>=20


Cheers, Andreas


--Apple-Mail=_FF22BD80-4F5F-4BF3-B7A4-F1027C38FA83
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+AFAmUItYUACgkQcqXauRfM
H+BKfw/8DvQoufdN0aJXZsdWMDVzUilPSPz7bqxkLerRebmfBTFeYUV0PMp3mhrY
D/3KvZ8GBVmcoj6p2dUqtAyaR6iEU14oXGSBEwrcG5kJUk6FsonsX4JMN5uQExMv
+ngasmokM8TQAgyPA/jLSUaxziCpsN1kQsIqjWpa68g3+w+KlmOE61xVsJLqPL+n
IRWbB4lM5tnVDZZU/bITZi19G/3lj54Xo4Y+0E7LG3ExM1C3Djl4e0RPD71AUVPq
ScqS54qEyKK0Qi+kY6HIy5KDfd+XY5jSLwQQva98sB6t4i4QV11a9rKWVPTByxyC
wCQ6XN2u0y/7X5hfi6sO/S4LcrjhgRWsn7v83Q2PllRg6PH4IAm73Vg1Cik5HTE1
8iEZX2bNfudPTrqu7Q0tdY8LZ9NYMf6ZLsLkJ2QvdmFWpU/dnOt6fIyABRq47Mb5
3u3L9FCYvAoEVTFO/J+2R+1uTqXoXkJd9BkH+EQQ7qzllz8b2cZ6oKkjpYL5CDVF
7rZwWGyBMd8tlhnGIUyPBuGAIze6rAsaNwrfQVpxf6/3rRi5DWA0hDOajVULK2Jp
bmZyCH2a6Y/cIMa+8hRvau7WMoe+wSwAcJpnYiDyhgiiK8VHFI8aKZnBgzOoct2l
e0RP1NZOsXnZ8kmSkhq1GzGW94JB26eFLHRH0vdAPE5GZwQEmIg=
=JZH4
-----END PGP SIGNATURE-----

--Apple-Mail=_FF22BD80-4F5F-4BF3-B7A4-F1027C38FA83--