Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <D3085B08-C2D9-43BE-9ACD-FDCCEE9CAB82@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_24FE7AED-C8F3-421E-A3A5-8319533E4982";
 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 15:31:44 -0600
In-Reply-To: <130596B4-5BA6-4377-B5CE-0D59FB79878F@dilger.ca>
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>
 <130596B4-5BA6-4377-B5CE-0D59FB79878F@dilger.ca>
Precedence: bulk


--Apple-Mail=_24FE7AED-C8F3-421E-A3A5-8319533E4982
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Sep 18, 2023, at 2:39 PM, Andreas Dilger <adilger@dilger.ca> wrote:
>=20
> 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>
>=20
> 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).
>=20
> I noticed that e2fsck/journal.c is totally missing any of the fast
> commit handling that was added to in this patch.

Sorry, this became incorrect during editing - the debugfs/journal.c
file does not have any of the fast commit handling that was added to
e2fsck/journal.

Cheers, Andreas

> 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.
>=20
> 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.
>=20
> 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.
>=20
> Cheers, Andreas
>=20
>> ---
>> 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
>=20
>=20
> Cheers, Andreas


Cheers, Andreas


--Apple-Mail=_24FE7AED-C8F3-421E-A3A5-8319533E4982
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+AFAmUIwcAACgkQcqXauRfM
H+BD6Q//ez94FGN/z3OkSRf7Cdu6sS6XtxRVCC8Y2sAnSeHBuWIfN2H2SqoWb8ba
u0Ob45MJX4dqJmMXRcGYYtX2BqNP9V8xz+uCEKDM6E2TKiQHtSRxe2NichDi2V3S
0dwOBOpvf7g8FmjCDOVtkt7okoCjYQS93QWnVsuHxq3pLhBB1An4rSA7fdvALv/o
f+E+Qp8ht81hinhi462QWjxs+jrSL/Yz1LXc4gFJkOPuRB0ZupcmqQp3+Vqi7Eu1
aSAgk++8kt8PX9XHvEFKxFWmNQgWDPJBQFz0NdNheoAneojrxpv8qfztpgO+ev07
YduBb8W+ChQuSzoBSCXf7QbrbhsuVcCa9UwMUhL570xyCUz+ndclAW0ctFlPdo/i
0TuLA/3zlztiz0fB7Qnq7lCVg3c2pF3eP4xyoW/52A/Wzbu/Jj/mDWhmC/vUeZsl
6N/UIkToEpDEV2dO33chzRvGvG3ZGKip8WR8311MUOGBI6FL3jWgF7rQfiu4KTw7
b6uXAsGE6wogWNyWiqMACKMGv2zIX6MpBYAxgUBAW5HVg5sjG9EVUBV0NzXBfyfp
dY72nQ2wY3peknT0xGxSyPb+s44bM25oqT5gNXGZLp/a+Xrx2dIqDu6TAmLUREE8
N538zGHRNX/rMkONL/yyoTYog3PLt+y4UHxDyjrOJpW7XeY2GFU=
=C3fk
-----END PGP SIGNATURE-----

--Apple-Mail=_24FE7AED-C8F3-421E-A3A5-8319533E4982--