Skip to contentSkip to conversation

ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

PATCH New alex chen 5 participants 10 messages Oct 20, 2017 – Oct 24, 2017 OCFS2

lore
Share & tools
lore

alex chen

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
 fs/ocfs2/aops.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +15 -6
index 88a31e9..5cb939f 100644
@@ -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

#1 lore raw

PATCH 1/1 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.

Quoted 68 lines · alex chen

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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
  fs/ocfs2/aops.c | 21 +++++++++++++++------
  1 file changed, 15 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +15 -7
index 88a31e9..5cb939f 100644
@@ -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

author
#2 lore raw

PATCH 1/1 ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

Hi Changwei,

Quoted 5 lines · Changwei Ge

Hi Alex,

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

You can reproduce the BUG in the following steps:
1. touch "/mnt/ocfs2/test";
2. fallocate -l 1G "/mnt/ocfs2/test";
3. write the file "/mnt/ocfs2/test" using the io_submit();
4. at the same time, creating another process to read this file from a random location;

Quoted 76 lines

Thanks,
Changwei.

Quoted 63 lines · alex chen

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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
  fs/ocfs2/aops.c | 21 +++++++++++++++------
  1 file changed, 15 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +15 -7
index 88a31e9..5cb939f 100644
@@ -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
Quoted 5 lines

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

.

#3 lore raw

PATCH 1/1 ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

In reply to alex chen

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.

Quoted 7 lines · alex chen

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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
Acked-by Changwei Ge <[email protected]>
Quoted 61 lines
---
  fs/ocfs2/aops.c | 21 +++++++++++++++------
  1 file changed, 15 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +15 -7
index 88a31e9..5cb939f 100644
@@ -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

author
#4 lore raw

PATCH 1/1 ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

Hi Changewei,

Thank for your suggestion, I will modify this patch and resend it.

Thanks
Alex

Quoted 101 lines · Changwei Ge

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.

Quoted 7 lines · alex chen

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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
Acked-by Changwei Ge <[email protected]>
Quoted 56 lines
---
  fs/ocfs2/aops.c | 21 +++++++++++++++------
  1 file changed, 15 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +15 -7
index 88a31e9..5cb939f 100644
@@ -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
Quoted 5 lines

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

.

#5 lore raw

PATCH 1/1 ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

In reply to alex chen
Quoted 51 lines · alex chen

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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
 fs/ocfs2/aops.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +14 -5
index 88a31e9..5cb939f 100644
@@ -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)

fs/ocfs2/aops.c static ssize_t ocfs2_direct_IO(struct kiocb *iocb, struct iov_iter *iter) · line 2436

Quoted 5 lines
fs/ocfs2/aops.c +1 -1
@@ -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

Quoted 6 lines
fs/ocfs2/aops.c +0 -1
 	else
 		get_block = ocfs2_dio_get_block;
 
-- 1.9.5.msysgit.1
#6 lore raw

PATCH 1/1 ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

In reply to alex chen

Hi,

Quoted 4 lines · alex chen

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

Quoted 59 lines
Signed-off-by Alex Chen <[email protected]>
Reviewed-by Jun Piao <[email protected]>
---
  fs/ocfs2/aops.c | 21 +++++++++++++++------
  1 file changed, 15 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +15 -7
index 88a31e9..5cb939f 100644
@@ -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
author
#7 lore raw

PATCH v2 1/1 ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

In reply to alex chen

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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
Acked-by Changwei Ge <[email protected]>
---
 fs/ocfs2/aops.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +16 -6
index 88a31e9..29607d9 100644
@@ -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;
Signature · 1 line
1.9.5.msysgit.1
#8 lore raw

PATCH v2 1/1 ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

Hello Alex,

Base on your bug description, I think that the fix should be OK.
But I suggest you to use more clear function name in this patch.
1) For new-added function ocfs2_get_block_lock(), I think that it is better if the function name is ocfs2_dio_rd_get_block().
2) Then, rename the original function ocfs2_dio_get_block() to ocfs2_dio_wr_get_block().
Why?
Since this function looks to only be used by direct IO (read) mode,
we should make the function name more clear, do NOT make other developer to misunderstand/misuse it in the future.

Thanks
Gang

Quoted 76 lines
Quoted 1 line
Quoted 1 line

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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
Acked-by Changwei Ge <[email protected]>
---
 fs/ocfs2/aops.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +13 -0
index 88a31e9..29607d9 100644
@@ -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)

{

fs/ocfs2/aops.c +2 -5
@@ -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) &&
fs/ocfs2/aops.c +1 -1
 		    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;
Signature · 8 lines
-- 
1.9.5.msysgit.1


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com 
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
author
#9 lore raw

PATCH v2 1/1 ocfs2: the ip_alloc_sem should be taken in ocfs2_get_block()

Hi Everybody,

Thanks for your reviews.

I agree with Gang to make function name ocfs2_get_block_lock() more readable. But the ocfs2_get_block_lock() may be called by the ocfs2_dio_get_block() in re-write sitiation, so I don't think the ocfs2_dio_rd_get_block() is a better name. I agree with Joseph and rename it to ocfs2_lock_get_block().

BTW, I think the ocfs2_dio_wr_get_block() is more suitable for the ocfs2_dio_get_block().

Please give me any feedback if you have any suggestions.

Thanks
Alex

Quoted 94 lines · Gang He

Hello Alex,

Base on your bug description, I think that the fix should be OK.
But I suggest you to use more clear function name in this patch.
1) For new-added function ocfs2_get_block_lock(), I think that it is better if the function name is ocfs2_dio_rd_get_block().
2) Then, rename the original function ocfs2_dio_get_block() to ocfs2_dio_wr_get_block().
Why?
Since this function looks to only be used by direct IO (read) mode,
we should make the function name more clear, do NOT make other developer to misunderstand/misuse it in the future.

Thanks
Gang

Quoted 76 lines
Quoted 1 line
Quoted 1 line

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 <[email protected]>
Reviewed-by Jun Piao <[email protected]>
Acked-by Changwei Ge <[email protected]>
---
 fs/ocfs2/aops.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)
fs/ocfs2/aops.c +13 -0
index 88a31e9..29607d9 100644
@@ -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)

{

fs/ocfs2/aops.c +2 -5
@@ -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) &&
fs/ocfs2/aops.c +1 -1
 		    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;
Signature · 8 lines
-- 
1.9.5.msysgit.1


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com 
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

.