On Mon 14-11-16 13:03:36, Eric Biggers wrote:
> On a lockdep-enabled kernel, xfstests generic/027 fails due to a lockdep
> warning when run on ext4 mounted with -o test_dummy_encryption:
>
> xfs_io/4594 is trying to acquire lock:
> (jbd2_handle
> ){++++.+}, at:
> [<ffffffff813096ef>] jbd2_log_wait_commit+0x5/0x11b
>
> but task is already holding lock:
> (jbd2_handle
> ){++++.+}, at:
> [<ffffffff813000de>] start_this_handle+0x354/0x3d8
>
> The abbreviated call stack is:
>
> [<ffffffff813096ef>] ? jbd2_log_wait_commit+0x5/0x11b
> [<ffffffff8130972a>] jbd2_log_wait_commit+0x40/0x11b
> [<ffffffff813096ef>] ? jbd2_log_wait_commit+0x5/0x11b
> [<ffffffff8130987b>] ? __jbd2_journal_force_commit+0x76/0xa6
> [<ffffffff81309896>] __jbd2_journal_force_commit+0x91/0xa6
> [<ffffffff813098b9>] jbd2_journal_force_commit_nested+0xe/0x18
> [<ffffffff812a6049>] ext4_should_retry_alloc+0x72/0x79
> [<ffffffff812f0c1f>] ext4_xattr_set+0xef/0x11f
> [<ffffffff812cc35b>] ext4_set_context+0x3a/0x16b
> [<ffffffff81258123>] fscrypt_inherit_context+0xe3/0x103
> [<ffffffff812ab611>] __ext4_new_inode+0x12dc/0x153a
> [<ffffffff812bd371>] ext4_create+0xb7/0x161
>
> When a file is created in an encrypted directory, ext4_set_context() is
> called to set an encryption context on the new file. This calls
> ext4_xattr_set(), which contains a retry loop where the journal is
> forced to commit if an ENOSPC error is encountered.
>
> If the task actually were to wait for the journal to commit in this
> case, then it would deadlock because a handle remains open from
> __ext4_new_inode(), so the running transaction can't be committed yet.
> Fortunately, __jbd2_journal_force_commit() avoids the deadlock by not
> allowing the running transaction to be committed while the current task
> has it open. However, the above lockdep warning is still triggered.
>
> Fix the problem by passing the handle through the 'fs_data' argument to
> ext4_set_context(), then using ext4_xattr_set_handle() instead of
> ext4_xattr_set(). And in the case where no journal handle is specified
> and ext4_set_context() has to open one, add an ENOSPC retry loop since
> in that case it is the outermost transaction.
>
> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
Looks good. You can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> fs/ext4/ialloc.c | 3 +--
> fs/ext4/super.c | 32 ++++++++++++++++++++++----------
> 2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 170421e..f543281 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -1115,8 +1115,7 @@ struct inode *__ext4_new_inode(handle_t *handle, struct inode *dir,
> }
>
> if (encrypt) {
> - /* give pointer to avoid set_context with journal ops. */
> - err = fscrypt_inherit_context(dir, inode, &encrypt, true);
> + err = fscrypt_inherit_context(dir, inode, handle, true);
> if (err)
> goto fail_free_drop;
> }
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index ff6f3ab..22d50cb 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1114,14 +1114,22 @@ static int ext4_prepare_context(struct inode *inode)
> static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> void *fs_data)
> {
> - handle_t *handle;
> - int res, res2;
> + handle_t *handle = fs_data;
> + int res, res2, retries = 0;
>
> - /* fs_data is null when internally used. */
> - if (fs_data) {
> - res = ext4_xattr_set(inode, EXT4_XATTR_INDEX_ENCRYPTION,
> - EXT4_XATTR_NAME_ENCRYPTION_CONTEXT, ctx,
> - len, 0);
> + /*
> + * If a journal handle was specified, then the encryption context is
> + * being set on a new inode via inheritance and is part of a larger
> + * transaction to create the inode. Otherwise the encryption context is
> + * being set on an existing inode in its own transaction. Only in the
> + * latter case should the "retry on ENOSPC" logic be used.
> + */
> +
> + if (handle) {
> + res = ext4_xattr_set_handle(handle, inode,
> + EXT4_XATTR_INDEX_ENCRYPTION,
> + EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
> + ctx, len, 0);
> if (!res) {
> ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
> ext4_clear_inode_state(inode,
> @@ -1130,14 +1138,15 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> return res;
> }
>
> +retry:
> handle = ext4_journal_start(inode, EXT4_HT_MISC,
> ext4_jbd2_credits_xattr(inode));
> if (IS_ERR(handle))
> return PTR_ERR(handle);
>
> - res = ext4_xattr_set(inode, EXT4_XATTR_INDEX_ENCRYPTION,
> - EXT4_XATTR_NAME_ENCRYPTION_CONTEXT, ctx,
> - len, 0);
> + res = ext4_xattr_set_handle(handle, inode, EXT4_XATTR_INDEX_ENCRYPTION,
> + EXT4_XATTR_NAME_ENCRYPTION_CONTEXT,
> + ctx, len, 0);
> if (!res) {
> ext4_set_inode_flag(inode, EXT4_INODE_ENCRYPT);
> res = ext4_mark_inode_dirty(handle, inode);
> @@ -1145,6 +1154,9 @@ static int ext4_set_context(struct inode *inode, const void *ctx, size_t len,
> EXT4_ERROR_INODE(inode, "Failed to mark inode dirty");
> }
> res2 = ext4_journal_stop(handle);
> +
> + if (res == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
> + goto retry;
> if (!res)
> res = res2;
> return res;
> --
> 2.8.0.rc3.226.g39d4020
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
