linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Theodore Y. Ts'o" <tytso@mit.edu>
To: Harshad Shirwadkar <harshadshirwadkar@gmail.com>
Cc: linux-ext4@vger.kernel.org
Subject: Re: [PATCH v3 03/13] jbd2: fast-commit commit path changes
Date: Wed, 16 Oct 2019 12:38:44 -0400	[thread overview]
Message-ID: <20191016163844.GC11103@mit.edu> (raw)
In-Reply-To: <20191001074101.256523-4-harshadshirwadkar@gmail.com>

On Tue, Oct 01, 2019 at 12:40:52AM -0700, Harshad Shirwadkar wrote:
> This patch adds core fast-commit commit path changes. This patch also
> modifies existing JBD2 APIs to allow usage of fast commits. If fast
> commits are enabled and journal->j_do_full_commit is not set,

This flag should really be a property of the transaction, not the
journal.  Otherwise it might not be clear what transaction is meant in
jbd2_log_start_commit():

> @@ -522,11 +539,23 @@ int jbd2_log_start_commit(journal_t *journal, tid_t tid)
>  	int ret;
>  
>  	write_lock(&journal->j_state_lock);
> +	journal->j_do_full_commit = true;
>  	ret = __jbd2_log_start_commit(journal, tid);
>  	write_unlock(&journal->j_state_lock);
>  	return ret;
>  }

Does tid refer to the transaction which is just starting to be
committed?  Or the next transaction?

If we make the flag be attached to the transaction, then it's very
clear which transaction must be a full commit, and I think it will
simplify things.

> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 132fb92098c7..7db3e2b6336d 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> + * fc is input / output parameter. If fc is non-null and is set to true, this
> + * function tries to perform fast commit. If the fast commit is successfully
> + * performed, *fc is set to true.
>   */
> -void jbd2_journal_commit_transaction(journal_t *journal)
> +void jbd2_journal_commit_transaction(journal_t *journal, bool *fc)

I think it's going to make things much simpler if we pull out the code
which does fast commits, which was added to this function, into its
own function, jbd2_fast_commit_transaction().

Right now the logic regarding whether to do a fast commit or a full
commit is split between kjournald2() and
jbd2_journal_commit_transact() --- and once we implement asynchronous
fast commits, it doesn't make sense to even have some of this logic.

I think it will be easier if you modify the commits to add support for
asynchronous commits from the beginning.  In that world, we don't need
to have the fast commit logic inside jbd2_journal_commit_transaction(),
and that means we don't have to add the fc variable.

It also avoids a minor inconsistency in the current code, where in
order to have kjournald2() actually call
jbd2_journal_commit_transaction(), we have to bump the
j_commit_request indicating that we want to commit the current
transaction.  But then if we can do the fast commit, j_commit_request
is left indicating that there is an outstanding request that the
existing transaction be committed --- but we don't start committing
it.

That's going to be confusing for future debugging, and I could imagine
current or existing code thinking that there has already been a
request to start committing the current transaction, so it doesn't try
waking up the kjournald2 thread.

> @@ -160,7 +160,13 @@ static void commit_timeout(struct timer_list *t)
>   *
>   * 1) COMMIT:  Every so often we need to commit the current state of the
>   *    filesystem to disk.  The journal thread is responsible for writing
> - *    all of the metadata buffers to disk.
> + *    all of the metadata buffers to disk. If fast commits are allowed,
> + *    journal thread passes the control to the file system and file system
> + *    is then responsible for writing metadata buffers to disk (in whichever
> + *    format it wants). If fast commit succeds, journal thread won't perform
> + *    a normal commit. In case the fast commit fails, journal thread performs
> + *    full commit as normal.

Note: this commit needs to be updated once we are doing async fc
commits.

> @@ -702,12 +745,27 @@ int jbd2_log_wait_commit(journal_t *journal, tid_t tid)
>  	}
>  #endif
>  	while (tid_gt(tid, journal->j_commit_sequence)) {
> -		jbd_debug(1, "JBD2: want %u, j_commit_sequence=%u\n",
> -				  tid, journal->j_commit_sequence);
> +		if ((!journal->j_do_full_commit) &&
> +		    !tid_gt(subtid, journal->j_fc_sequence))
> +			break;
> +		jbd_debug(1, "JBD2: want full commit %u %s %u, ",
> +			  tid, journal->j_do_full_commit ?
> +			  "and ignoring fast commit request for " :
> +			  "or want fast commit",
> +			  journal->j_fc_sequence);
> +		jbd_debug(1, "j_commit_sequence=%u, j_fc_sequence=%u\n",
> +			  journal->j_commit_sequence,
> +			  journal->j_fc_sequence);
>  		read_unlock(&journal->j_state_lock);
>  		wake_up(&journal->j_wait_commit);
> -		wait_event(journal->j_wait_done_commit,
> -				!tid_gt(tid, journal->j_commit_sequence));
> +		if (journal->j_do_full_commit)
> +			wait_event(journal->j_wait_done_commit,
> +				   !tid_gt(tid, journal->j_commit_sequence));
> +		else
> +			wait_event(journal->j_wait_done_commit,
> +				   !tid_gt(tid, journal->j_commit_sequence) ||
> +				   !tid_gt(subtid,
> +					    journal->j_fc_sequence));
>  		read_lock(&journal->j_state_lock);
>  	}
>  	read_unlock(&journal->j_state_lock);

This change is also not needed with async fast commits, right?

          	       	    	       - Ted

  reply	other threads:[~2019-10-16 16:39 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-01  7:40 [PATCH v3 00/13] ext4: add fast commit support Harshad Shirwadkar
2019-10-01  7:40 ` [PATCH v3 01/13] ext4: add handling for extended mount options Harshad Shirwadkar
2019-10-16  2:14   ` Theodore Y. Ts'o
2019-10-21 20:41     ` harshad shirwadkar
2019-10-01  7:40 ` [PATCH v3 02/13] jbd2: fast commit setup and enable Harshad Shirwadkar
2019-10-16 13:03   ` Theodore Y. Ts'o
2019-10-01  7:40 ` [PATCH v3 03/13] jbd2: fast-commit commit path changes Harshad Shirwadkar
2019-10-16 16:38   ` Theodore Y. Ts'o [this message]
2019-10-01  7:40 ` [PATCH v3 04/13] jbd2: fast-commit commit path new APIs Harshad Shirwadkar
2019-10-16 17:20   ` Theodore Y. Ts'o
2019-10-01  7:40 ` [PATCH v3 05/13] jbd2: fast-commit recovery path changes Harshad Shirwadkar
2019-10-16 17:30   ` Theodore Y. Ts'o
2019-10-22  0:51     ` harshad shirwadkar
2019-10-01  7:40 ` [PATCH v3 06/13] ext4: add fields that are needed to track changed files Harshad Shirwadkar
2019-10-16 18:26   ` Theodore Y. Ts'o
2019-10-01  7:40 ` [PATCH v3 07/13] ext4: track changed files for fast commit Harshad Shirwadkar
2019-10-16 20:26   ` Theodore Y. Ts'o
2019-10-01  7:40 ` [PATCH v3 08/13] ext4: fast-commit commit range tracking Harshad Shirwadkar
2019-10-16 21:36   ` Theodore Y. Ts'o
2019-10-30  5:12     ` harshad shirwadkar
2019-10-01  7:40 ` [PATCH v3 09/13] ext4: fast-commit commit path changes Harshad Shirwadkar
2019-10-16 22:45   ` Theodore Y. Ts'o
     [not found]     ` <CAAJeciXQiE022GqcsTr35jSqjA6eH+zBS2KNvDPj5PovButdYA@mail.gmail.com>
2019-10-23 12:44       ` Theodore Y. Ts'o
2019-10-01  7:40 ` [PATCH v3 10/13] ext4: fast-commit recovery " Harshad Shirwadkar
2019-10-18  2:07   ` Theodore Y. Ts'o
2019-10-01  7:41 ` [PATCH v3 11/13] ext4: add support for asynchronous fast commits Harshad Shirwadkar
2019-10-25  6:28   ` Xiaoguang Wang
2019-10-01  7:41 ` [PATCH v3 12/13] docs: Add fast commit documentation Harshad Shirwadkar
2019-10-18  1:56   ` Theodore Y. Ts'o
2019-10-18  4:51     ` Andreas Dilger
2019-10-18 13:28       ` Theodore Y. Ts'o
2019-10-31 18:53         ` Andreas Dilger
2019-10-31  5:34     ` harshad shirwadkar
2019-10-31  6:41       ` harshad shirwadkar
2019-10-04 19:12 ` [PATCH v3 00/13] ext4: add fast commit support Theodore Y. Ts'o
2019-10-04 20:11   ` harshad shirwadkar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191016163844.GC11103@mit.edu \
    --to=tytso@mit.edu \
    --cc=harshadshirwadkar@gmail.com \
    --cc=linux-ext4@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).