[PATCH 1/4] ext4: fix forgetten xattr lock protection in ext4_expand_extra_isize

[PATCH 1/4] ext4: fix forgetten xattr lock protection in ext4_expand_extra_isize

[Miao Xie]

We should avoid the contention between the i_extra_isize update and
the inline data insertion, so move the xattr trylock in front of
i_extra_isize update.

Signed-off-by: Miao Xie <miaoxie@huawei.com>
Reviewed-by: Wang Shilong <wshilong@ddn.com>
---
 fs/ext4/inode.c | 18 ++++++++++++++++--
 fs/ext4/xattr.c | 10 ----------
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 5cf82d0..4af3edc 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5639,10 +5639,15 @@ static int ext4_expand_extra_isize(struct inode *inode,
 {
 	struct ext4_inode *raw_inode;
 	struct ext4_xattr_ibody_header *header;
+	int no_expand;
+	int error;
 
 	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
 		return 0;
 
+	if (ext4_write_trylock_xattr(inode, &no_expand) == 0)
+		return 0;
+
 	raw_inode = ext4_raw_inode(&iloc);
 
 	header = IHDR(inode, raw_inode);
@@ -5654,12 +5659,21 @@ static int ext4_expand_extra_isize(struct inode *inode,
 		       EXT4_I(inode)->i_extra_isize, 0,
 		       new_extra_isize - EXT4_I(inode)->i_extra_isize);
 		EXT4_I(inode)->i_extra_isize = new_extra_isize;
+		ext4_write_unlock_xattr(inode, &no_expand);
 		return 0;
 	}
 
 	/* try to expand with EAs present */
-	return ext4_expand_extra_isize_ea(inode, new_extra_isize,
-					  raw_inode, handle);
+	error = ext4_expand_extra_isize_ea(inode, new_extra_isize,
+					   raw_inode, handle);
+	if (error) {
+		/*
+		 * Inode size expansion failed; don't try again
+		 */
+		no_expand = 1;
+	}
+	ext4_write_unlock_xattr(inode, &no_expand);
+	return error;
 }
 
 /*
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 5d3c253..12ee5fb 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1472,10 +1472,6 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 	int error = 0, tried_min_extra_isize = 0;
 	int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
 	int isize_diff;	/* How much do we need to grow i_extra_isize */
-	int no_expand;

[PATCH 2/4] ext4: restructure ext4_expand_extra_isize

[Miao Xie]

Current ext4_expand_extra_isize just tries to expand extra isize, if
someone is holding xattr lock or some check fails, it will give up.
So rename its name to ext4_try_to_expand_extra_isize.

Besides that, we clean up unnecessary check and move some relative checks
into it.

Signed-off-by: Miao Xie <miaoxie@huawei.com>
Reviewed-by: Wang Shilong <wshilong@ddn.com>
---
 fs/ext4/inode.c | 67 ++++++++++++++++++++++++---------------------------------
 fs/ext4/xattr.c | 11 +++++++++-
 2 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4af3edc..01a9340 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5632,21 +5632,35 @@ ext4_reserve_inode_write(handle_t *handle, struct inode *inode,
  * Expand an inode by new_extra_isize bytes.
  * Returns 0 on success or negative error number on failure.
  */
-static int ext4_expand_extra_isize(struct inode *inode,
-				   unsigned int new_extra_isize,
-				   struct ext4_iloc iloc,
-				   handle_t *handle)
+static int ext4_try_to_expand_extra_isize(struct inode *inode,
+					  unsigned int new_extra_isize,
+					  struct ext4_iloc iloc,
+					  handle_t *handle)
 {
 	struct ext4_inode *raw_inode;
 	struct ext4_xattr_ibody_header *header;
 	int no_expand;
 	int error;
 
-	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
-		return 0;
+	if (ext4_test_inode_state(inode, EXT4_STATE_NO_EXPAND))
+		return -EOVERFLOW;
+
+	/*
+	 * In nojournal mode, we can immediately attempt to expand
+	 * the inode.  When journaled, we first need to obtain extra
+	 * buffer credits since we may write into the EA block
+	 * with this same handle. If journal_extend fails, then it will
+	 * only result in a minor loss of functionality for that inode.
+	 * If this is felt to be critical, then e2fsck should be run to
+	 * force a large enough s_min_extra_isize.
+	 */
+	if (ext4_handle_valid(handle) &&
+	    jbd2_journal_extend(handle,
+				EXT4_DATA_TRANS_BLOCKS(inode->i_sb)) != 0)
+		return -ENOSPC;
 
 	if (ext4_write_trylock_xattr(inode, &no_expand) == 0)
-		return 0;
+		return -EBUSY;
 
 	raw_inode = ext4_raw_inode(&iloc);
 
@@ -5673,6 +5687,7 @@ static int ext4_expand_extra_isize(struct inode *inode,
 		no_expand = 1;
 	}
 	ext4_write_unlock_xattr(inode, &no_expand);
+
 	return error;
 }
 
@@ -5693,44 +5708,18 @@ int ext4_mark_inode_dirty(handle_t *handle, struct inode *inode)
 {
 	struct ext4_iloc iloc;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
-	static unsigned int mnt_count;
-	int err, ret;
+	int err;
 
 	might_sleep();
 	trace_ext4_mark_inode_dirty(inode, _RET_IP_);
 	err = ext4_reserve_inode_write(handle, inode, &iloc);
 	if (err)
 		return err;
-	if (EXT4_I(inode)->i_extra_isize < sbi->s_want_extra_isize &&
-	    !ext4_test_inode_state(inode, EXT4_STATE_NO_EXPAND)) {
-		/*
-		 * In nojournal mode, we can immediately attempt to expand
-		 * the inode.  When journaled, we first need to obtain extra
-		 * buffer credits since we may write into the EA block
-		 * with this same handle. If journal_extend fails, then it will
-		 * only result in a minor loss of functionality for that inode.
-		 * If this is felt to be critical, then e2fsck should be run to
-		 * force a large enough s_min_extra_isize.
-		 */
-		if (!ext4_handle_valid(handle) ||
-		    jbd2_journal_extend(handle,
-			     EXT4_DATA_TRANS_BLOCKS(inode->i_sb)) == 0) {
-			ret = ext4_expand_extra_isize(inode,
-						      sbi->s_want_extra_isize,
-						      iloc, handle);
-			if (ret) {
-				if (mnt_count !=
-					le16_to_cpu(sbi->s_es->s_mnt_count)) {
-					ext4_warning(inode->i_sb,
-					"Unable to expand inode %lu. Delete"
-					" some EAs or run e2fsck.",
-					inode->i_ino);
-					mnt_count =
-					  le16_to_cpu(sbi->s_es->s_mnt_count);
-				}
-			}
-		}
-	}
+
+	if (EXT4_I(inode)->i_extra_isize < sbi->s_want_extra_isize)
+		ext4_try_to_expand_extra_isize(inode, sbi->s_want_extra_isize,
+					       iloc, handle);
+
 	return ext4_mark_iloc_dirty(handle, inode, &iloc);
 }
 
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 12ee5fb..3c6c225 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1465,12 +1465,14 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 {
 	struct ext4_xattr_ibody_header *header;
 	struct buffer_head *bh = NULL;
+	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	static unsigned int mnt_count;
 	size_t min_offs;
 	size_t ifree, bfree;
 	int total_ino;
 	void *base, *end;
 	int error = 0, tried_min_extra_isize = 0;
-	int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
+	int s_min_extra_isize = le16_to_cpu(sbi->s_es->s_min_extra_isize);
 	int isize_diff;	/* How much do we need to grow i_extra_isize */
 
 retry:
@@ -1558,6 +1560,13 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 
 cleanup:
 	brelse(bh);
+
+	if (mnt_count != le16_to_cpu(sbi->s_es->s_mnt_count)) {
+		ext4_warning(inode->i_sb, "Unable to expand inode %lu. Delete some EAs or run e2fsck.",
+			     inode->i_ino);
+		mnt_count = le16_to_cpu(sbi->s_es->s_mnt_count);
+	}
+
 	return error;
 }
 
-- 
2.5.0

Re: [PATCH 2/4] ext4: restructure ext4_expand_extra_isize

[Theodore Ts'o]

On Fri, Jul 07, 2017 at 01:09:51AM +0800, Miao Xie wrote:
> Current ext4_expand_extra_isize just tries to expand extra isize, if
> someone is holding xattr lock or some check fails, it will give up.
> So rename its name to ext4_try_to_expand_extra_isize.
> 
> Besides that, we clean up unnecessary check and move some relative checks
> into it.
> 
> Signed-off-by: Miao Xie <miaoxie@huawei.com>
> Reviewed-by: Wang Shilong <wshilong@ddn.com>

Thanks, applied.

					- Ted

[PATCH 3/4] ext4: cleanup ext4_expand_extra_isize_ea()

[Miao Xie]

Clean up some goto statement, make ext4_expand_extra_isize_ea() clearer.

Signed-off-by: Miao Xie <miaoxie@huawei.com>
Reviewed-by: Wang Shilong <wshilong@ddn.com>
---
 fs/ext4/xattr.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index 3c6c225..73fbe4a 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -1464,7 +1464,7 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 			       struct ext4_inode *raw_inode, handle_t *handle)
 {
 	struct ext4_xattr_ibody_header *header;
-	struct buffer_head *bh = NULL;
+	struct buffer_head *bh;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	static unsigned int mnt_count;
 	size_t min_offs;
@@ -1478,7 +1478,7 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 retry:
 	isize_diff = new_extra_isize - EXT4_I(inode)->i_extra_isize;
 	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
-		goto out;
+		return 0;
 
 	header = IHDR(inode, raw_inode);
 
@@ -1513,6 +1513,7 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 			EXT4_ERROR_INODE(inode, "bad block %llu",
 					 EXT4_I(inode)->i_file_acl);
 			error = -EFSCORRUPTED;
+			brelse(bh);
 			goto cleanup;
 		}
 		base = BHDR(bh);
@@ -1520,11 +1521,11 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 		min_offs = end - base;
 		bfree = ext4_xattr_free_space(BFIRST(bh), &min_offs, base,
 					      NULL);
+		brelse(bh);
 		if (bfree + ifree < isize_diff) {
 			if (!tried_min_extra_isize && s_min_extra_isize) {
 				tried_min_extra_isize++;
 				new_extra_isize = s_min_extra_isize;
-				brelse(bh);
 				goto retry;
 			}
 			error = -ENOSPC;
@@ -1542,7 +1543,6 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 		    s_min_extra_isize) {
 			tried_min_extra_isize++;
 			new_extra_isize = s_min_extra_isize;
-			brelse(bh);
 			goto retry;
 		}
 		goto cleanup;
@@ -1554,14 +1554,9 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
 			EXT4_GOOD_OLD_INODE_SIZE + new_extra_isize,
 			(void *)header, total_ino);
 	EXT4_I(inode)->i_extra_isize = new_extra_isize;
-	brelse(bh);
-out:
-	return 0;
 
 cleanup:
-	brelse(bh);

Re: [PATCH 3/4] ext4: cleanup ext4_expand_extra_isize_ea()

[Theodore Ts'o]

On Fri, Jul 07, 2017 at 01:09:52AM +0800, Miao Xie wrote:
> Clean up some goto statement, make ext4_expand_extra_isize_ea() clearer.
> 
> Signed-off-by: Miao Xie <miaoxie@huawei.com>
> Reviewed-by: Wang Shilong <wshilong@ddn.com>

Thanks, applied.

				- Ted

[PATCH 4/4] ext4, project: expand inode extra size if possible

[Miao Xie]

when upgrading from old format, try to set project id
to old file first time, it will return EOVERFLOW, but if
that file is dirtied(touch etc), changing project id will
be allowed, this might be confusing for users, we could
try to expand @i_extra_iszie here too.

Reported-by: Zhang Yi <yi.zhang@huawei.com>
Signed-off-by: Miao Xie <miaoxie@huawei.com>
Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
 fs/ext4/ext4_jbd2.h |  3 ++
 fs/ext4/inode.c     | 97 +++++++++++++++++++++++++++++++++++++++++------------
 fs/ext4/ioctl.c     |  9 +++--
 3 files changed, 85 insertions(+), 24 deletions(-)

diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
index f976111..3149fdd 100644
--- a/fs/ext4/ext4_jbd2.h
+++ b/fs/ext4/ext4_jbd2.h
@@ -234,6 +234,9 @@ int ext4_reserve_inode_write(handle_t *handle, struct inode *inode,
 
 int ext4_mark_inode_dirty(handle_t *handle, struct inode *inode);
 
+int ext4_expand_extra_isize(struct inode *inode,
+			    unsigned int new_extra_isize,
+			    struct ext4_iloc *iloc);
 /*
  * Wrapper functions with which ext4 calls into JBD.
  */
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 01a9340..41a353f 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -5628,6 +5628,42 @@ ext4_reserve_inode_write(handle_t *handle, struct inode *inode,
 	return err;
 }
 
+static int __ext4_expand_extra_isize(struct inode *inode,
+				     unsigned int new_extra_isize,
+				     struct ext4_iloc *iloc,
+				     handle_t *handle, int *no_expand)
+{
+	struct ext4_inode *raw_inode;
+	struct ext4_xattr_ibody_header *header;
+	int error;
+
+	raw_inode = ext4_raw_inode(iloc);
+
+	header = IHDR(inode, raw_inode);
+
+	/* No extended attributes present */
+	if (!ext4_test_inode_state(inode, EXT4_STATE_XATTR) ||
+	    header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)) {
+		memset((void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE +
+		       EXT4_I(inode)->i_extra_isize, 0,
+		       new_extra_isize - EXT4_I(inode)->i_extra_isize);
+		EXT4_I(inode)->i_extra_isize = new_extra_isize;
+		return 0;
+	}
+
+	/* try to expand with EAs present */
+	error = ext4_expand_extra_isize_ea(inode, new_extra_isize,
+					   raw_inode, handle);
+	if (error) {
+		/*
+		 * Inode size expansion failed; don't try again
+		 */
+		*no_expand = 1;
+	}
+
+	return error;
+}
+
 /*
  * Expand an inode by new_extra_isize bytes.
  * Returns 0 on success or negative error number on failure.
@@ -5637,8 +5673,6 @@ static int ext4_try_to_expand_extra_isize(struct inode *inode,
 					  struct ext4_iloc iloc,
 					  handle_t *handle)
 {
-	struct ext4_inode *raw_inode;
-	struct ext4_xattr_ibody_header *header;
 	int no_expand;
 	int error;
 
@@ -5662,32 +5696,53 @@ static int ext4_try_to_expand_extra_isize(struct inode *inode,
 	if (ext4_write_trylock_xattr(inode, &no_expand) == 0)
 		return -EBUSY;
 
-	raw_inode = ext4_raw_inode(&iloc);
+	error = __ext4_expand_extra_isize(inode, new_extra_isize, &iloc,
+					  handle, &no_expand);
+	ext4_write_unlock_xattr(inode, &no_expand);
 
-	header = IHDR(inode, raw_inode);
+	return error;
+}
 
-	/* No extended attributes present */
-	if (!ext4_test_inode_state(inode, EXT4_STATE_XATTR) ||
-	    header->h_magic != cpu_to_le32(EXT4_XATTR_MAGIC)) {
-		memset((void *)raw_inode + EXT4_GOOD_OLD_INODE_SIZE +
-		       EXT4_I(inode)->i_extra_isize, 0,
-		       new_extra_isize - EXT4_I(inode)->i_extra_isize);
-		EXT4_I(inode)->i_extra_isize = new_extra_isize;
-		ext4_write_unlock_xattr(inode, &no_expand);
-		return 0;
+int ext4_expand_extra_isize(struct inode *inode,
+			    unsigned int new_extra_isize,
+			    struct ext4_iloc *iloc)
+{
+	handle_t *handle;
+	int no_expand;
+	int error, rc;
+
+	if (ext4_test_inode_state(inode, EXT4_STATE_NO_EXPAND)) {
+		brelse(iloc->bh);
+		return -EOVERFLOW;
 	}
 
-	/* try to expand with EAs present */
-	error = ext4_expand_extra_isize_ea(inode, new_extra_isize,
-					   raw_inode, handle);
+	handle = ext4_journal_start(inode, EXT4_HT_INODE,
+				    EXT4_DATA_TRANS_BLOCKS(inode->i_sb));
+	if (IS_ERR(handle)) {
+		error = PTR_ERR(handle);
+		brelse(iloc->bh);
+		return error;
+	}
+
+	ext4_write_lock_xattr(inode, &no_expand);
+
+	BUFFER_TRACE(iloc.bh, "get_write_access");
+	error = ext4_journal_get_write_access(handle, iloc->bh);
 	if (error) {
-		/*
-		 * Inode size expansion failed; don't try again
-		 */
-		no_expand = 1;
+		brelse(iloc->bh);
+		goto out_stop;
 	}
-	ext4_write_unlock_xattr(inode, &no_expand);
 
+	error = __ext4_expand_extra_isize(inode, new_extra_isize, iloc,
+					  handle, &no_expand);
+
+	rc = ext4_mark_iloc_dirty(handle, inode, iloc);
+	if (!error)
+		error = rc;
+
+	ext4_write_unlock_xattr(inode, &no_expand);
+out_stop:
+	ext4_journal_stop(handle);
 	return error;
 }
 
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 0c21e22..0120207 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -351,11 +351,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
 
 	raw_inode = ext4_raw_inode(&iloc);
 	if (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid)) {
-		err = -EOVERFLOW;
+		err = ext4_expand_extra_isize(inode,
+					      EXT4_SB(sb)->s_want_extra_isize,
+					      &iloc);
+		if (err)
+			goto out_unlock;
+	} else {
 		brelse(iloc.bh);
-		goto out_unlock;
 	}
-	brelse(iloc.bh);
 
 	dquot_initialize(inode);
 
-- 
2.5.0

Re: [PATCH 4/4] ext4, project: expand inode extra size if possible

[Theodore Ts'o]

On Fri, Jul 07, 2017 at 01:09:53AM +0800, Miao Xie wrote:
> when upgrading from old format, try to set project id
> to old file first time, it will return EOVERFLOW, but if
> that file is dirtied(touch etc), changing project id will
> be allowed, this might be confusing for users, we could
> try to expand @i_extra_iszie here too.
> 
> Reported-by: Zhang Yi <yi.zhang@huawei.com>
> Signed-off-by: Miao Xie <miaoxie@huawei.com>
> Signed-off-by: Wang Shilong <wshilong@ddn.com>

Thanks, applied.

				- Ted

Re: [PATCH 1/4] ext4: fix forgetten xattr lock protection in ext4_expand_extra_isize

[Miao Xie]

Ping......

on 2017/7/7 at 1:09, Miao Xie wrote:
> We should avoid the contention between the i_extra_isize update and
> the inline data insertion, so move the xattr trylock in front of
> i_extra_isize update.
> 
> Signed-off-by: Miao Xie <miaoxie@huawei.com>
> Reviewed-by: Wang Shilong <wshilong@ddn.com>
> ---
>  fs/ext4/inode.c | 18 ++++++++++++++++--
>  fs/ext4/xattr.c | 10 ----------
>  2 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 5cf82d0..4af3edc 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5639,10 +5639,15 @@ static int ext4_expand_extra_isize(struct inode *inode,
>  {
>  	struct ext4_inode *raw_inode;
>  	struct ext4_xattr_ibody_header *header;
> +	int no_expand;
> +	int error;
>  
>  	if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
>  		return 0;
>  
> +	if (ext4_write_trylock_xattr(inode, &no_expand) == 0)
> +		return 0;
> +
>  	raw_inode = ext4_raw_inode(&iloc);
>  
>  	header = IHDR(inode, raw_inode);
> @@ -5654,12 +5659,21 @@ static int ext4_expand_extra_isize(struct inode *inode,
>  		       EXT4_I(inode)->i_extra_isize, 0,
>  		       new_extra_isize - EXT4_I(inode)->i_extra_isize);
>  		EXT4_I(inode)->i_extra_isize = new_extra_isize;
> +		ext4_write_unlock_xattr(inode, &no_expand);
>  		return 0;
>  	}
>  
>  	/* try to expand with EAs present */
> -	return ext4_expand_extra_isize_ea(inode, new_extra_isize,
> -					  raw_inode, handle);
> +	error = ext4_expand_extra_isize_ea(inode, new_extra_isize,
> +					   raw_inode, handle);
> +	if (error) {
> +		/*
> +		 * Inode size expansion failed; don't try again
> +		 */
> +		no_expand = 1;
> +	}
> +	ext4_write_unlock_xattr(inode, &no_expand);
> +	return error;
>  }
>  
>  /*
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 5d3c253..12ee5fb 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1472,10 +1472,6 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
>  	int error = 0, tried_min_extra_isize = 0;
>  	int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
>  	int isize_diff;	/* How much do we need to grow i_extra_isize */
> -	int no_expand;
> -
> -	if (ext4_write_trylock_xattr(inode, &no_expand) == 0)
> -		return 0;
>  
>  retry:
>  	isize_diff = new_extra_isize - EXT4_I(inode)->i_extra_isize;
> @@ -1558,16 +1554,10 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
>  	EXT4_I(inode)->i_extra_isize = new_extra_isize;
>  	brelse(bh);
>  out:
> -	ext4_write_unlock_xattr(inode, &no_expand);
>  	return 0;
>  
>  cleanup:
>  	brelse(bh);
> -	/*
> -	 * Inode size expansion failed; don't try again
> -	 */
> -	no_expand = 1;
> -	ext4_write_unlock_xattr(inode, &no_expand);
>  	return error;
>  }
>  
> 

RE: [PATCH 1/4] ext4: fix forgetten xattr lock protection in ext4_expand_extra_isize

[Wang Shilong]

  I guess Maintainer will reply you when development cycle is ready for this.
Please be patient Miao!, let's play Glory of the king.^^^_^^^

________________________________________
From: Miao Xie [miaoxie@huawei.com]
Sent: Thursday, July 13, 2017 20:55
To: linux-ext4@vger.kernel.org
Cc: tytso@mit.edu; Li Xi; yi.zhang@huawei.com; adilger@dilger.ca; Wang Shilong; Shuichi Ihara
Subject: Re: [PATCH 1/4] ext4: fix forgetten xattr lock protection in ext4_expand_extra_isize

Ping......

on 2017/7/7 at 1:09, Miao Xie wrote:
> We should avoid the contention between the i_extra_isize update and
> the inline data insertion, so move the xattr trylock in front of
> i_extra_isize update.
>
> Signed-off-by: Miao Xie <miaoxie@huawei.com>
> Reviewed-by: Wang Shilong <wshilong@ddn.com>
> ---
>  fs/ext4/inode.c | 18 ++++++++++++++++--
>  fs/ext4/xattr.c | 10 ----------
>  2 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 5cf82d0..4af3edc 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5639,10 +5639,15 @@ static int ext4_expand_extra_isize(struct inode *inode,
>  {
>       struct ext4_inode *raw_inode;
>       struct ext4_xattr_ibody_header *header;
> +     int no_expand;
> +     int error;
>
>       if (EXT4_I(inode)->i_extra_isize >= new_extra_isize)
>               return 0;
>
> +     if (ext4_write_trylock_xattr(inode, &no_expand) == 0)
> +             return 0;
> +
>       raw_inode = ext4_raw_inode(&iloc);
>
>       header = IHDR(inode, raw_inode);
> @@ -5654,12 +5659,21 @@ static int ext4_expand_extra_isize(struct inode *inode,
>                      EXT4_I(inode)->i_extra_isize, 0,
>                      new_extra_isize - EXT4_I(inode)->i_extra_isize);
>               EXT4_I(inode)->i_extra_isize = new_extra_isize;
> +             ext4_write_unlock_xattr(inode, &no_expand);
>               return 0;
>       }
>
>       /* try to expand with EAs present */
> -     return ext4_expand_extra_isize_ea(inode, new_extra_isize,
> -                                       raw_inode, handle);
> +     error = ext4_expand_extra_isize_ea(inode, new_extra_isize,
> +                                        raw_inode, handle);
> +     if (error) {
> +             /*
> +              * Inode size expansion failed; don't try again
> +              */
> +             no_expand = 1;
> +     }
> +     ext4_write_unlock_xattr(inode, &no_expand);
> +     return error;
>  }
>
>  /*
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 5d3c253..12ee5fb 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1472,10 +1472,6 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
>       int error = 0, tried_min_extra_isize = 0;
>       int s_min_extra_isize = le16_to_cpu(EXT4_SB(inode->i_sb)->s_es->s_min_extra_isize);
>       int isize_diff; /* How much do we need to grow i_extra_isize */
> -     int no_expand;
> -
> -     if (ext4_write_trylock_xattr(inode, &no_expand) == 0)
> -             return 0;
>
>  retry:
>       isize_diff = new_extra_isize - EXT4_I(inode)->i_extra_isize;
> @@ -1558,16 +1554,10 @@ int ext4_expand_extra_isize_ea(struct inode *inode, int new_extra_isize,
>       EXT4_I(inode)->i_extra_isize = new_extra_isize;
>       brelse(bh);
>  out:
> -     ext4_write_unlock_xattr(inode, &no_expand);
>       return 0;
>
>  cleanup:
>       brelse(bh);
> -     /*
> -      * Inode size expansion failed; don't try again
> -      */
> -     no_expand = 1;
> -     ext4_write_unlock_xattr(inode, &no_expand);
>       return error;
>  }
>
>

Re: [PATCH 1/4] ext4: fix forgetten xattr lock protection in ext4_expand_extra_isize

[Theodore Ts'o]

On Thu, Jul 13, 2017 at 01:04:08PM +0000, Wang Shilong wrote:
>   I guess Maintainer will reply you when development cycle is ready for this.
> Please be patient Miao!, let's play Glory of the king.^^^_^^^

Sorry, this come in too late for it be included in the merge window,
since at that time I Was trying to do final regression testing before
sending a pull request to Linus.

I'm also currently on travel (attending Usenix ATC and visiting $WORK
in the Bay Area).  I will start accumulating fixes soon, probably
shortly after 4.13-rc1 comes out.  It *would* be helpful if other ext4
developers could review the patches while on travel (hint, hint).
Otherwise, I'll start reviewing patches in a week or two.

Cheers,

					- Ted

Re: [PATCH 1/4] ext4: fix forgetten xattr lock protection in ext4_expand_extra_isize

[Theodore Ts'o]

On Fri, Jul 07, 2017 at 01:09:50AM +0800, Miao Xie wrote:
> We should avoid the contention between the i_extra_isize update and
> the inline data insertion, so move the xattr trylock in front of
> i_extra_isize update.
> 
> Signed-off-by: Miao Xie <miaoxie@huawei.com>
> Reviewed-by: Wang Shilong <wshilong@ddn.com>

Thanks, applied.

					- Ted