From: Jan Kara Subject: Re: [PATCH] ext4: avoid lockdep warning when inheriting encryption context Date: Wed, 16 Nov 2016 11:10:18 +0100 Message-ID: <20161116101018.GC21785@quack2.suse.cz> References: <1479157416-144246-1-git-send-email-ebiggers@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-ext4@vger.kernel.org, Theodore Ts'o , Andreas Dilger , Jaegeuk Kim , Jan Kara To: Eric Biggers Return-path: Received: from mx2.suse.de ([195.135.220.15]:44438 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752651AbcKPKKW (ORCPT ); Wed, 16 Nov 2016 05:10:22 -0500 Content-Disposition: inline In-Reply-To: <1479157416-144246-1-git-send-email-ebiggers@google.com> Sender: linux-ext4-owner@vger.kernel.org List-ID: 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: > [] jbd2_log_wait_commit+0x5/0x11b > > but task is already holding lock: > (jbd2_handle > ){++++.+}, at: > [] start_this_handle+0x354/0x3d8 > > The abbreviated call stack is: > > [] ? jbd2_log_wait_commit+0x5/0x11b > [] jbd2_log_wait_commit+0x40/0x11b > [] ? jbd2_log_wait_commit+0x5/0x11b > [] ? __jbd2_journal_force_commit+0x76/0xa6 > [] __jbd2_journal_force_commit+0x91/0xa6 > [] jbd2_journal_force_commit_nested+0xe/0x18 > [] ext4_should_retry_alloc+0x72/0x79 > [] ext4_xattr_set+0xef/0x11f > [] ext4_set_context+0x3a/0x16b > [] fscrypt_inherit_context+0xe3/0x103 > [] __ext4_new_inode+0x12dc/0x153a > [] 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 Looks good. You can add: Reviewed-by: Jan Kara 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 SUSE Labs, CR