Bug#841144: kernel BUG at /build/linux-Wgpe2M/linux-4.8.11/fs/ocfs2/alloc.c:1514!

The dw_zero_count tracking was assuming that w_unwritten_list would always contain one element. The actual count is now tracked whenever the list is extended.

---
 fs/ocfs2/aops.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/ocfs2/aops.c +2 -0

index 88a31e9340a0..eb0a81368dbb 100644
@@ -784,6 +784,8 @@ struct ocfs2_write_ctxt {
 	struct ocfs2_cached_dealloc_ctxt w_dealloc;
 
 	struct list_head		w_unwritten_list;
+
+	unsigned int			w_unwritten_count;
 };
 
 void ocfs2_unlock_and_free_pages(struct page **pages, int num_pages)

fs/ocfs2/aops.c +1 -1

 	new = NULL;
 unlock:
 	spin_unlock(&oi->ip_lock);

@@ -2246,7 +2250,7 @@ static int ocfs2_dio_get_block(struct inode *inode, sector_t iblock,
 		ue->ue_phys = desc->c_phys;
 
 		list_splice_tail_init(&wc->w_unwritten_list, &dwc->dw_zero_list);
-		dwc->dw_zero_count++;
+		dwc->dw_zero_count += wc->w_unwritten_count;
 	}
 
 	ret = ocfs2_write_end_nolock(inode->i_mapping, pos, len, len, wc);

Signature · 1 line

2.11.0

In January Ben Hutchings reported Debian bug 841144 to the ocfs2-devel list:

https://oss.oracle.com/pipermail/ocfs2-devel/2017-January/012701.html

cPanel encountered this bug after upgrading our cluster to the 4.9 Debian stable kernel. In our environment, the bug would trigger every few hours.

The core problem seems to be that the size of dw_zero_list is not tracked correctly. This causes the ocfs2_lock_allocators() call in ocfs2_dio_end_io_write() to underestimate the number of extents needed. As a result, meta_ac is null when it's needed in ocfs2_grow_tree().

The attached patch is a forward-ported version of the fix we applied to Debian's 4.9 kernel to correct the issue.

Review

Quoted 6 lines

(quoted diff) +0 -0

@@ -873,6 +875,7 @@ static int ocfs2_alloc_write_ctxt(struct
 ocfs2_write_ctxt **wcp,

?
? ocfs2_init_dealloc_ctxt(&wc->w_dealloc);
? INIT_LIST_HEAD(&wc->w_unwritten_list);

(quoted diff) +1 -0

+	wc->w_unwritten_count = 0;

I think you don't have to evaluate ::w_unwritten_count to zero since? kzalloc already did that.

Review static int ocfs2_unwritten_check(struct inode · line 1380

Quoted 10 lines

?
? *wcp = wc;
?

(quoted diff) +1 -0

@@ -1373,6 +1376,7 @@ static int ocfs2_unwritten_check(struct inode
 *inode,
 ?	desc->c_clear_unwritten = 0;
 ?	list_add_tail(&new->ue_ip_node, &oi->ip_unwritten_list);
 ?	list_add_tail(&new->ue_node, &wc->w_unwritten_list);
+	wc->w_unwritten_count++;

You increase ::w_unwritten_coun once a new _ue_ is attached to? ::w_unwritten_list. So if no _ue_ ever is attached, ::w_unwritten_list?
is still empty. I think your change has the same effect with origin.

Moreover I don't see the relation between the reported crash issue and?your patch change. Can you elaborate further?

Quoted 10 lines

(quoted diff) +0 -0

@@ -2246,7 +2250,7 @@ static int ocfs2_dio_get_block(struct inode
 *inode, sector_t iblock,
 ?		ue->ue_phys = desc->c_phys;

?
? list_splice_tail_init(&wc->w_unwritten_list, &dwc->dw_zero_list);

(quoted diff) +1 -1

-		dwc->dw_zero_count++;
+		dwc->dw_zero_count += wc->w_unwritten_count;

? }
?

Quoted 9 lines · Changwei Ge

Review static int ocfs2_alloc_write_ctxt(struct · line 879

Quoted 6 lines

(quoted diff) +1 -0

@@ -873,6 +875,7 @@ static int ocfs2_alloc_write_ctxt(struct
 ocfs2_write_ctxt **wcp,
 
 ?	ocfs2_init_dealloc_ctxt(&wc->w_dealloc);
 ?	INIT_LIST_HEAD(&wc->w_unwritten_list);
+	wc->w_unwritten_count = 0;

I think you don't have to evaluate ::w_unwritten_count to zero since kzalloc already did that.

Very true. I was following the example of how dwc was handling the dw_zero_count. You'll have to forgive me a bit. I'm very unfamiliar with the linux kernel codebase.

Quoted 16 lines

Review static int ocfs2_unwritten_check(struct inode · line 1380

Quoted 10 lines

(quoted diff) +1 -0

 ?	*wcp = wc;
 

@@ -1373,6 +1376,7 @@ static int ocfs2_unwritten_check(struct inode
 *inode,
 ?	desc->c_clear_unwritten = 0;
 ?	list_add_tail(&new->ue_ip_node, &oi->ip_unwritten_list);
 ?	list_add_tail(&new->ue_node, &wc->w_unwritten_list);
+	wc->w_unwritten_count++;

You increase ::w_unwritten_coun once a new _ue_ is attached to ::w_unwritten_list. So if no _ue_ ever is attached, ::w_unwritten_list
is still empty. I think your change has the same effect with origin.

Quoted 2 lines

Moreover I don't see the relation between the reported crash issue and?your patch change. Can you elaborate further?

The important part is in the next segment in the patch. This block is just using w_unwritten_count to track the size of w_unwritten_list.

Quoted 10 lines

Review static int ocfs2_dio_get_block(struct inode · line 2256

Quoted 10 lines

(quoted diff) +1 -1

@@ -2246,7 +2250,7 @@ static int ocfs2_dio_get_block(struct inode
 *inode, sector_t iblock,
 ?		ue->ue_phys = desc->c_phys;
 
 ?		list_splice_tail_init(&wc->w_unwritten_list, &dwc->dw_zero_list);
-		dwc->dw_zero_count++;
+		dwc->dw_zero_count += wc->w_unwritten_count;
 ?	}
 

dw_zero_count is tracking the number of elements in dw_zero_list.

The old version assumed that after dw_zero_list and w_unwritten_list were spliced together, that the new length was dw_zero_count + 1. This assumption is not correct if w_unwritten_list contained more than one element.

The length of dw_zero_list is used by ocfs2_dio_end_io_write() to determine whether or not meta_ac will be needed to complete the write:

????ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2, ????????????????????&data_ac, &meta_ac);

This will return with success and a null meta_ac if there are at least dw_zero_count * 2 extents available for the write.

Since dw_zero_count was not being calculated correctly, this will occasionally result in the write getting into ocfs2_grow_tree() with a null meta_ac following this chain:

ocfs2_dio_end_io_write()
ocfs2_mark_extent_written()
ocfs2_change_extent_flag()
ocfs2_split_extent()
ocfs2_split_and_insert()
ocfs2_grow_tree()

That's my understanding of what's causing the bug.

Our OCFS2 cluster was crashing every two to three hours after we upgraded to a 4.x kernel. We've gone about 18 hours with this patch applied and no crashes.

Can your tell me how did you format your volume?
What's your _cluster size_ and _block size_?
Your can obtain such information via debugfs.ocfs2 <your volume> -R? 'stats' | grep 'Cluster Size'

It's better for you provide a way to reproduce this issue so that we can?
perform some test.

Quoted 5 lines · Changwei Ge

Can your tell me how did you format your volume?
What's your _cluster size_ and _block size_?
Your can obtain such information via debugfs.ocfs2 <your volume> -R 'stats' | grep 'Cluster Size'

Quoted 4 lines

It's better for you provide a way to reproduce this issue so that we can
perform some test.

The issue recurred in our cluster today, so at best my patch is just decreasing the frequency of the crashes.

Our setup has 10 machines sharing two OCFS2 mountpoints over fibre channel.

Both OCFS2 partitions have block size bits set to 12 and cluster size bits set to 20.

The two partitions contain around 310 files total with 200 of those being qcow2 files. The only inodes getting any read and write activity are the qcow2 files.

The qcow2 files were created as sparse files (preallocation=metadata) and some are reflinked copies.

It's not clear to me exactly why it's passing through ocfs2_lock_allocators() without allocating meta_ac. These qcow files wouldn't be written concurrently by different systems in the OCFS2 cluster.

Is it possible the 2 x multiplier in the ocfs2_lock_allocators call is not large enough?

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

Hi John,

I think a better method to solve this problem.

Quoted 12 lines · John Lightsey

Quoted 9 lines · Changwei Ge

Can your tell me how did you format your volume?
What's your _cluster size_ and _block size_?
Your can obtain such information via debugfs.ocfs2 <your volume> -R 'stats' | grep 'Cluster Size'

It's better for you provide a way to reproduce this issue so that we can
perform some test.

Quoted 2 lines

The issue recurred in our cluster today, so at best my patch is just decreasing the frequency of the crashes.

We need to check the free number of the records in each loop to mark extent written, because the last extent block may be changed through many times marking extent written and the num_free_extents also be changed. In the worst case, the num_free_extents may become less than at the beginning of the loop. So we should not estimate the free number of the records at the beginning of the loop to mark extent written.

I'd appreciate it if you could test the following patch and feedback the result.

Signed-off-by Alex Chen <[email protected]>

---
  fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++---------
  1 file changed, 64 insertions(+), 12 deletions(-)

fs/ocfs2/aops.c +8 -0

index d151632..80725f2 100644
@@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock,
  	return ret;
  }
 
+static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et,
+		struct ocfs2_alloc_context *meta_ac, int max_rec_needed)
+{
+	int status = 0, free_extents;
+
+	free_extents = ocfs2_num_free_extents(et);
+	if (free_extents < 0) {
+		status = free_extents;

fs/ocfs2/aops.c +0 -0

  static int ocfs2_dio_end_io_write(struct inode *inode,
  				  struct ocfs2_dio_write_ctxt *dwc,
  				  loff_t offset,

@@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
  	struct ocfs2_extent_tree et;
  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
  	struct ocfs2_inode_info *oi = OCFS2_I(inode);

fs/ocfs2/aops.c +4 -3

  	ocfs2_init_dealloc_ctxt(&dealloc);
 

@@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 
  	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);
 
-	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
-				    &data_ac, &meta_ac);
+restart_all:
+	ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEED_SPLIT,
+				    NULL, &meta_ac);
  	if (ret) {
  		mlog_errno(ret);
  		goto unlock;

@@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
  	if (IS_ERR(handle)) {
  		ret = PTR_ERR(handle);
  		mlog_errno(ret);
-		goto unlock;
+		goto free_ac;
  	}
  	ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
  				      OCFS2_JOURNAL_ACCESS_WRITE);

fs/ocfs2/aops.c +26 -5

+		if (ret < 0) {
+			mlog_errno(ret);
+			break;
+		} else if (ret == 1) {
+			restart_func = 1;
+			break;
+		}
+
  		ret = ocfs2_mark_extent_written(inode, &et, handle,
  						ue->ue_cpos, 1,
  						ue->ue_phys,

@@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
  			mlog_errno(ret);
  			break;
  		}
+
+		dwc->dw_zero_count--;
+		list_del_init(&ue->ue_node);
+		spin_lock(&oi->ip_lock);
+		list_del_init(&ue->ue_ip_node);
+		spin_unlock(&oi->ip_lock);
+		kfree(ue);
  	}
 
-	if (end > i_size_read(inode)) {
+	if (!restart_func && end > i_size_read(inode)) {
  		ret = ocfs2_set_inode_size(handle, inode, di_bh, end);
  		if (ret < 0)
  			mlog_errno(ret);
  	}
+
  commit:
  	ocfs2_commit_trans(osb, handle);
+free_ac:
+	if (meta_ac) {
+		ocfs2_free_alloc_context(meta_ac);
+		meta_ac = NULL;
+	}
+	if (restart_func) {
+		restart_func = 0;
+		goto restart_all;
+	}
  unlock:
  	up_write(&oi->ip_alloc_sem);
  	ocfs2_inode_unlock(inode, 1);
  	brelse(di_bh);
  out:
-	if (data_ac)
-		ocfs2_free_alloc_context(data_ac);
-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);
  	ocfs2_run_deallocs(osb, &dealloc);
  	if (locked)
  		inode_unlock(inode);

Hi Alex,
I just reviewed your patch and a few questions were come up with.

Quoted 17 lines · alex chen

Hi John,

I think a better method to solve this problem.

Quoted 12 lines · John Lightsey

Quoted 9 lines · Changwei Ge

Can your tell me how did you format your volume?
What's your _cluster size_ and _block size_?
Your can obtain such information via debugfs.ocfs2 <your volume> -R 'stats' | grep 'Cluster Size'

It's better for you provide a way to reproduce this issue so that we can
perform some test.

Quoted 9 lines

Quoted 2 lines

The issue recurred in our cluster today, so at best my patch is just decreasing the frequency of the crashes.

We need to check the free number of the records in each loop to mark extent written, because the last extent block may be changed through many times marking extent written and the num_free_extents also be changed. In the worst case, the num_free_extents may become less than at the beginning of the loop. So we should not estimate the free number of the records at the beginning of the loop to mark extent written.

From John's description, ::dw_zero_count was not calculated properly
and your patch seems not to address this issue. Do you agree that it's possible that we get a wrong ::dw_zero_count?

Quoted 28 lines

I'd appreciate it if you could test the following patch and feedback the result.

Signed-off-by Alex Chen <[email protected]>

---
  fs/ocfs2/aops.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++---------
  1 file changed, 64 insertions(+), 12 deletions(-)

fs/ocfs2/aops.c +12 -0

index d151632..80725f2 100644
@@ -2272,6 +2272,35 @@ static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock,
  	return ret;
  }
 
+static int ocfs2_dio_should_restart(struct ocfs2_extent_tree *et,
+		struct ocfs2_alloc_context *meta_ac, int max_rec_needed)
+{
+	int status = 0, free_extents;
+
+	free_extents = ocfs2_num_free_extents(et);
+	if (free_extents < 0) {
+		status = free_extents;
+		mlog_errno(status);
+		return status;
+	}
+

fs/ocfs2/aops.c static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock, · line 2292

Quoted 6 lines

fs/ocfs2/aops.c +6 -0

+	/*
+	 * there are two cases which could cause us to EAGAIN in the
+	 * we-need-more-metadata case:
+	 * 1) we haven't reserved *any*
+	 * 2) we are so fragmented, we've needed to add metadata too
+	 *    many times.

What does point 2 mean?

fs/ocfs2/aops.c static int ocfs2_dio_wr_get_block(struct inode *inode, sector_t iblock, · line 2303

Quoted 12 lines

fs/ocfs2/aops.c +11 -0

+	 */
+	if (free_extents < max_rec_needed) {
+		if (!meta_ac || (ocfs2_alloc_context_bits_left(meta_ac)
+				< ocfs2_extend_meta_needed(et->et_root_el)))
+			status = 1;
+	}
+
+	return status;
+}
+
+#define OCFS2_MAX_REC_NEED_SPLIT 2

Um, I don't figure why the Maximum value is 2, I suppose this is less than previously estimated one.

Quoted 8 lines

fs/ocfs2/aops.c +0 -0

  static int ocfs2_dio_end_io_write(struct inode *inode,
  				  struct ocfs2_dio_write_ctxt *dwc,
  				  loff_t offset,

@@ -2281,14 +2310,13 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
  	struct ocfs2_extent_tree et;
  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
  	struct ocfs2_inode_info *oi = OCFS2_I(inode);

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2319

Quoted 10 lines

fs/ocfs2/aops.c +2 -3

-	struct ocfs2_unwritten_extent *ue = NULL;
+	struct ocfs2_unwritten_extent *ue = NULL, *tmp_ue;
  	struct buffer_head *di_bh = NULL;
  	struct ocfs2_dinode *di;
-	struct ocfs2_alloc_context *data_ac = NULL;
  	struct ocfs2_alloc_context *meta_ac = NULL;
  	handle_t *handle = NULL;
  	loff_t end = offset + bytes;
-	int ret = 0, credits = 0, locked = 0;
+	int ret = 0, credits = 0, locked = 0, restart_func = 0;

I think _ restart_func_ is not named properly. Can we change it into realloc or something like that?

Quoted 28 lines

fs/ocfs2/aops.c +4 -3

  	ocfs2_init_dealloc_ctxt(&dealloc);
 

@@ -2330,8 +2358,9 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
 
  	ocfs2_init_dinode_extent_tree(&et, INODE_CACHE(inode), di_bh);
 
-	ret = ocfs2_lock_allocators(inode, &et, 0, dwc->dw_zero_count*2,
-				    &data_ac, &meta_ac);
+restart_all:
+	ret = ocfs2_lock_allocators(inode, &et, 0, OCFS2_MAX_REC_NEED_SPLIT,
+				    NULL, &meta_ac);
  	if (ret) {
  		mlog_errno(ret);
  		goto unlock;

@@ -2343,7 +2372,7 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
  	if (IS_ERR(handle)) {
  		ret = PTR_ERR(handle);
  		mlog_errno(ret);
-		goto unlock;
+		goto free_ac;
  	}
  	ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), di_bh,
  				      OCFS2_JOURNAL_ACCESS_WRITE);

@@ -2352,7 +2381,17 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
  		goto commit;
  	}
 

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2386

Quoted 4 lines

fs/ocfs2/aops.c +3 -1

-	list_for_each_entry(ue, &dwc->dw_zero_list, ue_node) {
+	list_for_each_entry_safe(ue, tmp_ue, &dwc->dw_zero_list, ue_node) {
+		ret = ocfs2_dio_should_restart(&et, meta_ac,
+				OCFS2_MAX_REC_NEED_SPLIT * 2);

Why extent block's free record number would change during marking written, which compels code path has to re-alloc.

Thanks,
Changwei

Quoted 45 lines

fs/ocfs2/aops.c +26 -1

+		if (ret < 0) {
+			mlog_errno(ret);
+			break;
+		} else if (ret == 1) {
+			restart_func = 1;
+			break;
+		}
+
  		ret = ocfs2_mark_extent_written(inode, &et, handle,
  						ue->ue_cpos, 1,
  						ue->ue_phys,

@@ -2361,24 +2400,37 @@ static int ocfs2_dio_end_io_write(struct inode *inode,
  			mlog_errno(ret);
  			break;
  		}
+
+		dwc->dw_zero_count--;
+		list_del_init(&ue->ue_node);
+		spin_lock(&oi->ip_lock);
+		list_del_init(&ue->ue_ip_node);
+		spin_unlock(&oi->ip_lock);
+		kfree(ue);
  	}
 
-	if (end > i_size_read(inode)) {
+	if (!restart_func && end > i_size_read(inode)) {
  		ret = ocfs2_set_inode_size(handle, inode, di_bh, end);
  		if (ret < 0)
  			mlog_errno(ret);
  	}
+
  commit:
  	ocfs2_commit_trans(osb, handle);
+free_ac:
+	if (meta_ac) {
+		ocfs2_free_alloc_context(meta_ac);
+		meta_ac = NULL;
+	}
+	if (restart_func) {
+		restart_func = 0;
+		goto restart_all;
+	}
  unlock:
  	up_write(&oi->ip_alloc_sem);

fs/ocfs2/aops.c static int ocfs2_dio_end_io_write(struct inode *inode, · line 2436

Quoted 10 lines

fs/ocfs2/aops.c +0 -4

  	ocfs2_inode_unlock(inode, 1);
  	brelse(di_bh);
  out:
-	if (data_ac)
-		ocfs2_free_alloc_context(data_ac);
-	if (meta_ac)
-		ocfs2_free_alloc_context(meta_ac);
  	ocfs2_run_deallocs(osb, &dealloc);
  	if (locked)
  		inode_unlock(inode);

.

We need to check the free number of the records in each loop to mark extent written,
because the last extent block may be changed through many times marking extent written
and the num_free_extents also be changed. In the worst case, the num_free_extents may
become less than at the beginning of the loop. So we should not estimate the free
number of the records at the beginning of the loop to mark extent written.

I'd appreciate it if you could test the following patch and feedback the result.

Quoted 11 lines · alex chen

We need to check the free number of the records in each loop to mark extent written,
because the last extent block may be changed through many times marking extent written
and the num_free_extents also be changed. In the worst case, the num_free_extents may
become less than at the beginning of the loop. So we should not estimate the free
number of the records at the beginning of the loop to mark extent written.

Quoted 2 lines

I'd appreciate it if you could test the following patch and feedback the result.

I managed to reproduce the bug in a test environment using the following method. Some of the specific details here are definitely irrelevant:

- Setup a 20GB iscsi lun going to a spinning disk drive.

- Configure the OCFS cluster with three KVM VMs.

- Connect the iscsi lun to all three VMs.

- Format an OCFS2 partition on the iscsi lun with block size 1k and cluster size 4k.

- Mount the OCFS2 partition on one VM.

- Write out a 1GB file with a random pattern of 4k chunks. 4/5 of the 4k chunks are filled with nulls. 1/5 are filled with data.

- Run fallocate -d <filename> to make sure the file is sparse.

- Copy the test file so that the next step can be run repeatedly with copies.

- Use directio to rewrite the copy of the file in 64k chunks of null bytes.

In my test setup, the assertion failure happens on the next loop iteration after the number of free extents drops from 59 to 0. The call to ocfs2_split_extent() in ocfs2_change_extent_flag() is what actually reduces the number of free extents to 0. The count drops all at once in this case, not by 1 or 2 per loop iteration.

With your patch applied, it does handle this sudden reduction in the number of free extents, and it's able to entirely overwrite the 1GB file without any problems.

Is it safe to bring up a few nodes in our production OCFS2 cluster with the patched 4.9 kernel while the remainder nodes are running a 3.16 kernel?

The downtime required to switch our cluster forward to a 4.9 kernel and then back to a 3.16 kernel is hard to justify, but I can definitely test one or two nodes in our production environment if it will be a realistic test.

Hi John,
It's better to paste your patch directly into message body. It's easy for reviewing.

So I copied your patch below:

Quoted 21 lines

The dw_zero_count tracking was assuming that w_unwritten_list would always contain one element. The actual count is now tracked whenever the list is extended.

---
 fs/ocfs2/aops.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/ocfs2/aops.c +2 -0

index 88a31e9340a0..eb0a81368dbb 100644
@@ -784,6 +784,8 @@ struct ocfs2_write_ctxt {
 	struct ocfs2_cached_dealloc_ctxt w_dealloc;
 
 	struct list_head		w_unwritten_list;
+
+	unsigned int			w_unwritten_count;
 };
 
 void ocfs2_unlock_and_free_pages(struct page **pages, int num_pages)

fs/ocfs2/aops.c static int ocfs2_alloc_write_ctxt(struct ocfs2_write_ctxt **wcp, · line 878

Quoted 4 lines

fs/ocfs2/aops.c +1 -0

@@ -873,6 +875,7 @@ static int ocfs2_alloc_write_ctxt(struct ocfs2_write_ctxt **wcp,
 
 	ocfs2_init_dealloc_ctxt(&wc->w_dealloc);
 	INIT_LIST_HEAD(&wc->w_unwritten_list);
+	wc->w_unwritten_count = 0;

I think you don't have to evaluate ::w_unwritten_count to zero since kzalloc already did that.

fs/ocfs2/aops.c static int ocfs2_unwritten_check(struct inode *inode, · line 1379

Quoted 8 lines

fs/ocfs2/aops.c +1 -0

 	*wcp = wc;
 

@@ -1373,6 +1376,7 @@ static int ocfs2_unwritten_check(struct inode *inode,
 	desc->c_clear_unwritten = 0;
 	list_add_tail(&new->ue_ip_node, &oi->ip_unwritten_list);
 	list_add_tail(&new->ue_node, &wc->w_unwritten_list);
+	wc->w_unwritten_count++;

You increase ::w_unwritten_coun once a new _ue_ is attached to ::w_unwritten_list. So if no _ue_ ever is attached, ::w_unwritten_list is still empty. I think your change has the same effect with origin.

Moreover I don't see the relation between the reported crash issue and your patch change. Can you elaborate further?

Thanks,
Changwei

Quoted 6 lines

fs/ocfs2/aops.c +0 -0

 	new = NULL;
 unlock:
 	spin_unlock(&oi->ip_lock);

@@ -2246,7 +2250,7 @@ static int ocfs2_dio_get_block(struct inode *inode, sector_t iblock,
 		ue->ue_phys = desc->c_phys;
 

fs/ocfs2/aops.c static int ocfs2_dio_get_block(struct inode *inode, sector_t iblock, · line 2253

Quoted 3 lines

fs/ocfs2/aops.c +1 -1

 		list_splice_tail_init(&wc->w_unwritten_list, &dwc->dw_zero_list);
-		dwc->dw_zero_count++;
+		dwc->dw_zero_count += wc->w_unwritten_count;

Quoted 5 lines

fs/ocfs2/aops.c +0 -0

 	}
 
 	ret = ocfs2_write_end_nolock(inode->i_mapping, pos, len, len, wc);

Signature · 1 line

2.11.0

Quoted 9 lines · John Lightsey

In January Ben Hutchings reported Debian bug 841144 to the ocfs2-devel list:

https://oss.oracle.com/pipermail/ocfs2-devel/2017-January/012701.html

cPanel encountered this bug after upgrading our cluster to the 4.9 Debian stable kernel. In our environment, the bug would trigger every few hours.

Quoted 8 lines

The core problem seems to be that the size of dw_zero_list is not tracked correctly. This causes the ocfs2_lock_allocators() call in ocfs2_dio_end_io_write() to underestimate the number of extents needed. As a result, meta_ac is null when it's needed in ocfs2_grow_tree().

The attached patch is a forward-ported version of the fix we applied to Debian's 4.9 kernel to correct the issue.

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

Quoted 11 lines

Review

Quoted 11 lines

? new = NULL;
?unlock:
? spin_unlock(&oi->ip_lock);

(quoted diff) +0 -0

@@ -2246,7 +2250,7 @@ static int ocfs2_dio_get_block(struct inode
 *inode, sector_t iblock,
 ?		ue->ue_phys = desc->c_phys;

?
? list_splice_tail_init(&wc->w_unwritten_list,
&dwc->dw_zero_list);

(quoted diff) +1 -1

-		dwc->dw_zero_count++;
+		dwc->dw_zero_count += wc->w_unwritten_count;

I prefer using a loop to calculate 'dwc->dw_zero_count' rather than introducing a new variable as below:

list_for_each(iter, &wc->w_unwritten_list)

	dwc->dw_zero_count++;