thr3ads.net - Ocfs2 devel - [Ocfs2-devel] [PATCH v2] ocfs2: the ip_alloc_sem should be taken in ocfs2_get

If this information is useful, please help other people find it:
Share via:

alex chen

2017-Oct-20 09:03 UTC

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

The ip_alloc_sem should be taken in ocfs2_get_block() when reading file
in DIRECT mode to prevent concurrent access to extent tree with
ocfs2_dio_end_io_write(), which may cause BUGON in
ocfs2_get_clusters_nocache()->BUG_ON(v_cluster <
le32_to_cpu(rec->e_cpos))

Signed-off-by: Alex Chen <alex.chen at huawei.com>
Reviewed-by: Jun Piao <piaojun at huawei.com>

---
 fs/ocfs2/aops.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 88a31e9..5cb939f 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -134,6 +134,19 @@ static int ocfs2_symlink_get_block(struct inode *inode,
sector_t iblock,
 	return err;
 }

+static int ocfs2_get_block_lock(struct inode *inode, sector_t iblock,
+		    struct buffer_head *bh_result, int create)
+{
+	int ret;
+	struct ocfs2_inode_info *oi = OCFS2_I(inode);
+
+	down_read(&oi->ip_alloc_sem);
+	ret = ocfs2_get_block(inode, iblock, bh_result, create);
+	up_read(&oi->ip_alloc_sem);
+
+	return ret;
+}
+
 int ocfs2_get_block(struct inode *inode, sector_t iblock,
 		    struct buffer_head *bh_result, int create)
 {
@@ -2154,12 +2167,8 @@ static int ocfs2_dio_get_block(struct inode *inode,
sector_t iblock,
 	 * while file size will be changed.
 	 */
 	if (pos + total_len <= i_size_read(inode)) {
-		down_read(&oi->ip_alloc_sem);
 		/* This is the fast path for re-write. */
-		ret = ocfs2_get_block(inode, iblock, bh_result, create);
-
-		up_read(&oi->ip_alloc_sem);
-
+		ret = ocfs2_get_block_lock(inode, iblock, bh_result, create);
 		if (buffer_mapped(bh_result) &&
 		    !buffer_new(bh_result) &&
 		    ret == 0)
@@ -2424,7 +2433,7 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct
iov_iter *iter)
 		return 0;

 	if (iov_iter_rw(iter) == READ)
-		get_block = ocfs2_get_block;
+		get_block = ocfs2_get_block_lock;
 	else
 		get_block = ocfs2_dio_get_block;

-- 1.9.5.msysgit.1

Changwei Ge

2017-Oct-20 09:23 UTC

head link

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

Hi Alex,

Are you able to provide a way to reproduce this issue?
I'm very interested in it.

Thanks,
Changwei.

On 2017/10/20 17:08, alex chen wrote:> The ip_alloc_sem should be taken in ocfs2_get_block() when reading file
> in DIRECT mode to prevent concurrent access to extent tree with
> ocfs2_dio_end_io_write(), which may cause BUGON in
> ocfs2_get_clusters_nocache()->BUG_ON(v_cluster <
le32_to_cpu(rec->e_cpos))
> 
> Signed-off-by: Alex Chen <alex.chen at huawei.com>
> Reviewed-by: Jun Piao <piaojun at huawei.com>
> 
> ---
>   fs/ocfs2/aops.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 88a31e9..5cb939f 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -134,6 +134,19 @@ static int ocfs2_symlink_get_block(struct inode
*inode, sector_t iblock,
>   	return err;
>   }
> 
> +static int ocfs2_get_block_lock(struct inode *inode, sector_t iblock,
> +		    struct buffer_head *bh_result, int create)
> +{
> +	int ret;
> +	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> +
> +	down_read(&oi->ip_alloc_sem);
> +	ret = ocfs2_get_block(inode, iblock, bh_result, create);
> +	up_read(&oi->ip_alloc_sem);
> +
> +	return ret;
> +}
> +
>   int ocfs2_get_block(struct inode *inode, sector_t iblock,
>   		    struct buffer_head *bh_result, int create)
>   {
> @@ -2154,12 +2167,8 @@ static int ocfs2_dio_get_block(struct inode *inode,
sector_t iblock,
>   	 * while file size will be changed.
>   	 */
>   	if (pos + total_len <= i_size_read(inode)) {
> -		down_read(&oi->ip_alloc_sem);
>   		/* This is the fast path for re-write. */
> -		ret = ocfs2_get_block(inode, iblock, bh_result, create);
> -
> -		up_read(&oi->ip_alloc_sem);
> -
> +		ret = ocfs2_get_block_lock(inode, iblock, bh_result, create);
>   		if (buffer_mapped(bh_result) &&
>   		    !buffer_new(bh_result) &&
>   		    ret == 0)
> @@ -2424,7 +2433,7 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb,
struct iov_iter *iter)
>   		return 0;
> 
>   	if (iov_iter_rw(iter) == READ)
> -		get_block = ocfs2_get_block;
> +		get_block = ocfs2_get_block_lock;
>   	else
>   		get_block = ocfs2_dio_get_block;
> 
> -- 1.9.5.msysgit.1
> 
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>

Changwei Ge

2017-Oct-21 03:27 UTC

head link

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

Hi,
Actually we encountered the same issue months ago and we also ever 
posted a patch to fix it, however, we never got a 'reviewed-by':-(

Your patch has a better change-log and the logic is fine to me.
I think it is deserved to be merged.

But it seems that your editor has '4 white spaces' occupied tab, it 
violates the exited code style. Can you adjust it to '8 white spaces' 
and re-send this patch?

Otherwise I thinks this is a good fix.

Moreover, this issue was introduced by commit c15471f79506 ("ocfs2: fix 
sparse file & data ordering issue in direct io")

Adding a *Fixs* tag to your change-log is encouraged.

Also I CC to ocfs2 maintainers so that they can provide some helpful 
comments.

Thanks,
Changwei.


On 2017/10/20 17:08, alex chen wrote:> The ip_alloc_sem should be taken in ocfs2_get_block() when reading file
> in DIRECT mode to prevent concurrent access to extent tree with
> ocfs2_dio_end_io_write(), which may cause BUGON in
> ocfs2_get_clusters_nocache()->BUG_ON(v_cluster <
le32_to_cpu(rec->e_cpos))
> 
> Signed-off-by: Alex Chen <alex.chen at huawei.com>
> Reviewed-by: Jun Piao <piaojun at huawei.com>
Acked-by: Changwei Ge <ge.changwei at h3c.com>
> 
> ---
>   fs/ocfs2/aops.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 88a31e9..5cb939f 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -134,6 +134,19 @@ static int ocfs2_symlink_get_block(struct inode
*inode, sector_t iblock,
>   	return err;
>   }
> 
> +static int ocfs2_get_block_lock(struct inode *inode, sector_t iblock,
> +		    struct buffer_head *bh_result, int create)
> +{
> +	int ret;
> +	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> +
> +	down_read(&oi->ip_alloc_sem);
> +	ret = ocfs2_get_block(inode, iblock, bh_result, create);
> +	up_read(&oi->ip_alloc_sem);
> +
> +	return ret;
> +}
> +
>   int ocfs2_get_block(struct inode *inode, sector_t iblock,
>   		    struct buffer_head *bh_result, int create)
>   {
> @@ -2154,12 +2167,8 @@ static int ocfs2_dio_get_block(struct inode *inode,
sector_t iblock,
>   	 * while file size will be changed.
>   	 */
>   	if (pos + total_len <= i_size_read(inode)) {
> -		down_read(&oi->ip_alloc_sem);
>   		/* This is the fast path for re-write. */
> -		ret = ocfs2_get_block(inode, iblock, bh_result, create);
> -
> -		up_read(&oi->ip_alloc_sem);
> -
> +		ret = ocfs2_get_block_lock(inode, iblock, bh_result, create);
>   		if (buffer_mapped(bh_result) &&
>   		    !buffer_new(bh_result) &&
>   		    ret == 0)
> @@ -2424,7 +2433,7 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb,
struct iov_iter *iter)
>   		return 0;
> 
>   	if (iov_iter_rw(iter) == READ)
> -		get_block = ocfs2_get_block;
> +		get_block = ocfs2_get_block_lock;
>   	else
>   		get_block = ocfs2_dio_get_block;
> 
> -- 1.9.5.msysgit.1
> 
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>

Joseph Qi

2017-Oct-23 03:31 UTC

head link

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

On 17/10/20 17:03, alex chen wrote:> The ip_alloc_sem should be taken in ocfs2_get_block() when reading file
> in DIRECT mode to prevent concurrent access to extent tree with
> ocfs2_dio_end_io_write(), which may cause BUGON in
> ocfs2_get_clusters_nocache()->BUG_ON(v_cluster <
le32_to_cpu(rec->e_cpos))
> 
> Signed-off-by: Alex Chen <alex.chen at huawei.com>
> Reviewed-by: Jun Piao <piaojun at huawei.com>
> 
> ---
>  fs/ocfs2/aops.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 88a31e9..5cb939f 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -134,6 +134,19 @@ static int ocfs2_symlink_get_block(struct inode
*inode, sector_t iblock,
>  	return err;
>  }
> 
> +static int ocfs2_get_block_lock(struct inode *inode, sector_t iblock,
> +		    struct buffer_head *bh_result, int create)
> +{
> +	int ret;
> +	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> +
> +	down_read(&oi->ip_alloc_sem);
> +	ret = ocfs2_get_block(inode, iblock, bh_result, create);
> +	up_read(&oi->ip_alloc_sem);
> +
> +	return ret;
> +}
> +
>  int ocfs2_get_block(struct inode *inode, sector_t iblock,
>  		    struct buffer_head *bh_result, int create)
>  {
> @@ -2154,12 +2167,8 @@ static int ocfs2_dio_get_block(struct inode *inode,
sector_t iblock,
>  	 * while file size will be changed.
>  	 */
>  	if (pos + total_len <= i_size_read(inode)) {
> -		down_read(&oi->ip_alloc_sem);
>  		/* This is the fast path for re-write. */
> -		ret = ocfs2_get_block(inode, iblock, bh_result, create);
> -
> -		up_read(&oi->ip_alloc_sem);
> -
> +		ret = ocfs2_get_block_lock(inode, iblock, bh_result, create);
>  		if (buffer_mapped(bh_result) &&
>  		    !buffer_new(bh_result) &&
>  		    ret == 0)
> @@ -2424,7 +2433,7 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb,
struct iov_iter *iter)
>  		return 0;
> 
>  	if (iov_iter_rw(iter) == READ)
> -		get_block = ocfs2_get_block;
> +		get_block = ocfs2_get_block_lock;ocfs2_lock_get_block may be better.

Thanks,
Joseph
>  	else
>  		get_block = ocfs2_dio_get_block;
> 
> -- 1.9.5.msysgit.1
> 
>

Eric Ren

2017-Oct-23 04:06 UTC

head link

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

Hi,

On 10/20/2017 05:03 PM, alex chen wrote:> The ip_alloc_sem should be taken in ocfs2_get_block() when reading file
> in DIRECT mode to prevent concurrent access to extent tree with
> ocfs2_dio_end_io_write(), which may cause BUGON in
> ocfs2_get_clusters_nocache()->BUG_ON(v_cluster <
le32_to_cpu(rec->e_cpos))
This maybe seem a obvious fix, but it would be great if you can
write a more detailed commit log, like paste the crash backtrace
here so that people can pick this fix easily when they see the same issue.

Thanks,
Eric>
> Signed-off-by: Alex Chen <alex.chen at huawei.com>
> Reviewed-by: Jun Piao <piaojun at huawei.com>
>
> ---
>   fs/ocfs2/aops.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 88a31e9..5cb939f 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -134,6 +134,19 @@ static int ocfs2_symlink_get_block(struct inode
*inode, sector_t iblock,
>   	return err;
>   }
>
> +static int ocfs2_get_block_lock(struct inode *inode, sector_t iblock,
> +		    struct buffer_head *bh_result, int create)
> +{
> +	int ret;
> +	struct ocfs2_inode_info *oi = OCFS2_I(inode);
> +
> +	down_read(&oi->ip_alloc_sem);
> +	ret = ocfs2_get_block(inode, iblock, bh_result, create);
> +	up_read(&oi->ip_alloc_sem);
> +
> +	return ret;
> +}
> +
>   int ocfs2_get_block(struct inode *inode, sector_t iblock,
>   		    struct buffer_head *bh_result, int create)
>   {
> @@ -2154,12 +2167,8 @@ static int ocfs2_dio_get_block(struct inode *inode,
sector_t iblock,
>   	 * while file size will be changed.
>   	 */
>   	if (pos + total_len <= i_size_read(inode)) {
> -		down_read(&oi->ip_alloc_sem);
>   		/* This is the fast path for re-write. */
> -		ret = ocfs2_get_block(inode, iblock, bh_result, create);
> -
> -		up_read(&oi->ip_alloc_sem);
> -
> +		ret = ocfs2_get_block_lock(inode, iblock, bh_result, create);
>   		if (buffer_mapped(bh_result) &&
>   		    !buffer_new(bh_result) &&
>   		    ret == 0)
> @@ -2424,7 +2433,7 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb,
struct iov_iter *iter)
>   		return 0;
>
>   	if (iov_iter_rw(iter) == READ)
> -		get_block = ocfs2_get_block;
> +		get_block = ocfs2_get_block_lock;
>   	else
>   		get_block = ocfs2_dio_get_block;
>
> -- 1.9.5.msysgit.1
>
>
>

alex chen

2017-Oct-23 05:40 UTC

head link

[Ocfs2-devel] [PATCH v2] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

The ip_alloc_sem should be taken in ocfs2_get_block() when reading file
in DIRECT mode to prevent concurrent access to extent tree with
ocfs2_dio_end_io_write(), which may cause BUGON in
ocfs2_get_clusters_nocache()->BUG_ON(v_cluster <
le32_to_cpu(rec->e_cpos))

Fixes: c15471f79506 ("ocfs2: fix sparse file & data ordering issue in
direct io")

Signed-off-by: Alex Chen <alex.chen at huawei.com>
Reviewed-by: Jun Piao <piaojun at huawei.com>
Acked-by: Changwei Ge <ge.changwei at h3c.com>

---
 fs/ocfs2/aops.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 88a31e9..29607d9 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -134,6 +134,19 @@ static int ocfs2_symlink_get_block(struct inode *inode,
sector_t iblock,
 	return err;
 }

+static int ocfs2_get_block_lock(struct inode *inode, sector_t iblock,
+		    struct buffer_head *bh_result, int create)
+{
+	int ret = 0;
+	struct ocfs2_inode_info *oi = OCFS2_I(inode);
+
+	down_read(&oi->ip_alloc_sem);
+	ret = ocfs2_get_block(inode, iblock, bh_result, create);
+	up_read(&oi->ip_alloc_sem);
+
+	return ret;
+}
+
 int ocfs2_get_block(struct inode *inode, sector_t iblock,
 		    struct buffer_head *bh_result, int create)
 {
@@ -2154,12 +2167,9 @@ static int ocfs2_dio_get_block(struct inode *inode,
sector_t iblock,
 	 * while file size will be changed.
 	 */
 	if (pos + total_len <= i_size_read(inode)) {
-		down_read(&oi->ip_alloc_sem);
-		/* This is the fast path for re-write. */
-		ret = ocfs2_get_block(inode, iblock, bh_result, create);
-
-		up_read(&oi->ip_alloc_sem);

+		/* This is the fast path for re-write. */
+		ret = ocfs2_get_block_lock(inode, iblock, bh_result, create);
 		if (buffer_mapped(bh_result) &&
 		    !buffer_new(bh_result) &&
 		    ret == 0)
@@ -2424,7 +2434,7 @@ static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct
iov_iter *iter)
 		return 0;

 	if (iov_iter_rw(iter) == READ)
-		get_block = ocfs2_get_block;
+		get_block = ocfs2_get_block_lock;
 	else
 		get_block = ocfs2_dio_get_block;

-- 
1.9.5.msysgit.1

Ocfs2 devel - Oct 2017 - [PATCH v2] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

[Ocfs2-devel] [PATCH] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

[Ocfs2-devel] [PATCH v2] ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()