| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 28 May 2013 23:22:05 +0200
From: Jan Kara <jack@...e.cz>
To: Dmitry Monakhov <dmonakhov@...nvz.org>
Cc: linux-ext4@...r.kernel.org, jack@...e.cz
Subject: Re: [PATCH 1/6] jbd2: optimize jbd2_journal_force_commit
On Tue 28-05-13 13:18:56, Dmitry Monakhov wrote:
> Current implementation of jbd2_journal_force_commit() is suboptimal because
> result in empty and useless commits. But callers just want to force and wait
> any unfinished commits. We already has jbd2_journal_force_commit_nested()
> which does exactly what we want, except we are guaranteed that we do not hold
> journal transaction open.
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@...nvz.org>
Hum, didn't I already review something like this?
> ---
> fs/jbd2/journal.c | 55 ++++++++++++++++++++++++++++++++++++------------
> fs/jbd2/transaction.c | 23 --------------------
> include/linux/jbd2.h | 2 +-
> 3 files changed, 42 insertions(+), 38 deletions(-)
>
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 886ec2f..078e8ea 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -566,21 +566,19 @@ int jbd2_log_start_commit(journal_t *journal, tid_t tid)
> }
>
> /*
> - * Force and wait upon a commit if the calling process is not within
> - * transaction. This is used for forcing out undo-protected data which contains
> - * bitmaps, when the fs is running out of space.
> - *
> - * We can only force the running transaction if we don't have an active handle;
> - * otherwise, we will deadlock.
> - *
> - * Returns true if a transaction was started.
> + * Force and wait any uncommitted transactions. We can only force the running
> + * transaction if we don't have an active handle, otherwise, we will deadlock.
> */
> -int jbd2_journal_force_commit_nested(journal_t *journal)
> +int __jbd2_journal_force_commit(journal_t *journal, int nested, int *progress)
> {
> transaction_t *transaction = NULL;
> tid_t tid;
> - int need_to_start = 0;
> + int need_to_start = 0, ret = 0;
>
> + J_ASSERT(!current->journal_info || nested);
> +
> + if (progress)
> + *progress = 0;
> read_lock(&journal->j_state_lock);
> if (journal->j_running_transaction && !current->journal_info) {
> transaction = journal->j_running_transaction;
> @@ -590,16 +588,45 @@ int jbd2_journal_force_commit_nested(journal_t *journal)
> transaction = journal->j_committing_transaction;
>
> if (!transaction) {
> + /* Nothing to commit */
> read_unlock(&journal->j_state_lock);
> - return 0; /* Nothing to retry */
> + return 0;
> }
> -
> tid = transaction->t_tid;
> read_unlock(&journal->j_state_lock);
> if (need_to_start)
> jbd2_log_start_commit(journal, tid);
> - jbd2_log_wait_commit(journal, tid);
> - return 1;
> + ret = jbd2_log_wait_commit(journal, tid);
> + if (!ret && progress)
> + *progress = 1;
> +
> + return ret;
> +}
Can't we just make this function return <0, 0, 1 and get rid of
'progress' argument? Caller jbd2_journal_force_commit() can then return 0
if __jbd2_journal_force_commit() returned >= 0...
> +
> +/**
> + * Force and wait upon a commit if the calling process is not within
> + * transaction. This is used for forcing out undo-protected data which contains
> + * bitmaps, when the fs is running out of space.
> + *
> + * @journal: journal to force
> + * Returns true if progress was made.
> + */
> +int jbd2_journal_force_commit_nested(journal_t *journal)
> +{
> + int progress;
> +
> + __jbd2_journal_force_commit(journal, 1, &progress);
> + return progress;
> +}
> +
> +/**
> + * int journal_force_commit() - force any uncommitted transactions
> + * @journal: journal to force
> + *
> + */
> +int jbd2_journal_force_commit(journal_t *journal)
> +{
> + return __jbd2_journal_force_commit(journal, 0, NULL);
> }
Also if we move the WARN_ON(!current->journal_info) here, we can remove
the 'nested' argument.
Honza
> /*
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 10f524c..dae6b3d 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
> @@ -1522,29 +1522,6 @@ int jbd2_journal_stop(handle_t *handle)
> return err;
> }
>
> -/**
> - * int jbd2_journal_force_commit() - force any uncommitted transactions
> - * @journal: journal to force
> - *
> - * For synchronous operations: force any uncommitted transactions
> - * to disk. May seem kludgy, but it reuses all the handle batching
> - * code in a very simple manner.
> - */
> -int jbd2_journal_force_commit(journal_t *journal)
> -{
> - handle_t *handle;
> - int ret;
> -
> - handle = jbd2_journal_start(journal, 1);
> - if (IS_ERR(handle)) {
> - ret = PTR_ERR(handle);
> - } else {
> - handle->h_sync = 1;
> - ret = jbd2_journal_stop(handle);
> - }
> - return ret;
> -}
> -
> /*
> *
> * List management code snippets: various functions for manipulating the
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 6e051f4..c9e1ab6 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1125,6 +1125,7 @@ extern void jbd2_journal_ack_err (journal_t *);
> extern int jbd2_journal_clear_err (journal_t *);
> extern int jbd2_journal_bmap(journal_t *, unsigned long, unsigned long long *);
> extern int jbd2_journal_force_commit(journal_t *);
> +extern int jbd2_journal_force_commit_nested(journal_t *);
> extern int jbd2_journal_file_inode(handle_t *handle, struct jbd2_inode *inode);
> extern int jbd2_journal_begin_ordered_truncate(journal_t *journal,
> struct jbd2_inode *inode, loff_t new_size);
> @@ -1199,7 +1200,6 @@ int __jbd2_log_space_left(journal_t *); /* Called with journal locked */
> int jbd2_log_start_commit(journal_t *journal, tid_t tid);
> int __jbd2_log_start_commit(journal_t *journal, tid_t tid);
> int jbd2_journal_start_commit(journal_t *journal, tid_t *tid);
> -int jbd2_journal_force_commit_nested(journal_t *journal);
> int jbd2_log_wait_commit(journal_t *journal, tid_t tid);
> int jbd2_complete_transaction(journal_t *journal, tid_t tid);
> int jbd2_log_do_checkpoint(journal_t *journal);
> --
> 1.7.1
>
--
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists