Re: [PATCH] jbd2: invalidate handle if jbd2_journal_restart() fails

[Younger Liu <younger.liu@huawei.com>]

On 2013/6/24 1:44, Theodore Ts'o wrote:
> If jbd2_journal_restart() fails the handle will have been disconnected
> from the current transaction.  In this situation, the handle must not
> be used for for any jbd2 function other than jbd2_journal_stop().
> Enforce this with by treating a handle which has a NULL transaction
> pointer as an aborted handle, and issue a kernel warning if
> jbd2_journal_extent(), jbd2_journal_get_write_access(),
> jbd2_journal_dirty_metadata(), etc. is called with an invalid handle.
> 
> This commit also fixes a bug where jbd2_journal_stop() would trip over
> a kernel jbd2 assertion check when trying to free an invalid handle.
> 
> Also move the responsibility of setting current->journal_info to
> start_this_handle(), simplifying the three users of this function.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Reported-by: Younger Liu <younger.liu@huawei.com>
> Cc: Jan Kara <jack@suse.cz>
Hi Ted,
There is a mistake in this patch.
Please see my comments below.
> ---
>  fs/jbd2/transaction.c | 27 ++++++++++++++++-----------
>  include/linux/jbd2.h  |  2 +-
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
> index 7391456..8ac8306 100644
> --- a/fs/jbd2/transaction.c
> +++ b/fs/jbd2/transaction.c
...
> @@ -1523,18 +1525,19 @@ int jbd2_journal_stop(handle_t *handle)
>  {
>  	transaction_t *transaction = handle->h_transaction;
>  	journal_t *journal = transaction->t_journal;
> -	int err, wait_for_commit = 0;
> +	int err = 0, wait_for_commit = 0;
>  	tid_t tid;
>  	pid_t pid;
>  
> +	if (!handle->h_transaction)
> +		goto free_and_exit;
> +
>  	J_ASSERT(journal_current_handle() == handle);
>  
If jbd2__journal_restart fails, handle->h_transaction may be NULL.
So we should check handle->h_transaction before 
"journal = transaction->t_journal", Right?
How about the following?

    transaction_t *transaction = handle->h_transaction;
    journal_t *journal = NULL;
    int err = 0, wait_for_commit = 0;
    tid_t tid;
    pid_t pid;

    if (!handle->h_transaction)
        goto free_and_exit;

    journal = transaction->t_journal;
    J_ASSERT(journal_current_handle() == handle);

We should check this in other functions too,such as 
jbd2_journal_extend(), jbd2_journal_get_create_access(),
jbd2_journal_dirty_metadata(),jbd2_journal_file_inode(),
jbd2__journal_restart(), etc. 

>  	if (is_handle_aborted(handle))
>  		err = -EIO;
> -	else {
> +	else
>  		J_ASSERT(atomic_read(&transaction->t_updates) > 0);
> -		err = 0;
> -	}
>  
>  	if (--handle->h_ref > 0) {
>  		jbd_debug(4, "h_ref %d -> %d\n", handle->h_ref + 1,
> @@ -1657,6 +1660,7 @@ int jbd2_journal_stop(handle_t *handle)
>  
>  	if (handle->h_rsv_handle)
>  		jbd2_journal_free_reserved(handle->h_rsv_handle);
> +free_and_exit:
>  	jbd2_free_handle(handle);
>  	return err;
>  }
> @@ -2364,6 +2368,7 @@ int jbd2_journal_file_inode(handle_t *handle, struct jbd2_inode *jinode)
>  	transaction_t *transaction = handle->h_transaction;
>  	journal_t *journal = transaction->t_journal;
>  
> +	WARN_ON(!handle->h_transaction);
>  	if (is_handle_aborted(handle))
>  		return -EIO;
>  
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 5f3c094..1891112 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -1295,7 +1295,7 @@ static inline int is_journal_aborted(journal_t *journal)
>  
>  static inline int is_handle_aborted(handle_t *handle)
>  {
> -	if (handle->h_aborted)
> +	if (handle->h_aborted || !handle->h_transaction)
>  		return 1;
>  	return is_journal_aborted(handle->h_transaction->t_journal);
>  }
>